GitoxideLabs / gitoxide

An idiomatic, lean, fast & safe pure Rust implementation of Git
Apache License 2.0
8.91k stars 303 forks source link

The 32-bit Windows releases are 64-bit #1472

Closed EliahKagan closed 2 months ago

EliahKagan commented 2 months ago

Current behavior 😯

On the GitHub release pages where binaries can be downloaded, the four archives named to indicate that they are i686-pc-windows-msvc builds are actually all x86_64-pc-windows-msvc builds.

I don't believe this is specific to recent versions. But in the current latest version the affected archives are:

Downloading these releases from a releases page or using gh release download, or attempting to automatically download and install them using cargo binstall, obtains 64-bit executables instead.

This problem appears specific to the 32-bit Windows releases. Other releases all appear to be of the claimed architecture.

The cause?

The release workflow contains win32-msvc jobs that are meant to build and produce archives of 32-bit Windows executables. But this is producing archives of 64-bit executables instead.

I'm not sure exactly what is going on. It seems to me from the build output that those jobs' behavior may be accidentally equivalent in effect to that of the win-msvc jobs--that is, they may be building the wrong target. But I may just be reading the build output wrong.

However, separately from the compilation itself, it looks like there is a bug in the "Build archive" step, where it accounts for the target that was built on Unix-like systems but does not account for it on Windows:

https://github.com/Byron/gitoxide/blob/55cffe4ecf78bfea76059b742fafaebe964a725f/.github/workflows/release.yml#L203-L211

That shows that, on Windows, the files being copied for inclusion in the archive are specified as:

target/release/${{ env.EXE_NAME }}.exe target/release/gix.exe

While, in contrast, on other operating systems, they are specified as:

target/${{ matrix.target }}/release/${{ env.EXE_NAME }} target/${{ matrix.target }}/release/gix

If that is not the full cause, or the needed fix is not immediately apparent when you look at it, then I would be pleased to investigate further and try to fix it.

I am not sure of the best way is to experiment with this, but it looks feasible. In my fork, I could remove the "Upload release archive" step check the output in some other way. Or if necessary I could leave it in and publish "releases" from my fork while testing.

Expected behavior 🤔

The releases for Windows marked i686 should target that architecture rather than x86_64.

(If some builds with particular feature configuration cannot be produced, then they can simply be omitted. But I suspect they all can be built. In particular, https://github.com/rust-lang/libz-sys/issues/197 does not seem to affect local cross-compilation from a 64-bit Windows machine.)

Git behavior

32-bit builds of Git for Windows offered for download are 32-bit, targeting i686.

Steps to reproduce 🕹

This can be verified manually, by attempting to install a 32-bit build with cargo binstall and examining the result, or semi-automatically by downloading all archives of a release and checking which of them match the archive names.

Demonstration with cargo binstall

This is from a 32-bit Windows 10 system:

PS C:\Users\ek> cargo binstall gitoxide
 INFO resolve: Resolving package: 'gitoxide'
 WARN The package gitoxide v0.37.0 (i686-pc-windows-msvc) has been downloaded from github.com
 INFO This will install the following binaries:
 INFO   - ein.exe (ein.exe -> C:\Users\ek\.cargo\bin\ein.exe)
 INFO   - gix.exe (gix.exe -> C:\Users\ek\.cargo\bin\gix.exe)
Do you wish to continue? yes/[no]
? y
 INFO Installing binaries...
 INFO Done in 26.3840891s
PS C:\Users\ek> gix
ResourceUnavailable: Program 'gix.exe' failed to run: An error occurred trying to start process 'C:\Users\ek\.cargo\bin\gix.exe' with working directory 'C:\Users\ek'. The specified executable is not a valid application for this OS platform.At line:1 char:1
+ gix
+ ~~~.
PS C:\Users\ek> ein
ResourceUnavailable: Program 'ein.exe' failed to run: An error occurred trying to start process 'C:\Users\ek\.cargo\bin\ein.exe' with working directory 'C:\Users\ek'. The specified executable is not a valid application for this OS platform.At line:1 char:1
+ ein
+ ~~~.
PS C:\Users\ek> &'C:\Program Files\Git\usr\bin\file' 'C:\Users\ek\.cargo\bin\gix.exe'
C:\Users\ek\.cargo\bin\gix.exe: PE32+ executable (console) x86-64, for MS Windows
PS C:\Users\ek> &'C:\Program Files\Git\usr\bin\file' 'C:\Users\ek\.cargo\bin\ein.exe'
C:\Users\ek\.cargo\bin\ein.exe: PE32+ executable (console) x86-64, for MS Windows

Demonstration checking binaries in all archives of a release

Downloading all archives of a release can be done with the gh command. I did the following in PowerShell on a Windows system with 7-Zip installed (providing the 7z command) and with the MSYS2 file command made available in the PATH:

mkdir gitoxide-v0.37.0-release
cd gitoxide-v0.37.0-release
gh release -R byron/gitoxide download v0.37.0
gci | %{ 7z x $_ }
gci *.tar | %{ 7z x $_ }
file */gix* */ein*
file */gix* */ein* | sls -raw i686

This confirmed that all i686-pc-windows-msvc archives were affected, with their executables being x86_64 rather than i686, and that no other archives were affected. The full transcript is in this gist. The most useful part is the output of the last command, which is:

gitoxide-lean-v0.37.0-i686-pc-windows-msvc/gix.exe:        PE32+ executable (console) x86-64, for MS Windows, 5 sections
gitoxide-max-pure-v0.37.0-i686-pc-windows-msvc/gix.exe:    PE32+ executable (console) x86-64, for MS Windows, 5 sections
gitoxide-max-v0.37.0-i686-pc-windows-msvc/gix.exe:         PE32+ executable (console) x86-64, for MS Windows, 5 sections
gitoxide-small-v0.37.0-i686-pc-windows-msvc/gix.exe:       PE32+ executable (console) x86-64, for MS Windows, 5 sections
gitoxide-lean-v0.37.0-i686-pc-windows-msvc/ein.exe:        PE32+ executable (console) x86-64, for MS Windows, 5 sections
gitoxide-max-pure-v0.37.0-i686-pc-windows-msvc/ein.exe:    PE32+ executable (console) x86-64, for MS Windows, 5 sections
gitoxide-max-v0.37.0-i686-pc-windows-msvc/ein.exe:         PE32+ executable (console) x86-64, for MS Windows, 5 sections
gitoxide-small-v0.37.0-i686-pc-windows-msvc/ein.exe:       PE32+ executable (console) x86-64, for MS Windows, 5 sections
Byron commented 2 months ago

Thanks a lot for reporting this and for the initial investigation!

The fact that this issue is created years after the first 64 bit binary disguised as 32 bit probably says a lot about the usage (or usefulness) of the Win32 builds. Besides that, they do serve the purpose of showing that gitoxide can build and function on 32 bit systems, even though compared to git there are various accepted limitations - limitations that would also be present in WASM builds while that only exists in 32 bit mode (for all I know).

Thus it's probably a good idea to try and fix the issue, rather than remove 32 bit Windows builds entirely.

I am not sure of the best way is to experiment with this, but it looks feasible. In my fork, I could remove the "Upload release archive" step check the output in some other way. Or if necessary I could leave it in and publish "releases" from my fork while testing.

Maybe you could zip up a tree output of the entire target directory to see which files are present, and where. Alternatively, a reverse-shell might work here as well. In any case, I would appreciate if you could contribute a fix.

Please note that I usually copied my setups from ripgrep, and even though they by now diverged, maybe there is a hint in that repository that shows where to find the files, or how to do cross-platform builds these day. This setup is old and definitely superseded, but it's stringed along while it works, and patched up along the way with minimal effort. So replacing it with tooling that is maintained elsewhere would definitely be a possible action here as well.

EliahKagan commented 2 months ago

ripgrep successfully builds 32-bit Windows binaries, but I wasn't able to identify a particular place where it looked like a bug like this was being fixed. I may return to that later if experimentation doesn't quickly lead to a solution.

I am thinking that actually making test releases is simpler that using a modified workflow that refrains from doing so, but I don't want those to come up as recommendations in GitHub for people, which would be confusing and could inadvertently lead to actual use of them. I could use a tagged version in my fork with something like v0.38.0-alpha-just-for-release-testing-do-not-use-this-ever-under-any-circumstances which would make accidental use unlikely but still not necessarily reduce the confusion to zero.

I'm wondering, though, if maybe the amount of time it uses the runners is low enough to fix within the free minutes for a private repo, so that manual private reupload of the repository could make private releases, allowing me to test this in a near-equivalent setup, except of course that I would not be uploading anything to crates.io.

Byron commented 2 months ago

That's very mindful of you! Personally I'd probably rely on people refraining from downloading binaries from someones fork by default, and deleting that (public) fork after the testing concluded. I would probably only use private minutes for mac testing as well.

EliahKagan commented 2 months ago

I had said:

ripgrep successfully builds 32-bit Windows binaries, but I wasn't able to identify a particular place where it looked like a bug like this was being fixed.

Actually I spoke too soon, sort of. One of the problems affecting gitoxide's release workflow is that environment variables are not being set on Windows because notation like >> $GITHUB_ENV does not work in PowerShell. That was reported for ripgrep in #1820 and the fix was among the changes in https://github.com/BurntSushi/ripgrep/commit/df83b8b44426b3f2179abe632eb183e8c8270524 (https://github.com/BurntSushi/ripgrep/pull/1884).

I've found that fixing just that problem--by using Git Bash--causes cross to run on Windows and fail in its attempt to use docker. The Windows runner has docker. But I don't think existing tooling facilitates using it in this way to set up a Windows container inside Windows:

Showing the command and error that happens for the build release (win-gnu, max) job, which is the one that happened to fail first (canceling the others and thereby conveniently saving some of my free private minutes):

+ C:\Windows\system32\docker.exe run --userns host -e 'PKG_CONFIG_ALLOW_CROSS=1' -e 'XARGO_HOME=/xargo' -e 'CARGO_HOME=/cargo' -e 'CARGO_TARGET_DIR=/target' -e 'CROSS_RUNNER=' -e CARGO_INCREMENTAL -e CARGO_TERM_COLOR -e TERM -e 'USER=runneradmin' --rm --user 1000:1000 -v 'C:\Users\runneradmin\.xargo:/xargo:z' -v 'C:\Users\runneradmin/.cargo:/cargo:z' -v /cargo/bin -v 'D:\a\private-gitoxide\private-gitoxide:/project:z' -v 'C:\Users\runneradmin\.rustup\toolchains\nightly-x86_64-unknown-linux-gnu:/rust:z,ro' -v 'D:\a\private-gitoxide\private-gitoxide\target:/target:z' -w /project ghcr.io/cross-rs/x86_64-pc-windows-gnu:0.2.5 sh -c 'PATH=$PATH:/rust/bin cargo build --verbose --release --target x86_64-pc-windows-gnu --no-default-features --features max'
Unable to find image 'ghcr.io/cross-rs/x86_64-pc-windows-gnu:0.2.5' locally
0.2.5: Pulling from cross-rs/x86_64-pc-windows-gnu
docker: no matching manifest for windows/amd64 10.0.20348 in the manifest list entries.
See 'docker run --help'.

So more than superficial changes may be required to fix this. As in the current ripgrep workflow, the cross command should probably not be attempted in the Windows builds, and instead cross-compilation should be done in another way, possibly just the traditional way of ensuring the target is installed and specifying it.

I may decide to hew to the ripgrep approach. Although GitHub Actions workflows tend to be patched together from prior workflows in other projects and, at least socially speaking, the expectation to worry about licensing for them seems fairly minimal, the situation with ripgrep is even significantly more permissive, since one of the licenses the project is offered under is the Unlicense. So I may "sync" the workflow with that. If I do, I'll note this at least in a commit message.

If doing so doesn't break anything, then I think such an overhaul could have further benefits. One of those benefits, related to the comment discussion in #1239, could be to facilitate more builds, including those requested in #1242.

Personally I'd probably rely on people refraining from downloading binaries from someones fork by default, and deleting that (public) fork after the testing concluded.

I don't know how long it will take to do this, and I may want to keep the testing repository for future related use, as well as to avoid prematurely deleting GitHub Actions logs in case some of the information there proves relevant.

The repo would not technically be a fork in the GitHub sense, since I can only have one (without making an organization or using a second account) and I don't want to delete the fork I've been using to contribute to gitoxide. What I could do is to make releases from that, try to mark them in a way that they are unlikely to be used, and delete them afterwards; even after deleting the releases, the remote reflog (as via the Activity page) and workflow runs logs would still persist as long as otherwise.

I am furthermore not confident that people would not use releases from a fork, even if it were a real GitHub fork. Based on the way GitHub places release announcements in feeds, people who want a prerelease hoping it may fix some issue may not notice, and visiting the repository would show things like your avatar for sponsoring (unless removed) that could create the wrong impression that it is official. Also, a number of forks do release new versions that are meant to be used, either because the fork officially supersedes its parent repo (e.g. vscode-python), or because the fork unofficially publishes a modified version that is useful for a while before the upstream project gains a feature or build target (which is something I have done).

So far I've been able to test in the private repo. If I get close to running out of free private minutes, I can pivot to testing the techniques and workflow in one of my own tiny Rust projects and then applying it to gitoxide. It may not be a bad idea anyway to test out release logic in my own projects that have few or no users before inflicting bestowing it on gitoxide.

Byron commented 2 months ago

Thanks for looking into this more, making releases work properly (again) would be great, and adding more targets would be even better as a side-effect. It's notable that the auto-release mechanism also stopped working - in theory, a new tag should trigger the workflow, but it just doesn't. It's a minor issue, and I keep triggering it by hand which is fine given the frequency of releases. It's just strange that it doesn't work anymore, probably a GitHub bug or me being very blind.

EliahKagan commented 2 months ago

Thanks for looking into this more, making releases work properly (again) would be great

I've opened #1475 for this.

and adding more targets would be even better as a side-effect.

I did not add any more targets, but that's something that should be easier to do now. There are major conditions where it should mostly "just work," as noted in the PR. But I think some possible future targets of interest might still present some difficulties.

It's notable that the auto-release mechanism also stopped working - in theory, a new tag should trigger the workflow, but it just doesn't.

I believe #1475 fixes that as well, though the only change clearly related to it is to the glob pattern that tags have to match to produce a release, and it looks like it's even something you had tried before. But in the private fork I used for testing, pushing was sufficient to trigger the workflow staring in the commit that made that change.