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

go_1_18: Use apple_sdk_11_0.callPackage #180704

Closed
wants to merge 37 commits into from
Closed

Conversation

toonn
Copy link
Contributor

@toonn toonn commented Jul 8, 2022

Go 1.18 requires a newer SDK than the one we build from sources. As a
workaround we're making use of the SDK we're using for aarch64-darwin.
This means Go 1.18 will not work on any Darwin systems that don't have
forwards-compatible SDK versions with the particular package in
question. We might need to mark Go packages broken based on the macOS
version rather than just the platform and architecture.

Until we find a better solution, Go packages will need to make sure to
get all their (Darwin) system dependencies from the apple_sdk_11_0,
this includes dependencies of build tools like xcbuild.

For convenience darwin.apple_sdk_11_0 has a callPackage attribute
which provides the correct stdenv and xcbuild attributes as
arguments. This function can be expanded to substitute other necessary
arguments when they come up.

Description of changes

Use darwin.apple_sdk_11_0.callPackage for go_1_18, buildGo118Module and buildGo118Package to insert a stdenv and xcbuild based on apple_sdk_11_0 on Darwin.

I've merged #179622 into this so I can run a Hydra evaluation for x86_64-darwin. I've kept this as a draft so I can either rebase after #179622 is merged or change the target branch to staging because of all the rebuilds.

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:
  • Same change but not this exact patch, ran by @reckenrode 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.

SuperSandro2000 and others added 7 commits July 5, 2022 17:44
Go 1.18 requires a newer SDK than the one we build from sources. As a
workaround we're making use of the SDK we're using for aarch64-darwin.
This means Go 1.18 will not work on any Darwin systems that don't have
forwards-compatible SDK versions with the particular package in
question. We might need to mark Go packages broken based on the macOS
version rather than just the platform and architecture.

Until we find a better solution, Go packages will need to make sure to
get all their (Darwin) system dependencies from the `apple_sdk_11_0`,
this includes dependencies of build tools like `xcbuild`.

For convenience `darwin.apple_sdk_11_0` has a `callPackage` attribute
which provides the correct `stdenv` and `xcbuild` attributes as
arguments. This function can be expanded to substitute other necessary
arguments when they come up.
@toonn toonn requested a review from reckenrode July 8, 2022 11:58
Comment on lines +13363 to 13365
go_1_18 = darwin.apple_sdk_11_0.callPackage ../development/compilers/go/1.18.nix {
inherit (darwin.apple_sdk_11_0.frameworks) Security Foundation;
};
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't mind a comment here

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

I think the easiest is that I cherry-pick the darwin fix commit into my branch when it is done because I need to rebase that on master again to remove new occurrences of buildGo118Module

@@ -21753,20 +21700,20 @@ with pkgs;
buildGo117Package = callPackage ../development/go-packages/generic {
go = buildPackages.go_1_17;
};
buildGo118Package = callPackage ../development/go-packages/generic {
buildGo118Package = darwin.apple_sdk_11_0.callPackage ../development/go-packages/generic {
Copy link
Member

Choose a reason for hiding this comment

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

or here


buildGo117Module = callPackage ../development/go-modules/generic {
go = buildPackages.go_1_17;
};
buildGo118Module = callPackage ../development/go-modules/generic {
buildGo118Module = darwin.apple_sdk_11_0.callPackage ../development/go-modules/generic {
Copy link
Member

Choose a reason for hiding this comment

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

or here

@reckenrode
Copy link
Contributor

The callPackages changes LGTM, though I agree regarding adding a comment to explain why the Darwin callPackage is being used. Should the Go-using packages that are marked broken on x86_64-darwin also have that removed in this PR?

We might need to mark Go packages broken based on the macOS version rather than just the platform and architecture.

Is this possible? I can see needing something like that in the future for DXVK when MoltenVK makes greater use of Metal 3 (and presumably can provide the missing extensions on macOS 13 or newer).

@SuperSandro2000
Copy link
Member

Should the Go-using packages that are marked broken on x86_64-darwin also have that removed in this PR?

Please lets do this in another PR. I don't want the go 1.18 PR to completely grow out of hand. There might also be failing tests or go.sum/vendoring files which need updating.

@SuperSandro2000
Copy link
Member

Cherry-picked into #179622

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

Successfully merging this pull request may close these issues.

3 participants