Yggdrasil icon indicating copy to clipboard operation
Yggdrasil copied to clipboard

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

Open fingolfin opened this issue 4 years ago • 7 comments

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 avatar Nov 26 '21 11:11 fingolfin

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?

fingolfin avatar Nov 26 '21 22:11 fingolfin

But why?

https://github.com/llvm/llvm-project/commit/315c1675e8ed558aa827f56af260c85e03f07477. Not much more informative...

giordano avatar Nov 26 '21 22:11 giordano

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.

benlorenz avatar Nov 26 '21 23:11 benlorenz

Thanks @benlorenz that makes sense

fingolfin avatar Nov 26 '21 23:11 fingolfin

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 avatar Nov 29 '21 14:11 fingolfin

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 https://github.com/JuliaPackaging/Yggdrasil/pull/3969#issuecomment-985614781:

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".

fingolfin avatar Feb 28 '22 23:02 fingolfin

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

giordano avatar Mar 01 '22 01:03 giordano

This was resolved some time ago, as discussed

fingolfin avatar Oct 25 '22 20:10 fingolfin