search

terralang / terra

Terra is a low-level system programming language that is embedded in and meta-programmed by the Lua programming language.
terralang.org
Other
2.72k stars 201 forks source link

Fix Nix include headers #501

Closed ErikMcClure closed 3 years ago

ErikMcClure commented 3 years ago

The previous nix build managed to get the nix tests to pass by incorrectly setting the INCLUDE_PATH environment variable. This does not actually let terra internalize the header files as expected, so while the tests pass, actually trying to use terra will fail. However, LLVM has decided that LLVM_INSTALL_PREFIX now means something completely different than what terra is expecting, so we are forced to simply allow overriding the CLANG_RESOURCE_DIR path and setting it directly when in a nix build.

This also incorporates the nix stable compatibility fix by @seylerius

elliottslaughter commented 3 years ago

Can you explain a little more about why LLVM_INSTALL_PREFIX doesn't match what Terra expects?

If I recall correctly, Clang's own LLVM support was nearly useless in figuring this stuff out. However that seems like the place where we really ought to be getting this information from, in theory.

Having said that, if there isn't an easy answer I'm fine with taking patch as-is.

seylerius commented 3 years ago

Can you explain a little more about why LLVM_INSTALL_PREFIX doesn't match what Terra expects?

If I recall correctly, Clang's own LLVM support was nearly useless in figuring this stuff out. However that seems like the place where we really ought to be getting this information from, in theory.

Having said that, if there isn't an easy answer I'm fine with taking patch as-is.

So, looking at LLVM's source code more closely, specifically L101-L118 of llvm/cmake/modules/CMakeLists.txt and L87-L106 of LLVMConfig.cmake.in, we can see that the entire generated structure of LLVMConfig.cmake seems to build up from the assumption that LLVM_INSTALL_PREFIX is apparently the root, from which /bin and /include and /lib descend. This is typically going to be /, and was the specific Nix derivation path before the packages were split. The problem is that after Nixpkgs' LLVM packages got split, the discrepancy in meanings was revealed — we can see it clearly in L201-210 of Nixpkgs' gnu-install-dirs.patch, where they mod the various paths that were derived from LLVM_INSTALL_PREFIX to also include the specific paths for the different outputs, because the derived paths are no longer in the same place.

What we could do in response to this, if we want to keep sticking with the assumption that Clang is bundled with LLVM (which I'm not convinced is a great assumption to begin with), is to take LLVM_LIBRARY_DIR and then ascend a couple levels in the directory tree, using get_filename_component(... PATH).

ErikMcClure commented 3 years ago

Fundamentally I don't think it's worth trying to avoid simply overriding CLANG_RESOURCE_DIR, because in all cases where LLVM_INSTALL_DIR does not actually correspond to the llvm root, the assumption that clang is bundled with LLVM is a bad one. If clang is bundled than you are probably using a prebuilt LLVM distribution on a sane linux distribution. Otherwise, you're doing something crazy (like using nix) and will just need to specify CLANG_RESOURCE_DIR manually. I do not think what terra is doing is a supported LLVM scenario because I haven't seen any information on how one creates a cmake module that uses clang as a library. I'm also not personally interested in trying to wrangle an explanation out of LLVM - if someone else wants to, they can.

elliottslaughter commented 3 years ago

Merged. I agree this is a reasonable solution given the available options.