Skip to content
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

llvmPackages_11: 11.0.0rc5 -> 11.0.0, enable on Darwin #99984

Merged
merged 2 commits into from Oct 16, 2020

Conversation

@ggreif
Copy link
Contributor

@ggreif ggreif commented Oct 7, 2020

Motivation for this change
Things done
  • Added conditionals
  • python3 now mandatory for building libcxx everywhere
  • Bumped to v11.0.0 final
  • 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 label Oct 7, 2020
@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 7, 2020

@GrahamcOfBorg build llvm_11 lld_11 clang_11

@ofborg ofborg bot requested review from dtzWill and 7c6f434c Oct 7, 2020
@ggreif ggreif changed the title llvm_11: don't pass a build-id to linker on Darwin llvmPackages_11: enable on Darwin Oct 8, 2020
@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 8, 2020

@DieGoldeneEnte this builds now for Darwin, but stopped working for Linux. I have no clue why.

@ggreif ggreif changed the title llvmPackages_11: enable on Darwin llvmPackages_11: 11.0.0rc5 -> 11.0.0rc6, enable on Darwin Oct 8, 2020
@ggreif ggreif force-pushed the ggreif:darwin-llvm11 branch from abcf864 to 02cda4e Oct 9, 2020
@ggreif ggreif mentioned this pull request Oct 9, 2020
2 of 10 tasks complete
@DieGoldeneEnte
Copy link
Contributor

@DieGoldeneEnte DieGoldeneEnte commented Oct 9, 2020

libc++ failed,because it coudn't find python, it build normally once python3 is added as native build input.
Could this be, because the llvm-source is added as input?

I need to take another look, why it suddenly needs python...

@ggreif ggreif force-pushed the ggreif:darwin-llvm11 branch from 02cda4e to e7ad07e Oct 12, 2020
@ggreif ggreif changed the title llvmPackages_11: 11.0.0rc5 -> 11.0.0rc6, enable on Darwin llvmPackages_11: 11.0.0rc5 -> 11.0.0 enable on Darwin Oct 12, 2020
@ggreif ggreif changed the title llvmPackages_11: 11.0.0rc5 -> 11.0.0 enable on Darwin llvmPackages_11: 11.0.0rc5 -> 11.0.0, enable on Darwin Oct 12, 2020
@ggreif ggreif marked this pull request as ready for review Oct 12, 2020
@ggreif ggreif requested a review from matthewbauer as a code owner Oct 12, 2020
@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 12, 2020

Could this be, because the llvm-source is added as input?

Then it would fail on Darwin too, right?

@DieGoldeneEnte
Copy link
Contributor

@DieGoldeneEnte DieGoldeneEnte commented Oct 12, 2020

Then it would fail on Darwin too, right?

That's my problem. I couldn't find a relevant difference between the working package in nixpkgs and this.
If I read it correctly, python is required when building libc++ standalone (I don't think we are setting LIBCXX_STANDALONE_BUILD, but python should only be needed if it is set) and using cmake > 3.12. Both should be given for the current nixpkgs derivation.

@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 12, 2020

@DieGoldeneEnte on Darwin I get cmake-3.18.2 with nix-build -A cmake. Same on Linux?

@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 12, 2020

@GrahamcOfBorg build lldb_11

@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 12, 2020

@GrahamcOfBorg build llvmPackages_11

@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 12, 2020

@DieGoldeneEnte Maybe the cmake machinery now runs some tests that require python3. I included Linux now, let's see what OfBorg says.

@ggreif ggreif marked this pull request as draft Oct 13, 2020
@DieGoldeneEnte
Copy link
Contributor

@DieGoldeneEnte DieGoldeneEnte commented Oct 14, 2020

I just tried rebuilding the current master version of libcxx, but it is actually broken, because of the missing monorepo-layout :(
Is this the same problem as on darwin (before the changes here)?

Regarding LIBCXX_STANDALONE_BUILD, this is set by cmake (in libcxx/cmake/Modules/HandleOutOfTreeLLVM.cmake). So I don't think it should be set.
Reading the CMakeLists.txt file, it seems python is mandatory when building standalone (and cmake >3.12). I don't understand, why darwin doesn't need it, since the cmake code is (shortened):

if (LIBCXX_STANDALONE_BUILD)
  if(CMAKE_VERSION VERSION_LESS 3.12)
    [...]
  else()
    find_package(Python3 COMPONENTS Interpreter)
    if(NOT Python3_Interpreter_FOUND)
      find_package(Python2 COMPONENTS Interpreter REQUIRED)
      [...]
    endif()
  endif()
endif()

Are you building libc++ in a sandbox? Maybe cmake finds a python version from the system?

error:

libc++abi now requires being built in a monorepo layout with libcxx available
@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 14, 2020

@DieGoldeneEnte
Copy link
Contributor

@DieGoldeneEnte DieGoldeneEnte commented Oct 14, 2020

shouldn't the test (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR) be true, because we run cmake in the libc++ directory. This would be false if the CMakeLists.txt file would be added via add_subdirectory/add_llvm_external_project (that's at least my understanding given my limited cmake knowledge).

@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 14, 2020

@DieGoldeneEnte yes that test should be true. It is true. So this is a standalone build in a monorepo-layout source.
Darwin looks for a Python3 and doesn't find it. But on my machine it finds some Python2 in a framework, thus it can proceed. It would break in a sandbox.

I am making python3 mandatory now.

@ggreif ggreif marked this pull request as ready for review Oct 14, 2020
@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 14, 2020

@DieGoldeneEnte things look good on my side!

@ggreif ggreif force-pushed the ggreif:darwin-llvm11 branch from ff2ca52 to cffb7cf Oct 14, 2020
@DieGoldeneEnte
Copy link
Contributor

@DieGoldeneEnte DieGoldeneEnte commented Oct 14, 2020

@ggreif Why is the llvm source needed for libc++*? Is it to simulate the monorepo?
Builds without problems on my machine :)

@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 14, 2020

@ggreif Why is the llvm source needed for libc++*? Is it to simulate the monorepo?

That's what I understand, too.

Builds without problems on my machine :)

THX! 🥇

@DieGoldeneEnte
Copy link
Contributor

@DieGoldeneEnte DieGoldeneEnte commented Oct 14, 2020

LGTM then!

I think we will need to rewrite the llvm package since building the projects standalone is less and less supported.

@volth
Copy link
Contributor

@volth volth commented Oct 14, 2020

@GrahamcOfBorg build pkgsi686Linux.llvm_11
@GrahamcOfBorg build pkgsi686Linux.lld_11
@GrahamcOfBorg build pkgsi686Linux.clang_11

@volth
Copy link
Contributor

@volth volth commented Oct 14, 2020

still broken on i686-linux

@DieGoldeneEnte
Copy link
Contributor

@DieGoldeneEnte DieGoldeneEnte commented Oct 15, 2020

Problem is with compilert-rt:

CMake Error at cmake/Modules/AddCompilerRT.cmake:318 (add_library):
  No SOURCES given to target: clang_rt.builtins-i686
Call Stack (most recent call first):
  lib/builtins/CMakeLists.txt:636 (add_compiler_rt_runtime)

fixed it (at least for i686) in my PR targeting this PR-branch.

@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 15, 2020

@volth @DieGoldeneEnte but this is not a regression vs. 11.0.0rcs, right? Is it a regression vs. 10.0.1?

Anyway, better open a PR against those separately.

@DieGoldeneEnte
Copy link
Contributor

@DieGoldeneEnte DieGoldeneEnte commented Oct 15, 2020

@ggreif It was introduced with 11.0.0-rc1, 10.0.1 does not have this change.

@ggreif
Copy link
Contributor Author

@ggreif ggreif commented Oct 15, 2020

@volth
Copy link
Contributor

@volth volth commented Oct 15, 2020

@GrahamcOfBorg build pkgsi686Linux.clang_7
@GrahamcOfBorg build pkgsi686Linux.clang_8
@GrahamcOfBorg build pkgsi686Linux.clang_9
@GrahamcOfBorg build pkgsi686Linux.clang_10

@ggreif ggreif force-pushed the ggreif:darwin-llvm11 branch from b895dd9 to 7a30df9 Oct 15, 2020
primeos pushed a commit that referenced this pull request Oct 16, 2020
compiler-rt (and as a result clang) can't be build for i686 (as noticed here: #99984).
The patch adds the required variables and should result in the same behavior as in the nixpkgs-llvm10. It essentially forces to use i386 buildins when using i486, i586 or i686, which are not supported.

Fixes #100392
Copy link
Member

@primeos primeos left a comment

The diff LGTM :) Thanks @ggreif and @DieGoldeneEnte!

@ggreif is this ready to be merged now?
Edit: I just re-read any comments and everything seems to be resolved :) I'll go ahead and merge this then. Huge thanks again!

@primeos primeos merged commit 86a4a50 into NixOS:master Oct 16, 2020
23 checks passed
23 checks passed
tests
Details
tests
Details
action
Details
action
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
Details
clang_11, clang_11.passthru.tests on aarch64-linux Success
Details
clang_11, clang_11.passthru.tests on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7a30df9"; rev="7a30df9225e3c47080744535ccbb0a1202b36a3a"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7a30df9"; rev="7a30df9225e3c47080744535ccbb0a1202b36a3a"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7a30df9"; rev="7a30df9225e3c47080744535ccbb0a1202b36a3a"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7a30df9"; rev="7a30df9225e3c47080744535ccbb0a1202b36a3a"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7a30df9"; rev="7a30df9225e3c47080744535ccbb0a1202b36a3a"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7a30df9"; rev="7a30df9225e3c47080744535ccbb0a1202b36a3a"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="7a30df9"; rev="7a30df9225e3c47080744535ccbb0a1202b36a3a"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
llvmPackages_11 on aarch64-linux Success
Details
llvmPackages_11 on x86_64-linux Success
Details
@ggreif ggreif deleted the ggreif:darwin-llvm11 branch Oct 16, 2020
primeos added a commit to primeos/nixpkgs that referenced this pull request Nov 4, 2020
compiler-rt (and as a result clang) can't be build for i686 (as noticed here: NixOS#99984).
The patch adds the required variables and should result in the same behavior as in the nixpkgs-llvm10. It essentially forces to use i386 buildins when using i486, i586 or i686, which are not supported.

Fixes NixOS#100392

(cherry picked from commit 6948875)
@primeos primeos mentioned this pull request Nov 9, 2020
3 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.