Can we have the old API back?

[jplatte]

I recently saw the announcement of xshell 0.2. I'm a very happy user of 0.1 and can not see what having to keep around a Shell instance buys me. I wasn't actually aware that the process-global environment was being changed previously (though I never touched the probably more problematic pushenv, only pushd and cmd!), I would have assumed that xshell has its own global / thread-local / whatever state tracking that its commands use.

Would you consider returning cmd! without the leading sh argument and global pushd (plus maybe pushenv) as a layer on top of the new Shell API?

[thomcc]

Hmm, yeah. The fact that changing the directory with xshell doesn't actually change the directory is really error prone. You need to be sure you always do sh.current_dir().join(path) before passing the path somewhere else, which is easy to forget, for someone not to know its needed.

That said, for the env I get it (needed to happen, for better or worse), and while I preferred not needing to pass the Shell into the cmd!, it isn't the end of the world.

And like, I do get that the path change is probably better semantics for multithreaded use. But... in the cases where I've used this library, that kind of concurrency bug isn't that likely¹ than messing the path up here, so it seems like an unfortunate call (But I get that having only some of the state be global would be pretty weird, and would probably mean you need to do some locking...)

I'll have to think about it. I really liked 0.1.x -- it's so nice to use and gets a lot right. So maybe I'll come around to this, it just feels like this is exactly the kind of bug I tend to end up making.

Footnotes

1. I don't think I've ever wanted to thread::spawn from inside a xtask. (Maybe I've called libraries that do, like cc in some configurations, but they aren't setting the cwd from those threads) ↩

[thomcc]

So, thinking about it more, this change certainly makes the use of xshell from inside "real programs" much less dubious (even if those programs use it from multiple threads). And honestly, looking at https://crates.io/crates/xshell/reverse_dependencies, several of these are "real programs" -- real enough for my book anyway.

So yeah, it's hard for me to really act like the old semantics would be really ideal for these programs, and these semantics are consistent -- changes to the state are entirely local to the Shell. While it seemed error-prone, perhaps this is just because I came in knowing how it used to work.

---

Sadly, I think how it used to work is still... kinda better for the cases¹ where I reach for xshell. I mean, I do think the change is probably for the best, but it makes me wonder, perhaps there's area for a design more focused on that case.

That is, what would a xshell-style library which explicitly didn't care about use from "real programs" would look like -- one that was focused on the xtask/repo tooling/one-off scripting use case, even if this means it's not useful in other cases.

I guess something like that would end up being... an opinionated² library for rust scripts and other xtasky things? Is there a way to do such a thing that doesn't totally defeat the point?

It's got me thinking a bit, maybe I'll experiment with it... (If nothing else, there's some stuff I end up doing every time I write that style of xtasky stuff). (This didn't pan out and I still use xshell, FWIW)

Footnotes

1. Small scripts written in Rust, often as part of some automation of dev tooling located within a repo -- often an xtask (but not always)... ↩

2. Hopefully only mildly opinionated? I mean, too opinionated and it'd end up being too limited, I think. ↩

[thomcc]

So, I've been using xshell 0.2 for a while now and I think the situation with the CWD is the worst part of this -- the env changes are better this way IMO (but I didn't like mutating the environment in the first place).

I can't speak for others, but its kind of... genuinely error prone for me, and I have to be extremely careful about any use of Paths. The situation that occurs is usually something like:

1. I display a path directly rather than via sh.current_dir().join(path).display(). This ends up being confusing/misleading, but tends not to cause actual bugs, since most of the output of an xtask is just diagnostics (this is the case for mine, anyway).

2. I call an inherent method on std::path::Path (like exists, canonicalize, ...) rather than either using a method on Shell, or getting the absolute path by joining it on the Shell's CWD first.

3. I pass a path to a function that doesn't know about xshell. This may be something in std::fs, or it may be something that uses it internally -- recently I moved some ucd-parse-using code into an (xshell-using) xtask, and forgot to account for this. Debugging this wasn't that bad, but it was fairly annoying none-the-less.

It's particularly easy to make this sort of mistake when converting code that doesn't use xshell to code that does. That said it's not limited to this, as it's also easy to miss in review too (not to mention cases like dbg! printing).

While it has gotten rarer that I make this kind of mistake, honestly it still occasionally causes issues even several months on (admittedly it's not like I am working on xtask code every day -- perhaps this is part of the issue though, if it was more regular it may be easier to avoid).

I think probably the best approach for me might be to just live with not changing the CWD after creating a Shell (I already do the initial cd-to-project-root prior to setting up the xshell::Shell, although probably I could just error on use from subdir instead). I guess I could also just use absolute paths always, but this is also not ideal.

This kinda feels like I'm fighting the lib though, so I felt I should come back to this and say something.

---

FWIW, unlike the process environment (which should basically never be written to), I'm not 100% sure why the current directory was moved into the Shell (I don't think it has the same thread-safety concerns as environment variables), so my gut feeling is to suggest that it's better off with that still being global (in an xshell 0.3 I suppose...).

While this may be error-prone if you have several threads in an xtask, it would only cause issues if you want each one to have their own current working directory, and could be avoided by not changing the working directory in that situation (not ideal, but it sounds somewhat uncommon... perhaps I'm mistaken, though). Which is the same situation I'm in, even in the single-threaded use case...

That said, maybe there's some other approach you prefer (or maybe I'm underestimating how important the "many threads which change their working directories" use case is).

[WaffleLapkin]

Not sure how important/common this is, but I certainly had cases where I needed multiple Shell instances with different working directories, so there is that 🤷

[RalfJung]

FWIW I'm also quite happy that I can change_dir a shell without mutating any kind of global state.