Byron / gitoxide

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

Split `release` profile into `release` and `release-opt` #1495

Closed EliahKagan closed 1 month ago

EliahKagan commented 1 month ago

This makes the kind of changes discussed in comments in #1475 and further in #1477. This fixes #1477, though some other fix might be preferred, so this may benefit from being changed, or further changes could be made afterward. See https://github.com/Byron/gitoxide/issues/1477#issuecomment-2257499363 and https://github.com/Byron/gitoxide/issues/1477#issuecomment-2274445001.

In Cargo.toml, this adds thin LTO to the release profile, and creates a separate more aggressively optimized release-opt profile with fat LTO and max 1 codegen unit. The effect on build times seems in both cases modest, but you might disagree. For more on that, see this gist where I timed the builds and https://github.com/Byron/gitoxide/issues/1477#issuecomment-2274445001 (which, among other related topics, discusses it). This also removes some old commented-out code, as also discussed in #1477.

In release.yml, this takes the profile to use from an environment variable (some occurrences of release had referred to that profile while others had not), does some related other refactoring, and most significantly changes which profile is used for the GitHub releases from release to the new release-opt profile.

The results seem okay, and in particular the Linux executables, which without configuring stripping in Cargo.toml might require more explicit logic as new targets are added, show as stripped. (The file implementation I am using does not seem to report the absence of symbols in macOS binaries; that affected prior runs' results as well.) That output is on assets downloaded from a draft release produced in this workflow run.

In case you decide to merge this without many changes but hope for an even better approach later, I've included a comment in Cargo.toml stating that the release-opt profile is not guaranteed to stick around.

EliahKagan commented 1 month ago

Please do double-check my changes in case they have a chance to break something, after all I didn't really test any of this.

I haven't rerun the workflow yet, but on inspection it doesn't seem like anything would be broken.

I have actually tested this now, and it seems fine.