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

libcxx: hardcode full path to <version> #155489

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

veprbl
Copy link
Member

@veprbl veprbl commented Jan 18, 2022

Motivation for this change

Fixes: #155458

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Fits CONTRIBUTING.md.
  • nix-build . -A llvmPackages_11.libcxx.dev -A llvmPackages_12.libcxx.dev -A llvmPackages_13.libcxx.dev -A llvmPackages_git.libcxx.dev | xargs grep -r version
  • nix-build -E 'with import ./. {}; itpp.override { stdenv = llvmPackages_git.stdenv; }'

@dtzWill
Copy link
Member

dtzWill commented Feb 1, 2022

Hmm, I'm not sure about this. I thought it was fine at first, but...

This isn't libcxx-private, at least according to: https://en.cppreference.com/w/cpp/header/version . For example, gcc's C++ stdlib has a <version> as well.

This means all sorts of software (boost for example: https://www.boost.org/doc/libs/1_74_0/boost/config/detail/select_stdlib_config.hpp ) may wish to include <version>, and while that might not be breaking now in anything we have currently, it may in the future-- as long as we're incorrectly searching header locations in the wrong order/precedence.

As a small consideration, I'm not sure what the expected stability of these paths are (include/c++/v1/version) and am worried if they change we'll miss it (as we likely propagate this fix forward in the future). This could be fixed, if a bit painfully, by including a check that the injected path indeed exists.

OTOH this is, of course, a problem we should try to fix! Can we do something at the wrapper level to provide the expected behavior here instead?
I would guess the reason we don't have this problem with gcc is because it already 'knows' where to find its C++ bits (where-as we build libcxx separately from clang and so have to point it to the right places via, -isystem or whichever).

The benefit is fixing this at the wrapper (or clang?) level would fix all (current and potential) include'ers of <version> instead of this instance. And possibly any other conflicts between user code/files and C++ standard library header names, as well (although 'version' is offhand probably the most likely to crop up in a source tree :)). Additionally, I think we want(ed) to try to separate gcc from libstdcxx (and other runtime libraries) to keep everything working the same way, but I don't know the status of that.


I guess this is a long way to say I'd prefer if we could fix this more completely, but other than the consideration mentioned above I suppose I don't know that there are significant drawbacks to modifying libcxx in the suggested way (don't see how this would break things really, for example).

cc @Mic92 for possible input on addressing this at the cc-wrapper level (thanks! ❤️).

@veprbl
Copy link
Member Author

veprbl commented Feb 1, 2022

From what I understand, the -I flag is by design allowed to override the system includes, so a change to the cc-wrapper would break that behaviour for all the libcxx includes, unless we move the to different path.

In ideal situation, upstream or us could patch all the software to not conflict with this include from C++20, and it might just happen over time if we just leave things as is. But then folks will be getting the compilation errors, and those kind of look weird.

Also, I wonder why we don't see this with libstdc++ yet.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 31, 2022
@rrbutani rrbutani mentioned this pull request Nov 14, 2022
92 tasks
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

libcxx: internal <version> include is found in build directory
4 participants