Skip to content

llvmPackages_11.compiler-rt: fix build on x86_64-darwin#123524

Merged
jonringer merged 1 commit intoNixOS:staging-nextfrom
thefloweringash:compiler-rt
May 19, 2021
Merged

llvmPackages_11.compiler-rt: fix build on x86_64-darwin#123524
jonringer merged 1 commit intoNixOS:staging-nextfrom
thefloweringash:compiler-rt

Conversation

@thefloweringash
Copy link
Member

@thefloweringash thefloweringash commented May 18, 2021

Amendment to changes in 56fcbcd.

Motivation for this change

llvmPackages_11.compiler-rt is broken on staging-next, see Hydra.

The diff is slightly more confusing than it could be, since it's an amendment to an earlier commit. To see the logical diff use:

git diff c99904e11307cbade607ad855a29f07694762f37 82a616d815fa452168c9009697a00d43481c9453 -- pkgs/development/compilers/llvm/11

Tested up to llvmPackages_11.libcxx for both x86_64-darwin native and x86_64-darwin->aarch64-darwin cross. I'm building aarch64-darwin native, but it will take a few hours to test and I want to unblock staging-next.

I'm not as confident as I would like to be about the state of llvm bootstrapping on darwin, but at least this contains the interesting logic to aarch64-darwin.

cc @Ericson2314 @NixOS/release-engineers

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label May 18, 2021
@ofborg ofborg bot requested review from 7c6f434c, dtzWill, lovek323 and primeos May 18, 2021 14:47
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels May 18, 2021
Ericson2314
Ericson2314 reviewed May 18, 2021
View reviewed changes
Ericson2314
Ericson2314 reviewed May 18, 2021
View reviewed changes
pkgs/development/compilers/llvm/11/compiler-rt/default.nix Outdated
Comment on lines 28 to 29
Copy link
Member

@Ericson2314 Ericson2314 May 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can just do this! Everything else I think we just beating around the bush.

Suggested change
] ++ lib.optionals (useLLVM || bareMetal || isMusl ||
(stdenv.hostPlatform.isDarwin && stdenv.hostPlatform.isAarch64)) [
] ++ lib.optionals (!haveLibc || baremetal || isMusl /* || isNewDarwinBootstrap */) [

I don't know why it doesn't build for aarch64-darwin though.

Well it doesn't build for musl either so we could just chalk it up to these being brittle or something.

Copy link
Member

@Ericson2314 Ericson2314 May 18, 2021

Choose a reason for hiding this comment

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

Sorry I edited this a few times, I'll think about it and do another.

Copy link
Member Author

@thefloweringash thefloweringash May 18, 2021

Choose a reason for hiding this comment

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

Won’t this disable the sanitizers on some platforms where they work, notably x86_64-darwin?

Copy link
Member

@Ericson2314 Ericson2314 May 18, 2021

Choose a reason for hiding this comment

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

I think after all my second-guessing and editing -- no :).

Copy link
Member Author

@thefloweringash thefloweringash May 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is that comment around isNewDarwinBootstrap intentional?

Ericson2314
Ericson2314 reviewed May 18, 2021
View reviewed changes
pkgs/development/compilers/llvm/11/compiler-rt/default.nix Outdated
Copy link
Member

@Ericson2314 Ericson2314 May 18, 2021

Choose a reason for hiding this comment

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

Suggested change
] ++ lib.optionals ((useLLVM || bareMetal) && !haveLibc) [
] ++ lib.optionals (!haveLibc || bareMetal) [

Ericson2314
Ericson2314 reviewed May 18, 2021
View reviewed changes
pkgs/development/compilers/llvm/11/compiler-rt/default.nix Outdated
Copy link
Member

@Ericson2314 Ericson2314 May 18, 2021

Choose a reason for hiding this comment

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

Suggested change
] ++ lib.optionals (useLLVM && !haveLibc) [
] ++ lib.optionals (!haveLibc) [

@s1341 I think this will still be fine for android? No reason not to link bionic right?

Copy link
Contributor

@s1341 s1341 May 18, 2021

Choose a reason for hiding this comment

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

I think so.

Copy link
Contributor

@s1341 s1341 May 18, 2021

Choose a reason for hiding this comment

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

aarch64-android is using llvm 12 anyway, but I imagine you'll port this change to 12 too.

@thefloweringash @Ericson2314
llvmPackages_11.compiler-rt: fix build on x86_64-darwin
2794eda 
Amendment to changes in 56fcbcd.

Co-authored-by: John Ericson <[email protected]>
@jonringer
Copy link
Contributor

jonringer commented May 19, 2021

@GrahamcOfBorg build llvmPackages_11.compiler-rt

@reckenrode reckenrode mentioned this pull request Sep 22, 2023
Closed
13 tasks
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@primeos primeos Awaiting requested review from primeos

@7c6f434c 7c6f434c Awaiting requested review from 7c6f434c

@dtzWill dtzWill Awaiting requested review from dtzWill

@lovek323 lovek323 Awaiting requested review from lovek323

+2 more reviewers

@s1341 s1341 s1341 left review comments

@jonringer jonringer jonringer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: darwin Running or building packages on Darwin 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@thefloweringash @jonringer @Ericson2314 @s1341 @rrbutani