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.

Sign up for GitHub

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

Jump to bottom

clang_15: fix build!=(host==target) cross compilation #226551

Merged
merged 1 commit into from Apr 19, 2023
Merged

clang_15: fix build!=(host==target) cross compilation #226551

merged 1 commit into from Apr 19, 2023

Conversation

ghost
Copy link

@ghost ghost commented Apr 17, 2023

Description of changes

clang_15 appears to not cross compile in the build!=(host==target) case due to two problems, which this commit fixes:

  • It trips -Wmaybe-uninitialized on recent gcc, but only in the build!=host case (likely due to #ifdefs)

  • Two more buildPlatform tools have been added: clang-tidy-confusable-chars-gen and clang-pseudo-gen

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ghost ghost requested a review from matthewbauer as a code owner April 17, 2023 00:10
@ofborg ofborg bot added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Apr 17, 2023
@ghost
Copy link
Author

ghost commented Apr 17, 2023

ofborg build pkgsCross.aarch64-multiplatform.clang_15

@rrbutani
Copy link
Contributor

(I think we have to actually @ ofborg; trying again)

@ofborg build pkgsCross.aarch64-multiplatform.clang_15

rrbutani
rrbutani suggested changes Apr 17, 2023
View reviewed changes
Copy link
Contributor

@rrbutani rrbutani left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! I definitely dropped the ball on making sure we didn't break cross in #194634.

This looks good; I left some notes with minor nits.

Could you also apply these changes to llvmPackages_git? I believe the llvmPackages_git version on master is just slightly too old to pick up llvm/llvm-project@18b4a8b so clang-tidy-confusable-chars-gen won't exist and the build will fail but it's fine; we're bumping llvmPackages_git to 15.0.7 shortly: #222894.

@ghost
Copy link
Author

ghost commented Apr 17, 2023

This looks good; I left some notes with minor nits.

I believe I've taken care of them; if not let me know.

Could you also apply these changes to llvmPackages_git?

Done: b789673fb9d62d1fe86d9ea49dc492ff24c65256

Next force-push will be a squash.

I definitely dropped the ball on making sure we didn't break cross in #194634.

Not at all! We have almost no ofborg/hydra coverage for pkgsCross -- until we do I just sort of accept that things will break a lot. Fortunately I am very, very close to having my daily-driver laptop be completely cross-compiled (by a much faster machine), so any future breakage should be noticed quickly.

@ofborg ofborg bot requested a review from rrbutani April 17, 2023 06:55
@rrbutani
Copy link
Contributor

Looks good, thank you!

I think this may have to target staging though; not sure.

@ofborg ofborg bot requested a review from rrbutani April 17, 2023 07:14
@RaitoBezarius
Copy link
Member

I believe indeed it should target staging.

alyssais
alyssais approved these changes Apr 17, 2023
View reviewed changes
Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

Thanks! I partially had the same fix (without the -Wno-maybe-uninitialized) in a local branch, but was waiting for LLVM 15 and git to be sufficiently in sync before I PRed it. (It's actually what spawned that whole effort.)

Should go to staging though, as others have pointed out.

@alyssais
Copy link
Member

@RaitoBezarius maybe we should merge #222894 first, so we don't forget to add the missing program once it's bumped? Is it waiting for anything?

@rrbutani
Copy link
Contributor

@RaitoBezarius maybe we should merge #222894 first, so we don't forget to add the missing program once it's bumped? Is it waiting for anything?

I believe #222894 is ready; I think we just needed to confirm that the ofborg failures on that PR are spurious.

Not sure I follow re: the missing program; IIUC this PR updates llvmPackages_git to copy all the host tools that are in LLVM 15. If we merge this PR before #222894 llvmPackages_git.llvm will be broken for a bit (and not just under cross) but if this PR is targeting staging I think it's fine? (so long as we merge #222894 before the next staging cycle finishes)

@RaitoBezarius
Copy link
Member

AFAIK we should proceed with the merge of the PR you are mentioning, I was waiting for your final validations.

@rrbutani
clang_15: fix build!=(host==target) cross compilation
77c704a 
clang_15 appears to not cross compile in the build!=(host==target)
case due to two problems, which this commit fixes:

- It trips -Wmaybe-uninitialized on recent gcc, but only in the
  build!=host case (likely due to #ifdefs)

- Two more buildPlatform tools have been added:
  clang-tidy-confusable-chars-gen and clang-pseudo-gen

Co-authored-by: Rahul Butani <[email protected]>
@ghost ghost marked this pull request as draft April 18, 2023 02:42
@ghost ghost changed the base branch from master to staging April 18, 2023 02:42
@ghost
Copy link
Author

ghost commented Apr 18, 2023

I believe indeed it should target staging.

Done.

@ghost ghost marked this pull request as ready for review April 18, 2023 02:43
@ofborg ofborg bot requested review from rrbutani and alyssais April 18, 2023 03:02
@rrbutani rrbutani added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Apr 18, 2023
@alyssais
Copy link
Member

@ofborg build pkgsCross.powernv.clang

@alyssais alyssais merged commit 2e77eb8 into NixOS:staging Apr 19, 2023
@ghost ghost deleted the pr/clang_15/fixcross branch April 20, 2023 19:29
@rrbutani rrbutani mentioned this pull request Jun 19, 2023
Merged
13 tasks
Artturin
Artturin reviewed Aug 21, 2023
View reviewed changes
pkgs/development/compilers/llvm/15/clang/default.nix
@@ -36,9 +36,14 @@ let
"-DSPHINX_OUTPUT_MAN=ON"
"-DSPHINX_OUTPUT_HTML=OFF"
"-DSPHINX_WARNINGS_AS_ERRORS=OFF"
] ++ lib.optionals (stdenv.hostPlatform != stdenv.buildPlatform) [
] ++ lib.optionals (!stdenv.buildPlatform.canExecute stdenv.hostPlatform) [
Copy link
Member

@Artturin Artturin Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Broke pkgsCross.musl64.llvmPackages_16.clang.cc and (presumably, not built) pkgsStatic.llvmPackages_16.clang.cc (15 too)

       > cd /build/clang-src-16.0.6/clang/build && clang-tblgen -gen-clang-diags-defs -clang-component=Serialization -I /build/clang-src-16.0.6/clang/include/clang/Basic -I/build/clang-src-16.0.6/clang/include -I/build/clang-src-16.0.6/clang/build/include -I/nix/store/0jp04rmkq2b25qqar9y6nm0z3dzmziib-llvm-x86_64-unknown-linux-musl-16.0.6-dev/include /build/clang-src-16.0.6/clang/include/clang/Basic/Diagnostic.td --write-if-changed -o include/clang/Basic/DiagnosticSerializationKinds.inc -d include/clang/Basic/DiagnosticSerializationKinds.inc.d
       > /bin/sh: clang-tblgen: not found

Copy link
Author

Choose a reason for hiding this comment

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

#250612

@ghost ghost mentioned this pull request Aug 21, 2023
Closed
12 tasks
@Artturin Artturin mentioned this pull request Jan 29, 2024
Open
@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

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@primeos primeos Awaiting requested review from primeos

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@lovek323 lovek323 Awaiting requested review from lovek323

@sternenseemann sternenseemann Awaiting requested review from sternenseemann

@RaitoBezarius RaitoBezarius Awaiting requested review from RaitoBezarius

@dtzWill dtzWill Awaiting requested review from dtzWill

@alyssais alyssais Awaiting requested review from alyssais

Assignees
No one assigned
Labels
6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ 10.rebuild-linux: 2501-5000 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rrbutani @RaitoBezarius @alyssais @Artturin