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

Ensure all our C/C++ cross compiler search for headers in $includedir and libraries in $libdir #3949

Closed
fingolfin opened this issue Nov 26, 2021 · 8 comments

Comments

@fingolfin
Copy link
Member

fingolfin commented Nov 26, 2021

This is the case for most of them, because SYSROOT/usr/local is a symlink to /workspace/destdir. But not all of them (see also the discussion on PR #3931).

But in a few exceptions, headers are not automatically found if they are in $includedir. The exceptions I know about right now are:

  • GCC 4.8.5 for x86_64-linux-musl
    • this may be caused by GCCBootstrap@4/bundled/patches/gcc494_musl.patch which changes INCLUDE_DEFAULTS from its default value; it may accidentally have removed SYSROOT/usr/local
  • clang version 12.0.0 on x86_64-unknown-freebsd12.2

However, it works for most other compiler variants, including GCC 4.8.5 for GNU libc, or Apple clang. I did not yet try to check this systematically, so I don't know yet what about non-Apple clang on other platforms.

Motivation: without this, one may get surprising build error that then require adding -I$includedir to CFLAGS or similar; but this is not obvious and confusing ("but why does it work everywhere else?"). So I think getting this right (=uniform) will make it easier to add and maintainer builder recipes.

I will try to look into the GCC 4.8.5 on musl issue. But so far, I don't even know where in the clang sources the default include search paths are defined; I simply have not yet even tried to find it, perhaps it is easy... But if anyone has insight into this, I'd appreciate a hint where to look.

@fingolfin
Copy link
Member Author

I think the relevant code in clang is this: https://github.com/llvm/llvm-project/blob/444513510999e4c1ea23253654196793834d53bf/clang/lib/Frontend/InitHeaderSearch.cpp#L235-L255 -- note that AddPath may add SYSROOT as prefix. So AddPath("/usr/local/include", System, false); adds SYSROOT/usr/local/include. Except that call is excluded on a bunch of systems, including FreeBSD. But why?

@giordano
Copy link
Member

But why?

llvm/llvm-project@315c167. Not much more informative...

@benlorenz
Copy link
Contributor

But why?

llvm/llvm-project@315c167. Not much more informative...

FreeBSD rather strictly separates the base-system (installed into /usr) from any extra software installed via ports (into LOCALBASE=/usr/local). So the system compiler should not look into /usr/local to avoid pulling in anything from the ports into the base system (e.g. when recompiling world) unless explicitly requested.
Many ports have some CPPFLAGS=-I${LOCALBASE}/include set for this reason (similarly for -L).

But this doesn't really apply for Yggdrasil, so I guess patching it away wont hurt.
[ Until someone tries to rebuild his FreeBSD system with the clang from Yggdrasil. ]

For Yggdrasil /usr/local (=destdir) should be considered before anything else from the sysroot. For example to avoid something like in #3916.

PS: I do believe that this a very reasonable default for FreeBSD, it just doesn't really fit into Yggdrasil where patching this would make it easier to add new recipes.

@fingolfin
Copy link
Member Author

Thanks @benlorenz that makes sense

@fingolfin
Copy link
Member Author

So I think it's pretty clear how to patch clang to get SYSROOT/usr/local/include to work. But I think it'd involve rebuilding all (?) LLVM_jll versions, or at least Clang_jll, I assume... Is it worth it? Perhaps it could be done at least for the latest clang and moving forward?

Or perhaps one just needs to accept this and instruct all packagers that they may have to add -I$includedir to their CPPFLAGS. Perhaps one could even set a default CPPFLAGS environment variable with that value?

I think it might be still worth it to do this for GCC 4.8.5 on musl (see also PR #3969) because it is the only variant of that GCC where behavior differs.

@fingolfin
Copy link
Member Author

Just run into this again, and then noticed that part of the relevant discussions is not here on this issue. So I'll quote myself from #3969 (comment):

Actually, having thought about this some more (and knowing that clang may also need this in various combinations), perhaps we should use a completely different approach, and just patch clang_compile_flags! and gcc_compile_flags! to add the equivalent of -isystem $includedir. Then no shards have to be changed, and all compilers will work. Am I missing something?

There is some more back and forth on this afterwards, so any readers may wish to follow the link above and read a bit more. But I still think this approach would work and be reasonably "safe".

@giordano
Copy link
Member

giordano commented Mar 1, 2022

Yes, that's fine with me, and being in BinaryBuilderBase/src/Runner.jl means we can easily fix any issues should they arise.

fingolfin added a commit to fingolfin/BinaryBuilderBase.jl that referenced this issue Mar 1, 2022
Currently several JLLs need to manually add `-I${includedir}` to their
CPPFLAGS in order to build correctly on FreeBSD. Fix this by adjusting
the clang wrapper.

See JuliaPackaging/Yggdrasil#3949
fingolfin added a commit to fingolfin/BinaryBuilderBase.jl that referenced this issue Mar 1, 2022
Currently several JLLs need to manually add `-I${includedir}` to their
CPPFLAGS in order to build correctly on FreeBSD. Fix this by adjusting
the clang wrapper.

See JuliaPackaging/Yggdrasil#3949
fingolfin added a commit to fingolfin/BinaryBuilderBase.jl that referenced this issue Mar 1, 2022
Currently several JLLs need to manually add `-I${includedir}` to their
CPPFLAGS in order to build correctly on FreeBSD. Fix this by adjusting
the clang wrapper.

See JuliaPackaging/Yggdrasil#3949
fingolfin added a commit to fingolfin/BinaryBuilderBase.jl that referenced this issue Mar 1, 2022
Currently several JLLs need to manually add `-I${includedir}` to their
CPPFLAGS in order to build correctly on these platforms. Fix this by
adjusting the compiler wrappers.

See JuliaPackaging/Yggdrasil#3949
fingolfin added a commit to fingolfin/BinaryBuilderBase.jl that referenced this issue Mar 3, 2022
Currently several JLLs need to manually add `-I${includedir}` to their
CPPFLAGS in order to build correctly on these platforms. Fix this by
adjusting the compiler wrappers.

See JuliaPackaging/Yggdrasil#3949
fingolfin added a commit to fingolfin/BinaryBuilderBase.jl that referenced this issue Mar 4, 2022
Currently several JLLs need to manually add `-I${includedir}` to their
CPPFLAGS in order to build correctly on these platforms. Fix this by
adjusting the compiler wrappers.

See JuliaPackaging/Yggdrasil#3949
giordano added a commit to JuliaPackaging/BinaryBuilderBase.jl that referenced this issue Mar 4, 2022
)

* Add `-isystem ${includedir}` for clang on FreeBSD, and GCC on musl

Currently several JLLs need to manually add `-I${includedir}` to their
CPPFLAGS in order to build correctly on these platforms. Fix this by
adjusting the compiler wrappers.

See JuliaPackaging/Yggdrasil#3949

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
@fingolfin
Copy link
Member Author

This was resolved some time ago, as discussed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants