Skip to content
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

nixStatic: Fix darwin. #235990

Merged
merged 4 commits into from
Jun 26, 2023
Merged

nixStatic: Fix darwin. #235990

merged 4 commits into from
Jun 26, 2023

Conversation

uri-canva
Copy link
Contributor

@uri-canva uri-canva commented Jun 5, 2023

Description of changes

Fix nixStatic on darwin. Some fixes in this PR are for pkgsStatic in general but I have not tried any other derivation so they might not be enough.

#214611 (comment)

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@uri-canva
Copy link
Contributor Author

This triggers a lot of new derivations, but it shouldn't affect anyone since they're new builds, not existing builds: pkgsStatic on darwin before this change didn't work at all, so no one would be building or depending on packages from it. That's why I opened it against master instead of staging.

@uri-canva
Copy link
Contributor Author

Oh no wait, it also affects linux packages since I fixed the propagation of NIX_CFLAGS_LINK and nativeBuildInputs in pkgsStatic, which affects linux as well as darwin. Well let me know if I should target staging with this instead, 100 <= x <= 500 packages is probably not too bad.

pkgs/stdenv/adapters.nix Outdated Show resolved Hide resolved
@@ -266,6 +266,11 @@ let
} // lib.optionalAttrs (stdenv.hostPlatform.system == "powerpc64-linux") {
gcc.abi = "elfv2";
};
} // lib.optionalAttrs stdenv.hostPlatform.isDarwin {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense for isStatic to be set on Linux and Darwin but not other operating systems.

Suggested change
} // lib.optionalAttrs stdenv.hostPlatform.isDarwin {
} // lib.optionalAttrs (!stdenv.hostPlatform.isDarwin) {

];
});
nativeBuildInputs = (finalAttrs.nativeBuildInputs or [])
++ lib.optional stdenv.hasCC [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change needs to be explained in the commit message.

@uri-canva
Copy link
Contributor Author

Reverted the incorrect exec format rename, split all the changes in self contained commits with clear commit messages.

@uri-canva
Copy link
Contributor Author

ping

@uri-canva
Copy link
Contributor Author

Bumping.

Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me (except I think we can make one small readability improvement), but I'd really like to hear what @Ericson2314 thinks before it's merged.

crossSystem = {
isStatic = true;
parsed = stdenv.hostPlatform.parsed;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we combine this with the case for Linux below? It's a bit confusing to see the generic crossSystem defined, and then immediately be overridden on Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, although now generic parsed is defined, and then immediately overridden on Linux. Did you have something different in mind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I meant more like this:

parsed = if stdenv.isLinux then makeMuslParsedPlatform stdenv.hostPlatform.parsed else stdenv.hostPlatform.parsed;

or

parsed = (if stdenv.isLinux then makeMuslParsedPlatform else lib.id) stdenv.hostPlatform.parsed;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, you're right much better now, thank you!

@uri-canva
Copy link
Contributor Author

ping @Ericson2314

1 similar comment
@uri-canva
Copy link
Contributor Author

ping @Ericson2314

@siraben
Copy link
Member

siraben commented Jun 26, 2023

Result of nixpkgs-review pr 235990 run on aarch64-darwin 1

6 packages built:
  • nixStatic
  • nixStatic.dev
  • nixVersions.nix_2_13
  • nixVersions.nix_2_13.dev
  • nixVersions.nix_2_13.doc
  • nixVersions.nix_2_13.man

@uri-canva uri-canva merged commit 6e51b1a into NixOS:master Jun 26, 2023
21 checks passed
@uri-canva uri-canva deleted the fix-static branch June 29, 2023 01:48
@anka-213
Copy link
Contributor

How do you use this? When I try this on macos

nix build nixpkgs#legacyPackages.x86_64-darwin.pkgsStatic.hello

I get this error message

error: don't yet have a `targetPackages.darwin.LibsystemCross for x86_64-apple-darwin`

@uri-canva
Copy link
Contributor Author

I'm not sure about how the flake output gets resolved, but if you use the non-flake command it's:

$ nix-build -A nixStatic
these 5 paths will be fetched (11.10 MiB download, 38.73 MiB unpacked):
...
/nix/store/rqbcx832rrcr128zjhx0jdgbgcsf22nx-nix-static-aarch64-apple-darwin-2.15.1
$ nix-build -A pkgsStatic.hello
this derivation will be built:
  /nix/store/y2pqixdlp2wpaxyl4ydl9axzz70dkv4l-hello-static-aarch64-apple-darwin-2.12.1.drv
these 24 paths will be fetched (74.34 MiB download, 470.33 MiB unpacked):
...
/nix/store/hmwmirw4f49y6mzwdhz8n13lsayj40c5-hello-static-aarch64-apple-darwin-2.12.1

I'm on an aarch64-darwin machine, on 100a155.

@anka-213
Copy link
Contributor

I'm on an aarch64-darwin machine [...]

That does indeed seem to be the issue!

nix build nixpkgs#legacyPackages.aarch64-darwin.pkgsStatic.hello

works just fine, while

nix build nixpkgs#legacyPackages.x86_64-darwin.pkgsStatic.hello

gives the error I wrote above. So for some reason it only works on arm macs and not on intel macs.

@Artturin
Copy link
Member

How do you use this? When I try this on macos

nix build nixpkgs#legacyPackages.x86_64-darwin.pkgsStatic.hello

I get this error message

error: don't yet have a `targetPackages.darwin.LibsystemCross for x86_64-apple-darwin`

#180771

@anka-213
Copy link
Contributor

Ah, I see! Btw, does pkgsStatic need to be using crossSystem for darwin? It isn't really cross-compilation in any real sense, since it's not switching the stdlib or anything.

@Thesola10
Copy link
Contributor

Thesola10 commented Sep 14, 2023

@anka-213 Internally, on any system, Nixpkgs considers static builds to be cross-compiling, since it invokes static libraries. If it weren't "cross-compiled", Nix would have to build a static compiler and static host libraries for configure-time and build-time tools as well, wasting precious space and time.

On Darwin specifically, there is no stable syscall interface, meaning libSystem.dylib has to remain dynamically loaded, which requires a reference "cross libSystem"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants