-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
flake: migrate to crane, unpin Nixpkgs, use static builds on Darwin #1380
Open
emilazy
wants to merge
9
commits into
DeterminateSystems:main
Choose a base branch
from
emilazy:push-lvxxwluxukzt
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+75
−166
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This belongs in `packages` instead (where a duplicate is already present anyway).
Crane handles cross‐compilation a bit better and seems to be generally more actively maintained than Naersk. We use Nixpkgs’ stock Rust toolchains instead of bringing in fenix, as Nixpkgs provides everything we need and tracks upstream stable Rust well. This also simplifies the cross‐compilation story. This doesn’t build the documentation like the Naersk build did, which I guess might be useful to ensure that the documentation always builds? That could be added on as a check if desired, using `craneLib.cargoDoc`.
Crane handles setting these for us.
This would be done differently with crane using `craneLib.cargoClippy`. That could be added as a check if desired.
I assume it was not the intent to override the configuration flags and test behaviour of all dependencies.
This is cleaner, handles any future native dependencies correctly, and will work more consistently across platforms (such as Darwin).
Fix a bug where the installer would link against a `libiconv` in the Nix store. This would silently fail to load (falling back to the system `libiconv`, I suppose? I’m not sure) when the Nix store was not present – as is usually the case when running the installer – but after installation, the store path could be populated by normal use of Nix. If that happened, then upon uninstallation, the Nix store `libiconv` would be loaded, the installer would delete the `/nix` partition, and then the next time a system library indirectly caused `dyld` to check up on what’s going on with the loaded dynamic libraries, it would try to read from an address mapped to the library in the now‐deleted Nix store and crash. Now that `pkgsStatic` works after Randy’s Darwin rework to provide mostly‐static builds (system libraries and frameworks are dynamically linked, but libraries from Nixpkgs are linked statically), we can build a truly standalone installer that has no dependency on `/nix/store`, fixing the crash for good.
None of this is required or does anything as of 24.11.
emilazy
force-pushed
the
push-lvxxwluxukzt
branch
from
January 10, 2025 22:48
4a8c851
to
b74638e
Compare
cole-h
approved these changes
Jan 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! Thanks a ton for all your hard work!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This should fix the macOS uninstallation bug, as well as simplifying the flake and making the static builds more robust on all platforms. See the commit messages for details, justifications, and potential caveats.
Note: This work was funded by Determinate Systems.
Checklist
cargo fmt
nix build
nix flake check
(minushydraJobs
, which looked like it’d take all year – caveat emptor)Validating with
install.determinate.systems
If a maintainer has added the
upload to s3
label to this PR, it will become available for installation viainstall.determinate.systems
: