-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
swift: Workaround Hydra darwin build problem #354192
Conversation
Wait, what? It’s a Ninja bump? What? Thank you so much for tracking this down! But I’m going to look at the Ninja commit log for a bit to try and figure out what’s going on here before giving in 😅 |
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 seeing mainly changes to dependency ordering and a move to C++11 in Ninja. The former makes me wonder if this is an incomplete dependency specification that only shows up under heavy load like Hydra builders are constantly under. I wonder if enableParallelBuilding = false;
works, or at least exposes the problem locally too?
It would be great if we could bisect this to the Ninja commit that broke it, but that would probably take a while…
@@ -190,6 +190,24 @@ let | |||
''; | |||
}; | |||
|
|||
ninja' = | |||
if stdenv.hostPlatform.isDarwin then |
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 should probably be buildPlatform
, right?
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.
Since darwin can't be cross compiled, it should function the same I believe. Though buildPlatform
is better suited 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.
Darwin to Darwin cross-compilation was fixed in #346043.
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 real question is whether it breaks compiling a Linux Swift on Darwin. (Probably that doesn’t quite work right now for unrelated reasons, is my guess.)
Anyway if we pin we should just pin unconditionally to match upstream.
I think it would be worth trying the commit immediately before and after the merge of ninja-build/ninja#2177, since the changelog disclaims it as potentially breaking builds. |
Looks like Swift pinned Ninja 1.11 for unrelated reasons: swiftlang/swift#72989. Perhaps we need a proper |
cc @JohnRTitor who bumped Ninja |
Yeah looks like Swift build on Darwin got overlooked.
If quite a few packages don't build with ninja 1.12, I think it's fine to have a 1.11 version. But let's consult ninja maintainers anyway. |
Can we get upstream to fix build with ninja 1.12? And also fix the regressions? Having a outdated pin potentially creates a maintainer hassle downstream. |
I think that's best long term. But, it would still make sense to pin this derivation to 1.11, like they have done upstream, until they fix it. |
If someone found the undeclared dependency in Swift upstream that would be ideal; we could submit a fix upstream and patch it downstream. However since nobody has yet fixed all the other issues we’ve had with the Swift derivation I doubt we can hope for that to happen soon. Maybe for 25.05 when I plan to finally corral an effort to get it in good shape, but ideally we’d not ship 24.11 with a broken Swift. Possible solutions I see:
If we do (2), it should probably be unconditional for all platforms, to match upstream’s decision. |
Note that we’re on a fairly old version of Swift and it’s possible this particular issue was already fixed upstream. But making Swift 5.10/6.0 happen is definitely a 25.05 task. It might be best just to bite the bullet on the old Ninja version for now. |
3fe4fab
to
b1e157d
Compare
Update the PR, override ninja unconditionally. |
|
Just confirmed the problem can be reproduced in your local machines using |
b1e157d
to
52211fe
Compare
Rebase to #354367 |
Needs rebasing for changes in the Ninja PR. Otherwise LGTM! |
Fail to build with ninja 1.12 when NIX_BUILD_CORES is low (Hydra or Github Actions): ``` ld: warning: directory not found for option '-L/nix/store/g9rbp9m6vs1xj4jl6b6vjb6bm8kgr107-SDKs/MacOSX10.15.sdk/usr/lib/swift' ... ld: warning: Could not find or use auto-linked library 'swiftCompatibility56' Undefined symbols for architecture arm64: "__swift_FORCE_LOAD_$_swiftCompatibility56", referenced from: __swift_FORCE_LOAD_$_swiftCompatibility56_$_Optimizer in libswiftCompilerModules-bootstrapping1.a(Optimizer.o) ... ``` Can reproduce using `nix --option cores 2 build -f . swiftPackages.swift-unwrapped`. Until we find out the exact cause, follow [swift upstream][1], pin ninja to version 1.11.1. [1]: swiftlang/swift#72989
52211fe
to
8dfed1b
Compare
Done. |
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.
LGTM
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 community builder is happy. Let’s see if Hydra is.
Thank you again for putting in the work to track this down 💜
When the host platform is darwin, building swift on Hydra and Github Actions with ninja 1.12 will both give the same error:
Beware the failure does not happen on developers' local machines.Edit: You can reproduce the problem using
nix --option cores 2 -L build -f . swiftPackages.swift-unwrapped
.Until we find out the exact cause, pin ninja to version 1.11 to workaround the problem.
I have verified
nix build github:azuwis/nixpkgs/push-yzpukurnozzk#swiftPackages.swift-unwrapped
works on Github Actions macos-latest (aarch64-darwin) withsandbox = relaxed
in/etc/nix/nix.conf
.This will hopefully also fix build problem in Hydra #327836, https://hydra.nixos.org/build/276929157
ZHF: #352882
A bit more about the build failure, compare the last successful build log and first broken build log with
grep swiftCompatibility56
.Last successful build log:
First broken build log:
The broken build log lacks this line
Generating ../../../lib/swift/macosx/libswiftCompatibility56.a
, it seemslibswiftCompatibility56.a
is not generated in the first place, hence the final ld error.And with
grep ninja
.Last successful build log:
First broken build log:
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.