-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
stdenv: make darwin builds reproducable #77632
Conversation
75c2904
to
1eed714
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_DEPLOYMENT_TARGET.html
the CMAKE_OSX_DEPLOYMENT_TARGET
will be initialized from MACOSX_DEPLOYMENT_TARGET
, so it should be safe to remove those flags from the affected packages.
let version = "10.12"; in | ||
|
||
# Ensure appleSdkVersion is up to date. | ||
assert stdenv.isDarwin -> stdenv.appleSdkVersion == version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we just use stdenv.appleSdkVersion
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The attribute is macOS specific, without the condition evaluating darwin only packages on linux fails on the assertion instead of meta.platforms with a nice message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I was trying to ask is: if version
must be equal to stdenv.appleSdkVersion
, then why is one of them not initialized from the other, as in version = stdenv.appleSdkVersion or "invalid platform";
?
Indeed, but it's explicitly disabled for some reason. @matthewbauer Why did you disabled the deployment target for cmake 8beb809? We do want binaries to have a minimum version. |
libuv fails on 1 test (was on 1eed714):
|
# we never want to use the global macOS SDK | ||
cmakeFlags="-DCMAKE_OSX_SYSROOT= $cmakeFlags" | ||
|
||
# disable OSX deployment target | ||
# we don't want our binaries to have a "minimum" OSX version | ||
cmakeFlags="-DCMAKE_OSX_DEPLOYMENT_TARGET= $cmakeFlags" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this isn't needed? How does CMake choose this value when it's unset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not set explicitly the value is initialized by the MACOSX_DEPLOYMENT_TARGET environment variable, if set, and otherwise computed based on the host platform.
So if I understand this correctly this was equivalent to unsetting MACOSX_DEPLOYMENT_TARGET from the stdenv.
|
||
# Ensure consistent LC_VERSION_MIN_MACOSX and remove LC_UUID. | ||
export MACOSX_DEPLOYMENT_TARGET=${macosVersionMin} | ||
export NIX_LDFLAGS+=" -macosx_version_min ${macosVersionMin} -sdk_version ${appleSdkVersion} -no_uuid" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how the linker works when you pass multiple values here. For instance, is -macosx_version_min 10.12 -macosx_version_min 10.14
a valid argument list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MACOSX_DEPLOYMENT_TARGET
and -macos_version_min
are closely related but not the same. AFAIK the variable is used by build systems and headers for version specific pragmas while the linker flag just configures the load command.
Alltho since it looks like the compiler always adds the flag based on the variable, overriding this original one, it's not really necessary. Added it just in case but should be fine to leave out if you're concerned about it.
# Workaround for https://openradar.appspot.com/22671534 on 10.11. | ||
export gl_cv_func_getcwd_abort_bug=no | ||
|
||
stripAllFlags=" " # the Darwin "strip" command doesn't know "-s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this a space is kind of hacky, but I think it is because of the -
in ${stripAllFlags:--s}
in setup.sh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but doesn't that do the opposite of what we want here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs say that :-
means:
If parameter is unset or null, the expansion of word is substituted. Otherwise, the value of parameter is substituted.
So yeah it sounds like the space is needed
@tobim libuv builds fine here |
Looks like anything that links against impure system frameworks still isn't reproducable, but that's somewhat to be expected. -/nix/store/1nkad14ypnqhlwn4w4rs287679y09ly3-libuv-1.34.0/lib/libuv.1.dylib:
+/nix/store/hywq7d7bmql8a1w0m28y59pim76dldav-libuv-1.34.0/lib/libuv.1.dylib:
Load command 0
cmd LC_SEGMENT_64
cmdsize 552
@@ -260,7 +260,7 @@
cmdsize 104
name /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (offset 24)
time stamp 2 Thu Jan 1 01:00:02 1970
- current version 1575.23.0
+ current version 1349.8.0
compatibility version 150.0.0
Load command 11
cmd LC_RPATH I've thought before about doing the same as Libsystem for all impure libraries we interact with, also solving some sandboxing problems, but that would be quite hard to maintain. |
Unless anybody has concerns I'm going to merge this soon. |
The build failure on my machine is gone now too. |
Fixes NixOS#21629 Passing these extra linker flags removes both the semi-random uuid included in most binaries as well as making the sdk version consistent instead of based on the current os version. Load command 8 cmd LC_UUID cmdsize 24 uuid 70FAF921-5DC8-371C-B814-4F121FADFDF4 Load command 9 cmd LC_VERSION_MIN_MACOSX cmdsize 16 version 10.12 sdk 10.13 The -macosx_version_min flag isn't strictly necessary since that's already handled by MACOSX_DEPLOYMENT_TARGET.
We _do_ want minimum versions in our packages.
This was initially introduced in 92188d9, not clear how relevant this still is but i686 isn't supported anymore so disable it explicitly.
ddc17f6
to
68513e4
Compare
FYI: Due to regressions in CMake 3.18, |
Motivation for this change
Fixes #21629
Here's an example using hello (without the stdenv changes) https://gist.github.com/LnL7/7e705ea7f4099c074a004c897810bc73
And a run of r13y over the darwin stdenv
https://dgn9s9wft3u6a.cloudfront.net/store/ppm2pvkw1ngvmw063qbkxsg7r0nvdcxi-report/index.html
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)/cc @NixOS/darwin-maintainers