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

llvm: fix cross-compilation #51226

Merged
merged 1 commit into from Dec 8, 2018
Merged

llvm: fix cross-compilation #51226

merged 1 commit into from Dec 8, 2018

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented Nov 29, 2018

Motivation for this change

We build for both the host and target platform because we need this for rust.
I have not yet checked the build for all llvm versions.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Mic92 Mic92 changed the title llvm: fix cross-compilation [WIP] llvm: fix cross-compilation Nov 29, 2018
@Mic92 Mic92 mentioned this pull request Nov 29, 2018
13 tasks
@matthewbauer
Copy link
Member

Does LLVM_TARGETS_TO_BUILD not support the target triple?

@Mic92
Copy link
Member Author

Mic92 commented Nov 29, 2018

No. This is about CPU architectures not operating systems.

Copy link
Member

@dtzWill dtzWill left a comment

Choose a reason for hiding this comment

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

I'll try to get to the other bits soon, but for now a stylistic suggestion :).

else if platform.parsed.cpu.family == "mips" then
"Mips"
else
throw "Unsupported system";
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps a "switch"-style instead here?

{
  mips = "Mips";
  x86 = "X86";
   ...
}."${platform.parsed.cpu.family}" or throw "Unsupported system"

(or thereabouts)

Copy link
Member

Choose a reason for hiding this comment

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

Shoudn't LLVM_TARGETS_TO_BUILD default to all? Not sure what the rust error is though:

https://llvm.org/docs/CMake.html#llvm-specific-variables

Copy link
Member

Choose a reason for hiding this comment

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

If it does then yes we can remove any mention of target and share llvms. Just compiler-rt would be rebuilt.

Copy link
Member

Choose a reason for hiding this comment

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

Should we be concerned about output size/build times with this? LLVM_TARGETS_TO_BUILD is probably being overriden from the default right now when TARGET_TRIPLE is set. I kind of like that GCC's target-specific stuff doesn't give you anything for free (you have to set depsBuildBuild, nativeBuildInputs where appropriate). We might also still need DEFAULT_TARGET_TRIPLE.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not do that because libllvm is also used for jit compiling and therefore a dependency of some applications. Do you think building all backends would have many users? Most of the time I think one picks one host platform to compile for and this is also how our cross-buildsystem is designed. So it won't be useful for many people to have all backends.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dtzWill the switch case style does not work here because of I have to use platform.parsed.cpu.name instead of family for aarch64.

@Ericson2314
Copy link
Member

Actually, you probably won't need this is you use buildPackages.llvm and buildPackages.buildPackages.llvm. I do like the explicit target list though so we can make it a package option and then map that function over it.

@matthewbauer
Copy link
Member

@Ericson2314 should we change "family" to use the capitalized version? Since we use LLVM as a basis for lib/systems/, it seems like we should keep them as close as possible to what LLVM accepts.

@Ericson2314
Copy link
Member

@matthewbauer if you look at http://llvm.org/doxygen/classllvm_1_1Triple.html it does use the lower case names for those. I'm fine with changing it, but just want to be clear that LLVM may not consistently be upper-case about this either.

@Mic92
Copy link
Member Author

Mic92 commented Dec 8, 2018

There is one test case failing on older llvm versions: https://gist.github.com/Mic92/df4ae94fdb5071cfbe660b1c753e9b20

If someone is interested in fixing this, have a look at my old branch: https://github.com/Mic92/nixpkgs/tree/llvm-cross-targets-all

@Mic92 Mic92 changed the title [WIP] llvm: fix cross-compilation llvm: fix cross-compilation Dec 8, 2018
@Mic92 Mic92 merged commit 7a4c81d into NixOS:staging Dec 8, 2018
@Mic92 Mic92 deleted the llvm-cross-targets branch December 8, 2018 19:27
@jtojnar
Copy link
Contributor

jtojnar commented Dec 24, 2018

This broke mesa_noglu, see #52772

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

Successfully merging this pull request may close these issues.

None yet

6 participants