Using the latest shell() or related facility in bash_program()

[EliahKagan]

Summary 💡

The limitation described in #1510 (comment), where gix-testtools cannot depend on current versions of gix-* crates, requires that gix-testtools depend on an old gix-path that may not have all the same facilities and bug fixes. In some cases, this is no problem:

gitoxide/tests/tools/src/lib.rs

Line 222 in 6d5f774

// TODO(deps): Upgrading dependencies will require changing `Exponential` to `Quadratic`.

In other cases, however, it prevents a feature from being used in gix-testtools at the time it is being developed:

gitoxide/tests/tools/src/lib.rs

Line 657 in 6d5f774

// TODO(deps): Once `gix_path::env::shell()` is available, maybe do `shell().parent()?.join("bash.exe")`

This gets worse if the facility becomes available, gix-testtools is changed to use it, and the facility's behavior changes. Then the effect is not obvious, and also bugs remain that would intuitively be thought of as fixed within this repository.

But right now I am more concerned about the missed opportunity, as applied to the shell-finding functionality of gix-path specifically. This is something I suspect will, and should, be iterated on with small gradual changes. Being able to use it immediately in gix-testtools would "test drive" the changes using gitoxide's test fixtures, which I expect would be very helpful. In particular, it would make me a lot less nervous about trying to make improvements to this functionality, including as may be needed to fix significant bugs in the shorter term, and for implementing other improvements such as environment customization for subprocesses (discussed in #1868) for the longer term. I expound on the anticipated benefits, and how they relate to some other ongoing work, in the "Motivation" section below.

Even before eventually alleviating the limitation in cargo-smart-release--which I assume should eventually be done, but which I don't know how to do, and which seems nontrivial to implement and nontrivial to test--I think it might be possible to use some form of dependency injection to give gix-testtools the result of shell().

Here are some ideas. They are grouped by relatedness, not in the order of how good or bad they are. (Unfortunately none of them seem especially good to me.)

Having test modules call a function in gix-testtools

gix_testtools could provide an initialization function which tests are expected to call to pass a path or a Fn that can be called to get one. If not called, a fallback value found in a simpler way would be used, unless an environment variable is present that signifies that the absence of initialization is to be treated as a hard error and result in a panic.

Give gix-testtools a non-default feature where it tries to call a no_mangle function

Each crate's test suite that uses fixtures would, in the test configuration, define an extern function marked no_mangle as a weak symbol. gix-testtools would have a non-default feature that tries to call shell() through it. A macro could be used to decrease repetition.

I am not sure if this is feasible. My guess is that it would be unsafe. My guess is that it would require the use of unstable Rust (rust-lang/rust#29603). I don't know enough Rust to understand all the problems with this idea, and my guess is that they are fairly serious. I mention it in case they are somehow less serious than I estimate.

Do something like that but with the Windows API

Although, per #1869, it might turn out that gix-path should do something nontrivial for finding a shell even outside Windows, it currently doesn't. Also, that wouldn't be necessary for bash_program() in gix-testtools, because possibly unlike sh, on a Unix-like system we should pretty much always be able to use bash as found by a path search, if bash is at all available. (Outside Windows, if bash exists but isn't in a PATH directory, the user probably doesn't intend that it be found.)

Therefore, this seems like a Windows-specific problem. Windows provides its own facilities for making code available between software components that would otherwise have difficulty sharing implementations. For example, we could register a COM object! I do not think this is the way to go.

Expose shell() in an executable, which gix-testtools calls if present

For example, internal-tools could depend on current gix-path and provide a subcommand to give the path, and gix_testtools::bash_program() could--when a new non-default feature for it is enabled--look for an executable in an expected place for internal-tools and attempt to run it and use the result.

The problem is that if the executable is not built, or is built for the wrong version or from the wrong feature branch or something, we use the wrong implementation.

Expose shell() by reading an environment variable the user must set manually

This is the simplest, but it is also more laborious to use, such that it seems to me that it is only marginally a solution: gix_testtools could consult an environment variable for where to find bash, which the user could set to a value known to be appropriate. Since I want to use this to test that gix_path::shell() gives a (shell path) value where (shell path) or (shell path)/../bash.exe runs the test suite properly, I could use it that way.

I expect that the burden in using this would lead it to be used less often (including by me) and thus for regressions in gix_path to be less often discovered.

Having gix-testtools vendor gix-path

gix-testtools could gain a new non-default feature where, when enabled, gix-testtools includes an internal copy of the current gix-path. I think this could be implemented in a build.rs for gix-testtools. If necessary, I guess it could be implemented by auto-generating a copy and maintaining it, sort of like the situation with gix-packetline and gix-packetline-blocking. This could also be done with other gix-* crates gix-testtools relies on, if necessary.

For any crates vendored this way, if they are found to have vulnerabilities, the vulnerabilities would separately affect gix-testtools unless gix-testtools can be known only to use them in ways the vulnerabilities are not exploitable. Thus new versions of gix-testtools would have to be released to fix them, and separate RUSTSEC advisories would have to be issued for the gix-testtools-vendored version of the vulnerability (though they could have almost the same text).

That seems undesirable, but I am uncertain how it compares to the current situation where gix-testtools can depend on a vulnerable version of a gix-* dependency, or where extra versions would have to be created to make it so two non-vulnerable versions exist. I think this is what happened in #1473 (reply in thread). Vendoring might be preferable.

Generating separate testtools versions of gix-path

This is the same as the previous idea but instead of vendoring them as part of gix-testtools they could be their own separate crates. This is analogous to the situation with gix-packetline and gix-packetline-blocking, though, if this were done, then to avoid confusion they should still not be side by side, and the copies should not be published.

This seems worse because, while it involves less nesting (sort of), the feature would be unavailable--or would be hard to make work--for users of gix-testtools other than this project.

Motivation 🔦

This would be useful in #1864, which could then be implemented in terms of facilities improved in #1862 with no version skew and little to no code duplication.

To be clear, this does not block either of #1862 or #1864. I consider attempting to do anything like this is outside the scope of both. I intend to use a weaker technique in #1864 than gix-testtools might be able to use later if this feature is implemented.

The specific experience that motivates this

Originally, in #1862, I planned not to use the shim for sh on Windows. By testing locally outside of a Git Bash environment, I discovered that--unless we make other changes that are beyond the scope of that PR--the shim should be used instead. The approach I originally intended there--of keeping the preexisting gix_path::env::shell() behavior of using the non-shim, while changing gix-command to use shell()--was broken in a way that CI did not discover. But it also just good luck that I found it when testing locally, since only one test failed.

In contrast, the analogous change in #1864 brought about far more failures (even without GIX_TEST_IGNORE_ARCHIVES). Because I happened to be working on #1864 at the same time, I discovered that. So if the local test failure in #1862 had not occurred, I would likely have found out about the problem before a release was made. Letting gix-testtools use changes to gix_path::env::shell() or related functions immediately would make this safeguard something we always get. Otherwise, we release gix-path crates before we test them on a wide variety of realistic and non-trivial shell() use cases.

However, in this case the mistake could also have been caught if I had kept in mind that anything usually done with a shim may depend on an environment that might not be set up otherwise. I had gotten into the bad habit of not attending to that, because with git itself it is usually okay (ever since git-for-windows/git#2506) to forgo the shim, and I had been thinking more about git than shells. Therefore, the above point arguably does not quite establish the future importance of this feature. Accordingly, the following states the motivation in broader terms not specific to this experience.

For design

If we want to figure out how it should work, especially if it is to be generalized to find more commands than sh--at least bash, and perhaps others--then being able to use it from gix-testtools would help with that.

Included in #1862 is a gix_path::env::auxiliary module with the beginning of a general facility for finding programs in a way that checks for the program that should be used with git first. It would already work to make a reasonable choice for many, though not all, executables provided by Git for Windows. Currently it is proposed only as an implementation detail of the proposed new implementation of gix_path::env::shell():

gitoxide/gix-path/src/env/mod.rs

Lines 40 to 49 in 93f0804

	pub fn shell() -> &'static OsStr {
	static PATH: Lazy<OsString> = Lazy::new(|| {
	if cfg!(windows) {
	auxiliary::find_git_associated_windows_executable_with_fallback("sh")
	} else {
	"/bin/sh".into()
	}
	});
	PATH.as_ref()
	}

But find_git_associated_windows_executable() and find_git_associated_windows_executable_with_fallback() should be feasible, and not too complex, to generalize further. (See comments in auxiliary.rs in #1862 for details.) They already should work well for sh, and they should work and at least as well for bash as anything we have done so far. I think being able to use the paths they find in gix-testtools before making releases would help in figuring out what the design should be, and also in testing the implementation.

For testing

Even after we know, or think we know, exactly how it should work, running fixture scripts with a shell path obtained via gix-path is a great way to find bugs that may lurk in its implementation. It would allow all changes to gix_path::env::shell()--and any related more general facilities that might be introduced later in gix-path--to be exercised by the entire test suite.

Specifically, running the test suite with GIX_TEST_IGNORE_ARCHIVES=1 would run all the fixture scripts, except the few that are only used by tests that do not run on the current platform, with a shell found the same way gix_path::env::shell() finds sh, using a path fo the same style as what it uses for sh, with only the command name at the end differing. Any newly failing tests, or tests that currently fail but start passing, would give insight into the effect of the change in gix-path. This could then be acted on before the change makes it into a gix-path release.

(On Windows, that we get sh from gix_path::env::shell() versus bash in gix_testtools::bash_program() is, for now, almost inconsequential, due to #1868. But even once that is fixed, it will be small enough that the effects on bash_program() will be relevant to shell().)

[Byron]

Thanks a lot for the write-up! I also agree that using the latest gix-path in gix-testtools is very beneficial.

Indeed it would be great if the gix-testtools issue in conjunction with cargo smart-release could just be fixed, but I'd also not have the capability to do it. The key to a fix is testing, and I guess one would have to setup a (mock-)registry to actually publish things into, to truly be sure. Maybe I am wrong about that and weaker tests would also work, just doing a dry-run publish. The situation certainly is a bit sad, especially since I'd be inclined to think that it can be solved if someone would put their mind to it (including myself).

I personally also lean towards auto-vendoring gix-path as part of the build process of gix-testtools, while hoping that doing so might somewhat simplify advisories if one can avoid to duplicate them.

[EliahKagan]

The key to a fix is testing, and I guess one would have to setup a (mock-)registry to actually publish things into, to truly be sure.

Is that currently blocked by Byron/cargo-smart-release#18?

I personally also lean towards auto-vendoring gix-path as part of the build process of gix-testtools, while hoping that doing so might somewhat simplify advisories if one can avoid to duplicate them.

Can this be done in build.rs for gix-testtools? I had hoped so, when I wrote the above, but for gix-testtools to actually be usable outside the project, it has to be able to build and install the crate without anything other than the crate as distributed on crates.io, and the dependencies the crate lists in Cargo.toml.

$ wget -qO- https://crates.io/api/v1/crates/gix-testtools/0.15.0/download | tar tzf -
gix-testtools-0.15.0/.cargo_vcs_info.json
gix-testtools-0.15.0/Cargo.lock
gix-testtools-0.15.0/Cargo.toml
gix-testtools-0.15.0/Cargo.toml.orig
gix-testtools-0.15.0/LICENSE-APACHE
gix-testtools-0.15.0/LICENSE-MIT
gix-testtools-0.15.0/src/lib.rs
gix-testtools-0.15.0/src/main.rs

But as far as I know, we can't solve the problem by listing gix-path as a dependency in Cargo.toml to use it in build.rs, because the problem is that we want gix-testtools to always use a newer version of gix-path than cargo-smart-release will currently allow to be a dependency of gix-testtools. (Unless this does not apply when it is a build dependency. I presume it does, though, and if it doesn't then making it a build dependency would presumably be a workaround by itself even without any vendoring.)

[Byron]

The key to a fix is testing, and I guess one would have to setup a (mock-)registry to actually publish things into, to truly be sure.

Is that currently blocked by Byron/cargo-smart-release#18?

I'd think so, even though I'd hope the actual fix is relatively straightforward as there isn't any logic that would (explicitly) impose constraints on the registry.

But as far as I know, we can't solve the problem by listing gix-path as a dependency in Cargo.toml to use it in build.rs, because the problem is that we want gix-testtools to always use a newer version of gix-path than cargo-smart-release will currently allow to be a dependency of gix-testtools. (Unless this does not apply when it is a build dependency. I presume it does, though, and if it doesn't then making it a build dependency would presumably be a workaround by itself even without any vendoring.)

Maybe it could be a build-dependency, as I think cargo smart-release will ignore them simply because gitoxide doesn't have build scripts that depend on workspace crates. I am not sure though, but maybe that's something that can be tested sufficiently well by using the various --no- flags when invoking cargo smart-release. I think there definitely is a chance that exploiting the right shortcomings of cargo smart-release would allow the build script to work.