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

treewide: unpin apple-sdk_11 #370445

Merged
merged 1 commit into from
Jan 3, 2025
Merged

treewide: unpin apple-sdk_11 #370445

merged 1 commit into from
Jan 3, 2025

Conversation

khaneliman
Copy link
Contributor

@khaneliman khaneliman commented Jan 3, 2025

Things done

Min SDK version was bumped in nixpkgs, unpinning derivations.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

Add a 👍 reaction to pull requests you find important.

@emilazy
Copy link
Member

emilazy commented Jan 3, 2025

Can we just do a treewide to remove all apple-sdk_11s and darwinMinVersionHooks ≤ 11.3? (No worries if you don’t feel like it, but it was on my to‐do list, so if you’re already doing this then maybe we can just knock it all out in one go. It’s not as big a job as it sounds like since a lot of the long tail of packages are still on the old pattern; those I can check my old branch for later.)

@khaneliman
Copy link
Contributor Author

Can we just do a treewide to remove all apple-sdk_11s and darwinMinVersionHooks ≤ 11.3? (No worries if you don’t feel like it, but it was on my to‐do list, so if you’re already doing this then maybe we can just knock it all out in one go. It’s not as big a job as it sounds like since a lot of the long tail of packages are still on the old pattern; those I can check my old branch for later.)

I can try doing that, if you'd like.

@github-actions github-actions bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 3, 2025
@emilazy
Copy link
Member

emilazy commented Jan 3, 2025

If you feel up to it, it would be great :)

@github-actions github-actions bot added 6.topic: python 6.topic: golang 6.topic: java Including JDK, tooling, other languages, other VMs 6.topic: games labels Jan 3, 2025
@khaneliman khaneliman changed the title various: unpin apple-sdk treewide: unpin apple-sdk_11 Jan 3, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 5001+ 10.rebuild-linux: 1-10 and removed 10.rebuild-darwin: 1-10 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 3, 2025
@paparodeo
Copy link
Contributor

wait -- can this be merged tho -- I looked over the 100 files.

@khaneliman
Copy link
Contributor Author

@emilazy Guessing this will have to go to staging.

wait -- can this be merged tho -- I looked over the 100 files.

I was wondering if it had to go to staging with how many are affected now.

@khaneliman khaneliman force-pushed the sdk branch 4 times, most recently from 1d484e6 to 0a42baa Compare January 3, 2025 02:36
@khaneliman khaneliman changed the base branch from master to staging January 3, 2025 02:37
@nix-owners
Copy link

nix-owners bot commented Jan 3, 2025

The PR's base branch is set to staging, but 20 commits from the master branch are included. Make sure you know the right base branch for your changes, then:

  • If the changes should go to the master branch, change the base branch to master
  • If the changes should go to the staging branch, rebase your PR onto the merge base with the staging branch:
    # git rebase --onto $(git merge-base upstream/staging HEAD) $(git merge-base upstream/master HEAD)
    git rebase --onto 89019580b73ddd7d4b05fa5f6952a4676538c031 771c7e608ec7551137b2d239355ccf5856c4f0e7
    git push --force-with-lease

@emilazy
Copy link
Member

emilazy commented Jan 3, 2025

I guess there’s no PR for the staging-only ones yet? (I had assumed it was just split entirely into master‐targeting PRs, which seemed wrong.)

@khaneliman
Copy link
Contributor Author

khaneliman commented Jan 3, 2025

I guess there’s no PR for the staging-only ones yet? (I had assumed it was just split entirely into master‐targeting PRs, which seemed wrong.)

Sorry, just waiting for the other merges before ripping them out of here. I suppose I don't have to. But, I was just double checking rebuild count on the packages left. Although, according to @paparodeo I might be able to include some more straight to master.

@emilazy
Copy link
Member

emilazy commented Jan 3, 2025

FWIW the Facebook libraries (folly, edencommon, etc.) are only huge rebuilds on Linux, which this PR shouldn’t rebuild. So I think a lot of this can still go straight to master – presumably everything except the changes in #370445 (comment).

@khaneliman
Copy link
Contributor Author

khaneliman commented Jan 3, 2025

FWIW the Facebook libraries (folly, edencommon, etc.) are only huge rebuilds on Linux, which this PR shouldn’t rebuild. So I think a lot of this can still go straight to master – presumably everything except the changes in #370445 (comment).

Yeah, I didn't think they should rebuild either... just saw a lot of stuff getting flagged in my nixpkgs-review locally and was being cautious. Running it again looks like it was listing them as just affected but not actual needing rebuild. So I think we are good.

@emilazy
Copy link
Member

emilazy commented Jan 3, 2025

Probably Mic92/nixpkgs-review#446.

@github-actions github-actions bot removed the 6.topic: java Including JDK, tooling, other languages, other VMs label Jan 3, 2025
@khaneliman khaneliman marked this pull request as ready for review January 3, 2025 16:54
Comment on lines 65 to 67
depsTargetTargetPropagated = lib.optionals stdenv.targetPlatform.isDarwin [
apple-sdk_11
xcbuild
];
Copy link
Member

Choose a reason for hiding this comment

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

BTW, I’m not sure we need xcbuild here any more. xcrun is in the stdenv already and I’d be surprised if many Go packages need xcodebuild itself. cc @zowoq

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it was xcrun: #107937 (comment).

#370779

@nix-owners nix-owners bot requested a review from qbit January 3, 2025 17:06
@emilazy emilazy merged commit 5982152 into NixOS:staging Jan 3, 2025
26 of 27 checks passed
@khaneliman khaneliman deleted the sdk branch January 3, 2025 20:51
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.

5 participants