treewide: replace darwin.apple_sdk_11_0.stdenv with stdenv on darwin #370661
Conversation
4a01d44 to
c73a057
Compare
pkgs/top-level/all-packages.nix
Outdated
| stdenv = if stdenv.hostPlatform.isDarwin && stdenv.hostPlatform.isx86_64 | ||
| then overrideSDK darwin.apple_sdk_11_0.stdenv { darwinMinVersion = "10.13"; } | ||
| then overrideSDK stdenv { darwinMinVersion = "10.13"; } | ||
| else stdenv; |
There was a problem hiding this comment.
I guess dropping overrideSDK here will result in 500+ rebuilds...
There was a problem hiding this comment.
Maybe best to do it in a separate PR and handle the darwin.apple_sdk_11_0.callPackage for it there too.
There was a problem hiding this comment.
Moved to #370670 and a subsequent PR.
pkgs/top-level/all-packages.nix
Outdated
| stdenv = if stdenv.hostPlatform.isDarwin then stdenv | ||
| else if stdenv.cc.isClang then llvmPackages.stdenv | ||
| else stdenv; |
There was a problem hiding this comment.
if stdenv.cc.isClang && !stdenv.hostPlatform.isDarwin? (But also: What on earth is this doing? Probably it can just be dropped?)
There was a problem hiding this comment.
Tracked it down to f863703. This is doing nothing useful and can just be dropped.
There was a problem hiding this comment.
I'll do it in #370672. Currently, all Bazel versions fail to build on darwin, I guess some Clang overrides will be needed: https://hydra.nixos.org/eval/1810825?filter=bazel&compare=1810818&full=#tabs-still-fail
93abeb4 to
117dd80
Compare
| @@ -27,7 +26,6 @@ makeScopeWithSplicing' { | |||
| inherit (self) callPackage; | |||
| noExtraAttrs = set: lib.attrsets.removeAttrs set [ "extend" "override" "overrideScope" "overrideDerivation" ]; | |||
| in (noExtraAttrs qt6) // { | |||
| inherit stdenv; | |||
There was a problem hiding this comment.
Maybe we should add a warning here for a release cycle first. LGTM otherwise.
There was a problem hiding this comment.
I've reverted this change, since I'm not sure how to properly add a warning here...
There was a problem hiding this comment.
I think just stdenv = lib.warn "qt6Packages.stdenv: this is going away!!" stdenv; would do it, as long as there’s nothing else in the tree using it.
There was a problem hiding this comment.
Indeed, thanks. I wonder what I was worried about...
e356f1e to
81f8fea
Compare
pkgs/top-level/qt6-packages.nix
Outdated
| @@ -27,7 +27,10 @@ makeScopeWithSplicing' { | |||
| inherit (self) callPackage; | |||
| noExtraAttrs = set: lib.attrsets.removeAttrs set [ "extend" "override" "overrideScope" "overrideDerivation" ]; | |||
| in (noExtraAttrs qt6) // { | |||
| inherit stdenv; | |||
| stdenv = | |||
| lib.warnIf (lib.oldestSupportedReleaseIsAtLeast 2411) | |||
There was a problem hiding this comment.
24.11 still supports 10.12, so this advice is only wholly applicable for 25.05 and higher; I think you want 2505 here.
(OTOH, most Qt‐using applications don’t need the macOS 11 SDK anyway, and those that do should just use apple-sdk_11 directly, even on 24.11. But it’s only a month’s difference.)
There was a problem hiding this comment.
I think 2411 actually works, because for 24.11 branches lib.trivial.oldestSupportedRelease should be 2405...
There was a problem hiding this comment.
I guess I don’t understand oldestSupportedReleaseIsAtLeast. Isn’t this just true unconditionally, even right now? I thought the idea was that you’d write something that becomes true after release so that unstable doesn’t throw up warnings that can’t be fixed on supported stable versions (so I assumed 2505 was the right thing).
There was a problem hiding this comment.
That's true, I'll just put a lib.warn.
81f8fea to
2b96961
Compare
|
Seems like there’s a few more of these references remaining in |
| @@ -62,8 +62,7 @@ buildPythonPackage rec { | |||
| hash = "sha256-slBJleeDi0mCVThty4NUX4M9vaCLV+E8rnp1Ab77TmE="; | |||
| }; | |||
|
|
|||
| stdenv = | |||
| if python.stdenv.hostPlatform.isDarwin then darwin.apple_sdk_11_0.stdenv else python.stdenv; | |||
| stdenv = python.stdenv; | |||
There was a problem hiding this comment.
BTW, I think these are no‐ops (probably should have caught them during review).
|
We need to revert the Qt thing because of the scope (e.g. @infinisil It looks like GHA eval breaks compatibility with the critical ofborg feature of failing the eval if anything in Nixpkgs triggers warnings. Unless I’m misunderstanding something we really need to get that fixed now that ofborg is gone. |
|
Would something like this help? Though it looks terrible... {
stdenv = lib.warn "qt6Packages.stdenv is deprecated. Use stdenv instead." stdenv;
}
// makeScopeWithSplicing' {
...
} |
|
Yeah, that might be a good idea; I’ll test it. Or we could just remove it entirely under the assumption that most people are getting it from the scope. (These warnings are actually channel blockers, so this is a pretty huge CI footgun… Sorry for not realizing it when I suggested it.) |
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.