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

x86_64-darwin: fix packages that won’t build #180931

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

reckenrode
Copy link
Contributor

@reckenrode reckenrode commented Jul 10, 2022

Description of changes

#176661 allows x86_64-darwin to use the 11.0 SDK. This PR fixes most packages that were flagged as broken on x86_64darwin. It also reworks the MoltenVK derivation to build without needing Xcode (and bumps it because that patch has been sitting in my queue along with the Xcode build change). There are two exceptions:

I also updated the SDK-specific callPackage to make it more robust. It also now provides a couple of GCC stdenvs and rustPlatform.

Sandboxing is set to relaxed. I had to add a sandboxProfile to ccl to let it build with sandboxing.

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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jul 10, 2022
@reckenrode reckenrode marked this pull request as draft July 10, 2022 05:06
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: ruby 6.topic: stdenv Standard environment 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 10, 2022
@reckenrode reckenrode changed the base branch from master to staging July 10, 2022 05:14
@reckenrode reckenrode marked this pull request as ready for review July 10, 2022 05:15
@github-actions github-actions bot removed 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: stdenv Standard environment 6.topic: ruby labels Jul 10, 2022
@reckenrode
Copy link
Contributor Author

I rebased on staging. That’s way more rebuilds than I expected.

@reckenrode reckenrode marked this pull request as draft July 10, 2022 05:24
@reckenrode
Copy link
Contributor Author

I converted the PR to a draft because it looks like nixpkgs-review is identifying more things that are broken, which should probably be addressed in a “fix the broken things on Darwin” PR.

@reckenrode reckenrode force-pushed the darwin-sdk-fixes branch 2 times, most recently from ea2917e to d5b70be Compare July 10, 2022 14:03
@reckenrode
Copy link
Contributor Author

Based on feedback from @SuperSandro2000, I’ve split the MoltenVK changes off into a separate PR (#180988) to keep this one focused on fixing packages that were marked broken on Darwin.

Copy link
Contributor

@toonn toonn left a comment

Choose a reason for hiding this comment

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

Pretty much all of this LGTM.

@@ -31,14 +41,5 @@ rustPlatform.buildRustPackage rec {
homepage = "https://rustpython.github.io";
license = licenses.mit;
maintainers = with maintainers; [ prusnak ];

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can leave a TODO here to switch back away from the SDK when stdenv gets bumped to 10.13, if that is enough?

@@ -28,10 +28,6 @@ rustPlatform.buildRustPackage rec {
homepage = "https://bupstash.io";
license = licenses.mit;
platforms = platforms.unix;
# = note: Undefined symbols for architecture x86_64:
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like another one that might work with the stdenv bump to 10.13. Another TODO?

@@ -21,17 +21,5 @@ stdenv.mkDerivation rec {
license = licenses.gpl3Plus;
maintainers = with maintainers; [ qyliss ];
platforms = platforms.unix;

Copy link
Contributor

Choose a reason for hiding this comment

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

utimensat will be in the stdenv bump to 10.13, TODO?

@@ -46,7 +46,7 @@ let
inherit MacOSX-SDK;

Libsystem = callPackage ./libSystem.nix {};
LibsystemCross = pkgs.darwin.Libsystem;
LibsystemCross = packages.Libsystem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure this is fine? I feel like it'd have been defined as an alias before if that was the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow. Aren’t aliases for deprecated names?

This change was to correct what I believe was a mistake in the previous patch. The 11.0 SDK originally assumed that pkgs.darwin.Libsystem belongs to the SDK, but that’s not necessarily the case. On x86_64-darwin, that’s the 10.12 SDK.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant with an alias was rec { Libsystem = ...; LibsystemCross = Libsystem; }, not aliases.nix.

I'm really not sure how cross works in Nix. But I do suspect LibsystemCross might have to be different from Libsystem in some way, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. It’s not currently different on aarch64-darwin, so I propagated that here. I assume it works because Apple’s SDK is naturally cross-capable.

};
in
if !needsOverrides then stdenv
else overridePlatformDarwinVersions (pkgs.overrideCC stdenv clang) "10.12" "11.0";
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having some trouble keeping track of all the overrides.
Is this stdenv the standard stdenv, based on the source releases for 10.12, with an overridden compiler that is based on SDK Libsystem? Is that the only reference to system libraries from stdenv? Or is this completely taken care of by the inherit in callPackage?

Copy link
Contributor Author

@reckenrode reckenrode Jul 12, 2022

Choose a reason for hiding this comment

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

It’s the 10.12 stdenv with the compiler and SDK Libsystem replaced. This is the same implementation as in current master just moved to a new file and with build/hostPlatform overridden. I couldn’t see a way to build a pure 11.0 SDK without doing surgery on the Darwin stdenv definition.

I originally wrote the overridePlatformDarwinVersions function to use with overriding the GCC stdenv, but it turns out that builds fine from the private nixpkgs. If it would improve clarity, I can inline the function definition, so it’s more like how things are currently on master.

Copy link
Contributor

Choose a reason for hiding this comment

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

The function is perfectly fine.

What I'm concerned about here is that apple_sdk_11_0.stdenv is still using the 10.12 SDK somehow. Having a bootstrap stage is totally reasonable if that's what it takes. If it's no problem whatsoever then so much the better : )

@bobby285271 bobby285271 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jul 11, 2022
@reckenrode
Copy link
Contributor Author

I’ve got an updated reflecting the changes requested here plus also discussed on Matrix. I’m waiting for it to build successfully before pushing. It now uses a clang built in the 11.0 SDK stdenv instead of one from the 10.12 SDK with overrides.

@reckenrode
Copy link
Contributor Author

I also added cmake to the callPackage since that was mentioned on Matrix as having an issue with pulling in 10.12 SDK stuff even with an 11.0 SDK environment. I expect other compilers and build tools may need this as well.

@reckenrode reckenrode marked this pull request as draft July 13, 2022 01:31
@reckenrode
Copy link
Contributor Author

Converting this to draft while I work through the stdenv stuff.

@siraben
Copy link
Member

siraben commented Jul 13, 2022

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

4 packages marked as broken and skipped:
  • llvmPackages_10.clangNoLibc
  • llvmPackages_7.clangNoLibc
  • llvmPackages_8.clangNoLibc
  • llvmPackages_9.clangNoLibc
1 package failed to build:
  • vc4-newlib
34 packages built:
  • aspino
  • bintoolsNoLibc
  • binutilsNoLibc
  • bsnes-hd
  • btop
  • bullet-roboschool
  • crossLibcStdenv
  • llvmPackages.bintoolsNoLibc
  • llvmPackages.clangNoCompilerRt
  • llvmPackages.clangNoLibc
  • llvmPackages.compiler-rt-no-libc
  • llvmPackages_10.bintoolsNoLibc
  • llvmPackages_10.clangNoCompilerRt
  • llvmPackages_12.bintoolsNoLibc
  • llvmPackages_12.clangNoCompilerRt
  • llvmPackages_12.clangNoLibc
  • llvmPackages_13.bintoolsNoLibc
  • llvmPackages_13.clangNoCompilerRt
  • llvmPackages_13.clangNoLibc
  • llvmPackages_14.bintoolsNoLibc
  • llvmPackages_14.clangNoCompilerRt
  • llvmPackages_14.clangNoLibc
  • llvmPackages_7.bintoolsNoLibc
  • llvmPackages_7.clangNoCompilerRt
  • llvmPackages_8.bintoolsNoLibc
  • llvmPackages_8.clangNoCompilerRt
  • llvmPackages_9.bintoolsNoLibc
  • llvmPackages_9.clangNoCompilerRt
  • newlib-nanoCross
  • newlibCross
  • or1k-newlib
  • preLibcCrossHeaders
  • rustpython
  • vengi-tools

@chasecaleb
Copy link
Contributor

chasecaleb commented Jul 13, 2022

@reckenrode Could you look at Trivy as part of this PR too, please? I suspect that should work now too:

# Need updated macOS SDK
# https://github.com/NixOS/nixpkgs/issues/101229
broken = (stdenv.isDarwin && stdenv.isx86_64);

Edit: Sorry, I made this comment too quickly and just realized you called out Go packages as being worked on in #179622 . So maybe Trivy isn't relevant here in your PR? But I don't see Trivy being unmarked as broken in that PR either so I don't know where it belongs.

@reckenrode reckenrode mentioned this pull request Jul 14, 2022
@reckenrode
Copy link
Contributor Author

Sorry, I made this comment too quickly and just realized you called out Go packages as being worked on in #179622 . So maybe Trivy isn't relevant here in your PR? But I don't see Trivy being unmarked as broken in that PR either so I don't know where it belongs.

Based on what @SuperSandro2000 said in #180704, I believe the plan is to address those packages in a follow up PR to keep the Go 1.18 update PR from growing out of hand.

@prusnak
Copy link
Member

prusnak commented Dec 27, 2022

This should be broken to individual pull requests, since some of the packages have already been fixed elsewhere.

@wegank wegank mentioned this pull request Jan 28, 2023
13 tasks
@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: darwin Running or building packages on Darwin 6.topic: python 6.topic: rust 8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants