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

llvmPackages_15.libcxx: specify LIBCXX_CXX_ABI_{LIBRARY_PATH,INCLUDE_PATHS} for all cxxabis #216273

Closed
wants to merge 1 commit into from
Closed

llvmPackages_15.libcxx: specify LIBCXX_CXX_ABI_{LIBRARY_PATH,INCLUDE_PATHS} for all cxxabis #216273

wants to merge 1 commit into from

Conversation

rrbutani
Copy link
Contributor

@rrbutani rrbutani commented Feb 14, 2023 •
edited
Loading

Description of changes

This PR mostly just fixes #214524 but also takes the time to:

TODO:

  • test the LLVM FreeBSD cross stdenv (cxxrt)
  • make sure all the tests.cc-wrapper.* tests pass
  • modify llvmPackages_git as well
  • modify llvmPackages_16 as is appropriate
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.

@rrbutani rrbutani changed the title Fix/llvm 15 libcxx linker script bug llvmPackages_15.libcxx: fix linker script bug Feb 14, 2023
@rrbutani rrbutani mentioned this pull request Feb 14, 2023
Closed
48 tasks
@ofborg ofborg bot added 8.has: clean-up 8.has: package (new) This PR adds a new package labels Feb 14, 2023
@SchrodingerZhu
Copy link

any update on this?

@rrbutani
Copy link
Contributor Author

rrbutani commented Feb 28, 2023 •
edited
Loading

@SchrodingerZhu I'll work through the remaining TODOs in the next couple of days; is this blocking something else for you?

@SchrodingerZhu
Copy link

SchrodingerZhu commented Feb 28, 2023 •
edited
Loading

Nothing urgent. I am just hoping to setup an LLVM15 environment with libcxxStdenv

@aaronmondal
Copy link
Contributor

This is probably also required for LLVM 16.

@rrbutani
Copy link
Contributor Author

rrbutani commented Apr 4, 2023

Apologies, once again, for the delay; I should have time to finish this and the remaining issues in #213033 this weekend.

This is probably also required for LLVM 16.

I believe this actually isn't; as mentioned the patch we're applying here is present in LLVM 16.

We'll see though; the goal of the other half of this PR is to actually test the LLVM stdenvs a bit so we can catch issues like these 🤞.

@aaronmondal
Copy link
Contributor

I believe this actually isn't; #214524 (comment)

Oh I totally overlooked that. Sorry 🙏

the goal of the other half of this PR is to actually test the LLVM stdenvs a bit so we can catch issues like these

Does the nixpkgs CI run global tests with the LLVM stdenv on x86_64-linux (with glibc, not with musl)? I keep running into curious issues that make it seem like the LLVM toolchain is not really tested against downstream projects. I also noticed that there is no build cache for pkgsLLVM.

Maybe it would make sense to dogfood the LLVM toolchain to a few buildsystem packages like CMake, Meson and Bazel? The tests for such packages might be a helpful sanity check.

@rrbutani
Copy link
Contributor Author

rrbutani commented Apr 4, 2023 •
edited
Loading

Oh I totally overlooked that. Sorry

no worries!

Does the nixpkgs CI run global tests with the LLVM stdenv on x86_64-linux (with glibc, not with musl)? I keep running into curious issues that make it seem like the LLVM toolchain is not really tested against downstream projects. I also noticed that there is no build cache for pkgsLLVM.

I'm not the expert here but I believe the most testing the LLVM stdenv currently gets (outside of packages that explicitly use it in their builds) is this (builds the stdenv but does not use it). This PR adds the cc-wrapper tests.

Maybe it would make sense to dogfood the LLVM toolchain to a few buildsystem packages like CMake, Meson and Bazel? The tests for such packages might be a helpful sanity check.

Definitely! release-cross is set up for exactly this kind of testing but at present (AFAIK) it does not test usage of the LLVM stdenv (i.e. useLLVM = true isn't in the cross system configs it tests). IIRC no channels actually block on that jobset (the health doesn't look great either).

My experience matches yours: I've seen lots of breakage when trying to use the LLVM stdenvs, in their various forms; better testing seems undoubtedly valuable here.

At the moment though, I've been focusing on trying to deduplicate the LLVM package sets (which will make it a little easier to introduce tests and fix bugs and such).

@RaitoBezarius
Copy link
Member

We should definitely extract the cc-wrapper tests stuff and merge it ASAP, for the rest, it can wait ofc, but I am available to work on reviewing or helping on that. :)

@centromere
Copy link
Member

Any word on when this can be merged? I have a package requiring LLVM 15 which I am unable to build without this patch.

I also needed the change mentioned by @LogicalOverflow here.

@DieracDelta
Copy link
Member

DieracDelta commented May 4, 2023 •
edited
Loading

+1 would also love to see this merged. I'm on macos right now (so no linux box). Is there anything I can do to help?

@RaitoBezarius RaitoBezarius requested a review from alyssais May 4, 2023 13:04
@rrbutani
Copy link
Contributor Author

rrbutani commented May 4, 2023 •
edited
Loading

I'll take a look at this tomorrow; IIRC I still needed to look at the proposed fix here and spin off the cc-wrapper-test changes.

@centromere
Copy link
Member

Hi @rrbutani. Did you have any success?

@Artturin
Copy link
Member

Artturin commented Sep 7, 2023

Test changes split to #253752

@Artturin Artturin mentioned this pull request Sep 7, 2023
Merged
12 tasks
@Artturin Artturin force-pushed the fix/llvm-15-libcxx-linker-script-bug branch from 99b2500 to 5786324 Compare September 8, 2023 01:48
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Sep 8, 2023
@Artturin Artturin force-pushed the fix/llvm-15-libcxx-linker-script-bug branch from b54b7e4 to 746ceb3 Compare September 8, 2023 02:32
Artturin
Artturin reviewed Sep 8, 2023
View reviewed changes
pkgs/development/compilers/llvm/15/libcxx/default.nix Outdated
# - https://reviews.llvm.org/D133566
# - https://github.com/NixOS/nixpkgs/issues/214524#issuecomment-1429146432
# !!! Drop in LLVM 16+
(fetchpatch {
Copy link
Member

Choose a reason for hiding this comment

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

I've split this to #253936 as it's ready while the other issue still needs reviewing

@Artturin Artturin mentioned this pull request Sep 8, 2023
Merged
12 tasks
@Artturin
Copy link
Member

Artturin commented Sep 8, 2023

CC @pwaller who did many fixes #246577

@rrbutani @Artturin
llvmPackages_15.libcxx: specify `LIBCXX_CXX_ABI_{LIBRARY_PATH,INCLUDE…
906d39b 
…_PATHS}` for all cxxabis

Previously we only specified `LIBCXX_CXX_ABI_INCLUDE_PATHS` for
`libcxxabi` and not for `libcxxrt` which causes `libcxx`'s build to
fall back to looking in `/usr/include/c++/v1`: https://github.com/llvm/llvm-project/blob/aa656f6c2dec73faceeed21e15401d8f0c743c8b/libcxx/cmake/Modules/HandleLibCXXABI.cmake#L148-L151

We didn't set `LIBCXX_CXX_ABI_LIBRARY_PATH` (and this did not seem to be
a problem; I think this gets defaulted elsewhere in the `libcxx` build)
but it doesn't hurt to.
@Artturin Artturin force-pushed the fix/llvm-15-libcxx-linker-script-bug branch from 2a82460 to a26d168 Compare September 8, 2023 03:16
@pwaller
Copy link
Contributor

pwaller commented Sep 8, 2023

CC @pwaller who did many fixes #246577

[only had a cursory look] Seems reasonable at a glance.

@Artturin Artturin mentioned this pull request Sep 16, 2023
Closed
Artturin
Artturin reviewed Sep 16, 2023
View reviewed changes
pkgs/development/compilers/llvm/15/libcxx/default.nix
Copy link
Member

Choose a reason for hiding this comment

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

are both of the changes needed or only one or is the libcxx/default.nix change unneeded or is it needed in 16 too

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIR, LIBCXX_CXX_ABI_* doesn't exist in 16, which is why I introduced the change to include it via the clangUseLLVM wrapper.

There could be further issues lurking within the LLVM build; some time around LLVM 16 they moved things to assume that you're doing an in-tree build and that libcxx can access libcxxabi's headers; I see that nixpkgs has worked around that to some degree.

I don't fully understand how libcxxabi should get into the compilation environment for nixpkgs, whether it's really a part of libcxx's install or not for example. I consider what I did above as a bit of a hack.

@pwaller pwaller mentioned this pull request Sep 16, 2023
Closed
@Artturin
Copy link
Member

#255488

@Artturin Artturin closed this Sep 21, 2023
@Artturin Artturin reopened this Sep 21, 2023
@Artturin Artturin force-pushed the fix/llvm-15-libcxx-linker-script-bug branch from 185dc1b to 906d39b Compare September 21, 2023 04:32
@Artturin Artturin changed the title llvmPackages_15.libcxx: fix linker script bug llvmPackages_15.libcxx: specify LIBCXX_CXX_ABI_{LIBRARY_PATH,INCLUDE_PATHS} for all cxxabis Sep 21, 2023
@centromere
Copy link
Member

What is preventing this from being merged?

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 5, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 5, 2024
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1688

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@emilazy
Copy link
Member

emilazy commented Oct 26, 2024

I understand this was fixed, I think by #307184. Sorry that this stalled out!

@emilazy emilazy closed this Oct 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Artturin Artturin Artturin left review comments

@pwaller pwaller pwaller left review comments

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@dtzWill dtzWill Awaiting requested review from dtzWill

@lovek323 lovek323 Awaiting requested review from lovek323

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@sternenseemann sternenseemann Awaiting requested review from sternenseemann

@primeos primeos Awaiting requested review from primeos

@alyssais alyssais Awaiting requested review from alyssais

@RaitoBezarius RaitoBezarius Awaiting requested review from RaitoBezarius

Assignees
No one assigned
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

llvmPackages_15: Building with clang++ using libcxx/-stdlib=libc++ fails