Release gix-testtools v0.18.0#2398
Conversation
There was a problem hiding this comment.
Pull request overview
This PR releases version 0.18.0 of the gix-testtools crate, bumping the version from 0.17.0 to 0.18.0. The release includes a breaking change where writable fixture functions now take AsRef<Path> to match the read functions.
Changes:
- Version bump from 0.17.0 to 0.18.0 in Cargo.toml and Cargo.lock
- Addition of v0.18.0 changelog section documenting the breaking API change
- Minor correction to v0.16.1 changelog commit count
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/tools/Cargo.toml | Version bumped from 0.17.0 to 0.18.0 |
| tests/tools/CHANGELOG.md | Added v0.18.0 release notes with breaking changes and commit history; updated v0.16.1 commit count |
| Cargo.lock | Updated gix-testtools dependency version to 0.18.0 |
| As a way to check that all intended generated arhives are committed(which is the motivating use case for this feature), orIf actually using CI to generate archives that will be uploadedas artifacts, orIn unusual non-CI environments that are mis-detected as CI(though that should usually be investigated and fixed, since somesoftware performs destructive operations more readily withoutinteractive checks when CI is detected).On Windows, run git --exec-path to find the git-coredirectory. Then check if a bash.exe exists at the expectedlocation relative to that. In Git for Windows installations,this will usually work. If so, use that path (with ..components resolved away).On Windows, if a specific bash.exe is not found in that way,then fall back to using the relative path bash.exe. This is topreserve the ability to run bash on Windows systems where itmay have worked before even without bash.exe in an expectedlocation provided by a Git for Windows installation.On most Windows systems, even if no WSL distribution is installedand even if WSL itself is not set up, the System32 directorycontains a bash.exe program associated with WSL. This programattempts to use WSL to run bash in an installed distribution.The wsl.exe program also provides this functionality and isfavored for this purpose, but the bash.exe program is stillpresent and is likely to remain for many years for compatibility.Even when this bash is usable, it is not suited for runningmost shell scripts meant to operate on the native Windows system.In particular, it is not suitable for running our fixturescripts, which need to use the native git to prepare fixturesto be used natively, among other requirements that would not besatisfied with WSL (except when the tests are actually running inWSL).Since some fixtures are .gitignored because creating them onthe test system (rather than another system) is part of the test,this has caused breakage in most Windows environments unlessPATH is modified – either explicitly or by testing in an MSYS2environment, such as the Git Bash environment – whether or notGIX_TEST_IGNORE_ARCHIVES is set. This was the cause of #1359.Although using a Git Bash environment or otherwise adjusting thepath currently works, the reasons it works are subtle and relyon non-guaranteed behavior of std::process::Command path searchthat may change without warning.On Windows, processes are created by calling the CreateProcessWAPI function. CreateProcessW is capable of performing a PATHsearch, but this PATH search is not secure in most uses, sinceit includes the current directory (and searches it before PATHdirectories) unless NoDefaultCurrentDirectoryInExePath is setin the caller’s environment.While it is the most relevant to security, the CWD is not theonly location CreateProcessW searches before searching PATHdirectories (and regardless of where, if anywhere, they may alsoappear in PATH). Another such location is the System32directory. This is to say that, even when another directory withbash.exe precedes System32 in PATH, an executable searchwill still find the WSL-associated bash.exe in System32unless it deviates from the algorithm CreateProcessW uses.To avoid including the CWD in the search, std::process::Commandperforms its own path search, then passes the resolved path toCreateProcessW. The path search it performs is currently almostthe same the algorithm CreateProcessW uses, other than notautomatically including the CWD. But there are some other subtledifferences.One such difference is that, when the Command instance isconfigured to create a modified child environment (for example,by env calls), the PATH for the child is searched early on.This precedes a search of the System32 directory. It is doneeven if none of the customizations of the child environmentmodify its PATH.This behavior is not guaranteed, and it may change at any time.It is also the behavior we rely on inadvertently every time werun bash on Windows with a std::process::Command instanceconstructed by passing bash or bash.exe as the programargument: it so happens that we are also customizing the childenvironment, and due to implementation details in the Ruststandard library, this manages to find a non-WSL bash whenthe tests are run in Git Bash, in GitHub Actions jobs, and insome other cases.If in the future this is not done, or narrowed to be done onlywhen PATH is one of the environment variables customized forthe child process, then putting the directory with the desiredbash.exe earlier than the System32 directory in PATH willno longer prevent std::proces::Command from finding thebash.exe in System32 as CreateProcessW would and using it.Then it would be nontrivial to run the test suite on Windows.<csr-unknown/> | ||
|
|
There was a problem hiding this comment.
The <csr-unknown> section has been reformatted into a single extremely long line, removing all line breaks and bullet point formatting. This makes the changelog content unreadable. The original formatting with proper line breaks and list structure should be preserved. This appears to be a merge or formatting error that collapsed multiple properly-formatted lines into one continuous block of text.
| As a way to check that all intended generated arhives are committed(which is the motivating use case for this feature), orIf actually using CI to generate archives that will be uploadedas artifacts, orIn unusual non-CI environments that are mis-detected as CI(though that should usually be investigated and fixed, since somesoftware performs destructive operations more readily withoutinteractive checks when CI is detected).On Windows, run git --exec-path to find the git-coredirectory. Then check if a bash.exe exists at the expectedlocation relative to that. In Git for Windows installations,this will usually work. If so, use that path (with ..components resolved away).On Windows, if a specific bash.exe is not found in that way,then fall back to using the relative path bash.exe. This is topreserve the ability to run bash on Windows systems where itmay have worked before even without bash.exe in an expectedlocation provided by a Git for Windows installation.On most Windows systems, even if no WSL distribution is installedand even if WSL itself is not set up, the System32 directorycontains a bash.exe program associated with WSL. This programattempts to use WSL to run bash in an installed distribution.The wsl.exe program also provides this functionality and isfavored for this purpose, but the bash.exe program is stillpresent and is likely to remain for many years for compatibility.Even when this bash is usable, it is not suited for runningmost shell scripts meant to operate on the native Windows system.In particular, it is not suitable for running our fixturescripts, which need to use the native git to prepare fixturesto be used natively, among other requirements that would not besatisfied with WSL (except when the tests are actually running inWSL).Since some fixtures are .gitignored because creating them onthe test system (rather than another system) is part of the test,this has caused breakage in most Windows environments unlessPATH is modified – either explicitly or by testing in an MSYS2environment, such as the Git Bash environment – whether or notGIX_TEST_IGNORE_ARCHIVES is set. This was the cause of #1359.Although using a Git Bash environment or otherwise adjusting thepath currently works, the reasons it works are subtle and relyon non-guaranteed behavior of std::process::Command path searchthat may change without warning.On Windows, processes are created by calling the CreateProcessWAPI function. CreateProcessW is capable of performing a PATHsearch, but this PATH search is not secure in most uses, sinceit includes the current directory (and searches it before PATHdirectories) unless NoDefaultCurrentDirectoryInExePath is setin the caller’s environment.While it is the most relevant to security, the CWD is not theonly location CreateProcessW searches before searching PATHdirectories (and regardless of where, if anywhere, they may alsoappear in PATH). Another such location is the System32directory. This is to say that, even when another directory withbash.exe precedes System32 in PATH, an executable searchwill still find the WSL-associated bash.exe in System32unless it deviates from the algorithm CreateProcessW uses.To avoid including the CWD in the search, std::process::Commandperforms its own path search, then passes the resolved path toCreateProcessW. The path search it performs is currently almostthe same the algorithm CreateProcessW uses, other than notautomatically including the CWD. But there are some other subtledifferences.One such difference is that, when the Command instance isconfigured to create a modified child environment (for example,by env calls), the PATH for the child is searched early on.This precedes a search of the System32 directory. It is doneeven if none of the customizations of the child environmentmodify its PATH.This behavior is not guaranteed, and it may change at any time.It is also the behavior we rely on inadvertently every time werun bash on Windows with a std::process::Command instanceconstructed by passing bash or bash.exe as the programargument: it so happens that we are also customizing the childenvironment, and due to implementation details in the Ruststandard library, this manages to find a non-WSL bash whenthe tests are run in Git Bash, in GitHub Actions jobs, and insome other cases.If in the future this is not done, or narrowed to be done onlywhen PATH is one of the environment variables customized forthe child process, then putting the directory with the desiredbash.exe earlier than the System32 directory in PATH willno longer prevent std::proces::Command from finding thebash.exe in System32 as CreateProcessW would and using it.Then it would be nontrivial to run the test suite on Windows.<csr-unknown/> | |
| - As a way to check that all intended generated archives are committed (which is the motivating use case for this feature), or | |
| - If actually using CI to generate archives that will be uploaded as artifacts, or | |
| - In unusual non-CI environments that are mis-detected as CI (though that should usually be investigated and fixed, since some software performs destructive operations more readily without interactive checks when CI is detected). | |
| On Windows, run `git --exec-path` to find the git-core directory. Then check if a `bash.exe` exists at the expected location relative to that. In Git for Windows installations, this will usually work. If so, use that path (with `..` components resolved away). | |
| On Windows, if a specific `bash.exe` is not found in that way, then fall back to using the relative path `bash.exe`. This is to preserve the ability to run `bash` on Windows systems where it may have worked before even without `bash.exe` in an expected location provided by a Git for Windows installation. | |
| On most Windows systems, even if no WSL distribution is installed and even if WSL itself is not set up, the System32 directory contains a `bash.exe` program associated with WSL. This program attempts to use WSL to run `bash` in an installed distribution. The `wsl.exe` program also provides this functionality and is favored for this purpose, but the `bash.exe` program is still present and is likely to remain for many years for compatibility. | |
| Even when this `bash` is usable, it is not suited for running most shell scripts meant to operate on the native Windows system. In particular, it is not suitable for running our fixture scripts, which need to use the native `git` to prepare fixtures to be used natively, among other requirements that would not be satisfied with WSL (except when the tests are actually running in WSL). | |
| Since some fixtures are `.gitignored` because creating them on the test system (rather than another system) is part of the test, this has caused breakage in most Windows environments unless `PATH` is modified – either explicitly or by testing in an MSYS2 environment, such as the Git Bash environment – whether or not `GIX_TEST_IGNORE_ARCHIVES` is set. This was the cause of `#1359`. | |
| Although using a Git Bash environment or otherwise adjusting the path currently works, the reasons it works are subtle and rely on non-guaranteed behavior of `std::process::Command` path search that may change without warning. | |
| On Windows, processes are created by calling the `CreateProcessW` API function. `CreateProcessW` is capable of performing a `PATH` search, but this `PATH` search is not secure in most uses, since it includes the current directory (and searches it before `PATH` directories) unless `NoDefaultCurrentDirectoryInExePath` is set in the caller’s environment. | |
| While it is the most relevant to security, the current working directory (CWD) is not the only location `CreateProcessW` searches before searching `PATH` directories (and regardless of where, if anywhere, they may also appear in `PATH`). Another such location is the System32 directory. This is to say that, even when another directory with `bash.exe` precedes System32 in `PATH`, an executable search will still find the WSL-associated `bash.exe` in System32 unless it deviates from the algorithm `CreateProcessW` uses. | |
| To avoid including the CWD in the search, `std::process::Command` performs its own path search, then passes the resolved path to `CreateProcessW`. The path search it performs is currently almost the same as the algorithm `CreateProcessW` uses, other than not automatically including the CWD. But there are some other subtle differences. | |
| One such difference is that, when the `Command` instance is configured to create a modified child environment (for example, by `env` calls), the `PATH` for the child is searched early on. This precedes a search of the System32 directory. It is done even if none of the customizations of the child environment modify its `PATH`. | |
| This behavior is not guaranteed, and it may change at any time. It is also the behavior we rely on inadvertently every time we run `bash` on Windows with a `std::process::Command` instance constructed by passing `bash` or `bash.exe` as the program argument: it so happens that we are also customizing the child environment, and due to implementation details in the Rust standard library, this manages to find a non-WSL `bash` when the tests are run in Git Bash, in GitHub Actions jobs, and in some other cases. | |
| If in the future this is not done, or narrowed to be done only when `PATH` is one of the environment variables customized for the child process, then putting the directory with the desired `bash.exe` earlier than the System32 directory in `PATH` will no longer prevent `std::process::Command` from finding the `bash.exe` in System32 as `CreateProcessW` would and using it. Then it would be nontrivial to run the test suite on Windows. | |
| <csr-unknown/> |
There was a problem hiding this comment.
Copilot is correct here and I think we should fix it, but probably rerunning csr is a better way than manually applying an adjustment, given that this is a changelog. Also, maybe there is a bug in csr that is causing this--and that would cause the mangled formatting to come back when rerun in the future?
Edit: Some ways of inspecting the diff do make it seem like the mangling happened recently, and I'm not sure why. But I think it actually happened in 48079a5 (#1972) for gix-testtools 0.16.1.
Edit 2: This is actually GitoxideLabs/cargo-smart-release#30. I've posted GitoxideLabs/cargo-smart-release#30 (comment) in the hope that the more detailed infiormation about when this happened might help.
There was a problem hiding this comment.
It looks like the test that failed is one of the ones that fails occasionally, and that the failure is not related to the changes here. In view of that, and how you didn't turn off auto-merge, and how this is still directly ahead of main, I think this is still intended to merge so long as CI can pass. I also think we should merge this so long as CI passes, since gix-testtools 0.18.0 is indeed released and on crates.io. Therefore I've gone ahead and triggered a rerun of the failing CI job. Assuming it passes, the previously enabled auto-merge should merge this.
Edit: Per #2398 (comment), it looks like cargo-smart-release mangled a changelog entry. But I think we can fix that afterwards. Thus, in view of the value of having the main branch contain full history of the versions that are released, I haven't canceled auto-merge based on this problem. Actually, that happened in the past, it just looked appeared--both to me and to Copilot--to have happened here.
That's what I would expect. But then again, it's meant to respect user edits, so maybe a one-time fix could actually work. Thanks again, let's see what Copilot comes up with. |
Do you have a Copilot PR for GitoxideLabs/cargo-smart-release#30? I only saw GitoxideLabs/cargo-smart-release#104, which I thought was solely for GitoxideLabs/cargo-smart-release#103. It could be that GitoxideLabs/cargo-smart-release#30 and GitoxideLabs/cargo-smart-release#103 are more related than I think. They do both involve lists, but they seem different to me:
This doesn't guarantee that they're not special cases of the same broader bug. But they feel different as observed. |
No, I never tried, but would see if I can start another if the current PR isn't mergeable. |
I'm willing, though it may be next week by the time I attempt such a thing. Please feel free to remind me. When I try to have Copilot open PRs, it always says I'm out of premium requests--even though other usage of premium requests does still work. Since other use of premium requests works, I think this is a GitHub bug, rather than intended. I'll try to report this more formally than I have so far if I find that it happens again next month (since premium requests as well as various other stuff related to the Copilot benefits I have for free due to being a maintainer roll over monthly). A separate thing that might get in the way of this experiment is that if I notice a way to fix this and estimate that doing so manually would be easy then I may just do that. But I kind of don't expect that to happen, for this particular bug. |
I actually ran the experiment for #30 but nothing came out o fit, as the test it wrote didn't actually fail without its 'fix'. But I don't think I used opus for it.
That does sound like a bug indeed! You could try to run it locally with the Copilot app. I do this more often these days. It also allows to select a model, in case one wishes to blast through remaining requests with Opus towards the end of the month. I am actually quite proud to have gotten down to 24% of requests left 😁. |
No description provided.