-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
compiler-rt: build builtins on darwin #186575
Conversation
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.
should be good to go to staging ; make sure to follow status of staging since sometimes these kind of stdenv changes can cause unforeseen consequences
Thanks for bringing this up, I was wondering where I can follow this? It doesn't look like Hydra builds staging anymore, so is the idea to keep an eye out for the next round of stabilization on staging-next? |
] | ||
++ lib.optionals stdenv.hostPlatform.isDarwin [ | ||
../../common/compiler-rt/darwin-plistbuddy-workaround.patch | ||
# Prevent a compilation error on darwin | ||
./darwin-targetconditionals.patch | ||
] |
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.
Can we make this unconditional?
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 armv7l patch below this was specifically made conditional in d616ae8
But I think the idea is to avoid Darwin stdenv rebuilds? And I'm not sure how much we care about other rebuilds. So the Darwin patches could be made non-optional.
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 armv7l patch below this was specifically made conditional in d616ae8
That would no longer apply when targeting staging because we need to rebuild it anyway already.
Applying the patches always makes it easier to notice when they no longer apply when updating on other platforms.
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 rebased and made the patches non-optional. Verified v11, v12, v13, v14 on aarch64-darwin
(with stdenv rebuild), and verified git at least evaluates. Also did a build of v11 on x86_64-linux
.
The missing xcrun meant builtins were missing from darwin. This apparently wasn't an issue until now, but is in projects using `@available` checks. (The ARM64 hack was apparently the previous solution to fixing broken SDK detection.)
ec646f4
to
fb91bfc
Compare
@@ -15,7 +15,8 @@ stdenv.mkDerivation { | |||
inherit version; | |||
src = fetch "compiler-rt" "0x1j8ngf1zj63wlnns9vlibafq48qcm72p4jpaxkmkb4qw0grwfy"; | |||
|
|||
nativeBuildInputs = [ cmake python3 libllvm.dev ]; | |||
nativeBuildInputs = [ cmake python3 libllvm.dev ] | |||
++ lib.optional stdenv.isDarwin xcbuild.xcrun; |
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.
This won't fly. Hydra doesn't do xcbuild and since this is in the stdenv, we wouldn't get any darwin builds whatsoever.
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.
Oh, what prevents hydra from building xcbuild? I tried to make it stdenv-friendly in #185969 by exposing xcrun
separately, which is just some text files generated by Nix. If that doesn't work, then I guess we'll have to patch the CMake files.
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 fact that builders don't have no Xcode installed (to my knowledge) and that it's highly impure to run external build tools.
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.
Ah, but we don't really use Xcode or the SDKs, just emulate them. For example, our xcrun
is this shell script:
nixpkgs/pkgs/development/tools/xcbuild/wrapper.nix
Lines 54 to 97 in ab26522
xcrun = writeShellScriptBin "xcrun" '' | |
args=( "$@" ) | |
# If an SDK was requested, check that it matches. | |
for ((i = 0; i < ''${#args[@]}; i++)); do | |
case "''${args[i]}" in | |
--sdk | -sdk) | |
i=$((i + 1)) | |
if [[ "''${args[i]}" != '${xcrunSdkName}' ]]; then | |
echo >&2 "xcodebuild: error: SDK \"''${args[i]}\" cannot be located." | |
exit 1 | |
fi | |
;; | |
esac | |
done | |
while [ $# -gt 0 ]; do | |
case "$1" in | |
--sdk | -sdk) shift ;; | |
--toolchain | -toolchain) shift ;; | |
--find | -find | -f) | |
shift | |
command -v $1 ;; | |
--log | -log) ;; # noop | |
--verbose | -verbose) ;; # noop | |
--no-cache | -no-cache) ;; # noop | |
--kill-cache | -kill-cache) ;; # noop | |
--show-sdk-path | -show-sdk-path) | |
echo ${sdks}/${sdkName}.sdk ;; | |
--show-sdk-platform-path | -show-sdk-platform-path) | |
echo ${platforms}/${xcodePlatform}.platform ;; | |
--show-sdk-version | -show-sdk-version) | |
echo ${sdkVer} ;; | |
--show-sdk-build-version | -show-sdk-build-version) | |
echo ${sdkBuildVersion} ;; | |
*) break ;; | |
esac | |
shift | |
done | |
if ! [[ -z "$@" ]]; then | |
exec "$@" | |
fi | |
''; |
And it references fake macOS SDK and platform directories that are generated plist files. Its entire closure is:
$ nix-store -qR /nix/store/7142q0hx4a6pxhy01y3iwhdwngv26a36-xcrun
/nix/store/i7ql4rg0667cc3ppp1s0v133cr2414ww-SDKs
/nix/store/kvji81lb8cgbzcj59qv8v5lqwmpz1kss-Platforms
/nix/store/rdsvxa6vz0hj00jl7f7dn2ik14yfl6p9-bash-5.1-p16
/nix/store/7142q0hx4a6pxhy01y3iwhdwngv26a36-xcrun
I believe it should be fine, but will try to verify. I thought I was already on a clean macOS, working on a machine provisioned by Scaleway, but it seems Xcode is already on it. I'll delete and try a rebuild.
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 removed Xcode and Command-Line Tools, cleaned up my Nix store, then did a rebuild, and stdenv builds correctly. (I did find an impurity in my Swift PR, so good thing we tried!)
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.
Okay then, I was concerned because it's located under the xcbuild umbrella. Since it isn't really related to xcode, wouldn't it be best to give it a more fitting name and put it somewhere else?
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 guess it's related in the sense that xcbuild, xcrun, xcode-select, etc all emulate Xcode tooling. It's probably called xcbuild because the core tool its packaging is facebook/xcbuild? And the rest of it is small Nix shims.
But facebook/xcbuild is also unmaintained. It looks like we have several dozen packages using it, though not sure how many use the xcbuild parts, and how many just need the Nix shims. Also not sure how well it is capable of building newer Xcode projects (including Swift stuff).
It might make sense to move the non-xcbuild stuff to a separate top-level attribute, like xcode-shims
? Maybe @matthewbauer has a stronger opinion. 🙂
Ping! Is this okay to merge? |
This merge introduced an evaluation issue:
There's a longer sad story for that job. By an accident it wasn't evaluated and built by Hydra for a few months – and in the meantime it stopped working. Fixing it completely would be nice, as normally the |
I guess /cc PR #195862 as this is the reason why an "important job" is missing on Hydra. |
(cherry picked from commit 09b8886)
Darwin-specific builtins were present on x86-64-darwin, but not on aarch64-darwin. This is the same issue as #186575, but while the fixes were correctly applied to LLVM 15, we were still disabling the build of builtins on aarch64-darwin. Likely just a confusion.
Description of changes
On Darwin, compiler-rt is missing builtins. There is supposed to be a
libclang_rt.osx.a
, but it is missing completely. I'm a little surprised this wasn't an issue before, but I'm trying to build some code that usesif (@available(...))
which is breaking, because it uses__isPlatformVersionAtLeast
from compiler-rt.I believe this is down to the CMake configuration using
xcrun
to determine what to build for the target SDK, and we simply didn't provide it and ignored the errors. In #185969, I added the necessary functionality to our shell-basedxcrun
, and more importantly made it available separately fromxcbuild
asxcbuild.xcrun
. This hopefully means it's not an objection to make it part of Darwin stdenv, as it's just a bunch of Nix-generated scripts and files.I'm also patching out a check that uses
PlistBuddy
. We could add the field to ourSDKSettings.plist
, but the output of querying an array withPlistBuddy
from facebook/xcbuild is different from Apple'sPlistBuddy
. I felt it was easier to patch the CMake check.Finally, I removed the subtitute rule that disabled ARM64, because it is obviously essential. I believe this was just a workaround for the other issues.
I tested buils of compiler-rt itself on
x86_64-darwin
, versions 12, 13 & 14. Onaarch64-darwin
, I did a full rebuild of stdenv including version 11, and also tested version 13 (part of my intended build).Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes