GitoxideLabs / gitoxide

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

Fix 32-bit Windows releases so they are the correct architecture #1475

Closed EliahKagan closed 2 months ago

EliahKagan commented 2 months ago

This fixes #1472 so that the 32-bit Windows releases contain 32-bit binaries rather than 64-bit binaries, makes it so pushing a version tag automatically triggers the release workflow, and somewhat streamlines and clarifies the workflow.

Further improvement is possible, including by incorporating some of the other recent changes from the ripgrep release workflow on which it was based. This is something I would be interested to do separately but that I think is not naturally within the scope of these changes.

At least in principle, the workflow can also at this point be extended easily for more build targets, so long as they are otherwise able to build and so long as either (a) they can be built with cross-compilation using cross on a Linux-based host or (b) they do not require cross. However, if adding more Linux-based targets, it would be a good idea to generalize the strip logic, or the resulting binaries would have debug symbols even though they would be optimized builds.

There are some more details in the commit messages. This includes some approaches I tried but did not keep, when they were entangled with the other changes. This information about the journey may be useful because the actual workflow runs I did for testing are not currently public. As discussed in #1472, I used a private reupload to avoid notifying people in their feeds of something the might misunderstand as an actual prerelease of gitoxide. However, I'd be pleased to republish from my public fork with distinctive tag names (e.g., with do-not-use in them), provide you access to the private repository, make the private repository at least temporarily public, or any combination of those things.

For verification, and to demonstrate that the bug does not occur when the release made by the revised workflow is used, I ran these commands on Windows, which correspond to those in "Demonstration checking binaries in all archives of a release" in #1472, except that they will not currently work for anyone but me because they refer to a private repository to which no one else currently has access:

mkdir tmp
cd tmp
gh release -R EliahKagan/private-gitoxide download v0.38.0-alpha.5
gci | %{ 7z x $_ }
gci *.tar | %{ 7z x $_ }
file */gix* */ein*
file */gix* */ein* | sls -raw i686

The last four commands are the same. The full output is in this second file in the related gist. The output just of the last command is:

gitoxide-lean-v0.38.0-alpha.5-i686-pc-windows-msvc/gix.exe:        PE32 executable (console) Intel 80386, for MS Windows, 4 sections
gitoxide-max-pure-v0.38.0-alpha.5-i686-pc-windows-msvc/gix.exe:    PE32 executable (console) Intel 80386, for MS Windows, 4 sections
gitoxide-max-v0.38.0-alpha.5-i686-pc-windows-msvc/gix.exe:         PE32 executable (console) Intel 80386, for MS Windows, 4 sections
gitoxide-small-v0.38.0-alpha.5-i686-pc-windows-msvc/gix.exe:       PE32 executable (console) Intel 80386, for MS Windows, 4 sections
gitoxide-lean-v0.38.0-alpha.5-i686-pc-windows-msvc/ein.exe:        PE32 executable (console) Intel 80386, for MS Windows, 4 sections
gitoxide-max-pure-v0.38.0-alpha.5-i686-pc-windows-msvc/ein.exe:    PE32 executable (console) Intel 80386, for MS Windows, 4 sections
gitoxide-max-v0.38.0-alpha.5-i686-pc-windows-msvc/ein.exe:         PE32 executable (console) Intel 80386, for MS Windows, 4 sections
gitoxide-small-v0.38.0-alpha.5-i686-pc-windows-msvc/ein.exe:       PE32 executable (console) Intel 80386, for MS Windows, 4 sections

Note that these are now 32-bit executables. The other Windows builds remain 64-bit.

Edit: I have also manually tested the binaries from the gitoxide-max-v0.38.0-alpha.5-i686-pc-windows-msvc.zip test release to ensure that they run on a 32-bit Windows system and that I can use the gix executable to clone the gitoxide repository via SSH.

Skgland commented 2 months ago

By now, it's possible to specify strip = true in the Cargo.toml manifest directly, so cargo will perform the stripping of release binaries. Thus far I refrained from doing that because when profiling release builds, symbols are a necessity. However, maybe it's possible to define a specific profile configuration that performs no stripping but otherwise is equal to the release build which does perform stripping. If that's possible, I think one could just use cargo to strip symbols which should be cross-platform. But when looking at the workflow, I could imagine that within cross it wouldn't work after all as it would have to be able to find strangely named platform-dependent strip-binaries.

That sounds like you are looking for a custom profile inheriting from release and overriding strip https://doc.rust-lang.org/cargo/reference/profiles.html#custom-profiles

Byron commented 2 months ago

Wow, thanks, that's pretty much spot-on! I will leave it to @EliahKagan to implement these (e.g. add a profile profile), as I am afraid that the strip option doesn't work if it has to invoke external programs. But maybe the compiler simply emits no names for functions then instead of stripping them out in an extra step. But definitely something to validate I suppose, despite learning towards trusting the rustc implementation always works.

Skgland commented 2 months ago

The strip setting is documented at https://doc.rust-lang.org/cargo/reference/profiles.html#strip and corresponds to the -C strip=val compiler flag documented at https://doc.rust-lang.org/rustc/codegen-options/index.html#strip. The way that is worded sounds to me like rustc instructs the linker to strip the info while linking rather than invoking a separate strip tool. Though based on the tracking issue for -c strip=val mac OS is special as the linker doesn't support a flag to instruct it to stripping while linking.

Byron commented 2 months ago

Thanks for sharing your research - it truly looks like this would work on all platforms. MacOS being special shouldn't be a problem as no crosscompilation is going on there. Also, I'd think they will be able to handle differences between MacOS versions better than us in a bash script.

Great, maybe @EliahKagan can use the momentum and adjust the publishing so that stripping is left to cargo itself. I'd think that I can use a release-profile named profiling to get an unstripped release binary for when a profiling run is desired.