-
Notifications
You must be signed in to change notification settings - Fork 14.1k
[docs] Rewrite HowToCrossCompileLLVM #129451
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
Conversation
The document has had a few minor tweaks over the years, but the last major piece of work on it was 2016, after first being introduced in 2013. My aim is to provide a clear and clean recipe for cross-compiling LLVM that: * Should be achievable for anyone on common variants of Linux (_including_ the step of acquiring a working sysroot). * I think I've kept the coverage of setting up acquiring a Debian sysroot minimal enough that it can reasonably be included. `debootstrap` is packaged for most common Linux distributions including non-Debian derived distributions like Arch Linux and Fedora. * Describes a setup that we can reasonably support within the community. * I realise with the ninja symlink canonicalisation issue I haven't completely avoided hacks, but I look particularly to point 2 under hacks in the current docs which talks about libraries on the host being found by CMake and adding `-L` and `-I` to try to hack around this. We've all been there and made these kind of temporary workarounds to see if we can get further, but it's very hard to support someone who has problems with a setup that's improperly leaking between the host and target like this. The approach I describe with a clean sysroot and setting appropriate `CMAKE_FIND_ROOT_PATH_MODE_*` settings doesn't have this issue. * Cuts down on extraneous / outdated information, especially where it is better covered elsewhere (e.g. detailed descriptions of CMake options not directly relevant to cross compilation). I've run through the instructions for AArch64, RISC-V (64-bit), and armhf.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would avoid contractions, and I think in a couple of places here it is clearer to have, for example, "do not" instead of "don't". Since we are dealing with exact steps.
llvm/docs/HowToCrossCompileLLVM.rst
Outdated
|
||
The packages you'll need are: | ||
These instructions have been tested for targeting 32-bit ARM, AArch64, or | ||
RISC-V but should be equally applicable to any other target. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could put this in a .. note
so it stands out, because someone will miss it.
Maybe I'll think different when I've reviewed the whole thing but putting the arch names in the subtitle is probably best overall just for search purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done this and changed the heading, as the overall nesting of headings didn't make sense anyway.
llvm/docs/HowToCrossCompileLLVM.rst
Outdated
which will create a sysroot on the install-dir. You can then tar | ||
that directory into a binary with the full triple name (for easy | ||
identification), like: | ||
After LLVM/Clang has built successfully, you can install it via: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to say that most people do not need to set the install prefix setting in the previous section, it seems weird to show use of the install target here with no warning.
Maybe we should recommend that folks always set that setting to some path, just in case?
Removing a bunch of files scattered around my system is going to be a pain. It's ok for us working on imaged corporate machines or containers, but worse for anyone else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe ..note: Use of the ``install`` target requires that you have <set the thing> otherwise <bad stuff happens>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to make it clearer why you might want to install and the note about setting CMAKE_INSTALL_PREFIX as suggested.
@DavidSpickett thanks for the thorough review. I believe all comments are addressed, and I've also done a pass to remove contractions as suggested. I hope you don't mind I've marked as resolved the suggestions that were just "suggest change to X" where I've done exactly as requested. Anything where there's some level of interpretation as to whether the edit I made solved the issue raised, I've left unresolved. I think it's ready for another look once you get a chance - thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just a few small things.
Also I will hold off approving because guides always benefit from a few different reviewers. |
@tru @petrhosek @rengolin Might any of you be able to be a second pair of eyes and give an approving review, as David suggests? Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, just two minor comments.
find downloads for all architectures. | ||
cmake -G Ninja \ | ||
-DCMAKE_BUILD_TYPE=Release \ | ||
-DLLVM_ENABLE_PROJECTS="lld;clang" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth mentioning compiler-rt (and libc++)? it could be challenging to set up and readers might want to know how to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be a great follow-up improvement. My goal with this PR is to make something that's covers the same content as the original documentation, but in a better way.
.. note:: | ||
Use of the ``install`` target requires that you have set | ||
``CMAKE_INSTALL_PREFIX`` otherwise it will attempt to install in | ||
directories under `/` on your host. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another variable that's relevant and worth mentioning here is DESTDIR
. The directory install
target will install files to is $DESTDIR/$CMAKE_INSTALL_PREFIX
so even if CMAKE_INSTALL_PREFIX
is set to /
you can still change the destination directory to avoid overwriting files under /
on your host.
This is commonly used by package managers, e.g. you might set CMAKE_INSTALL_PREFIX
to /usr
and DESTDIR
to /path/to/stage
.
The document has had a few minor tweaks over the years, but the last major piece of work on it was 2016, after first being introduced in
2013. My aim is to provide a clear and clean recipe for cross-compiling LLVM that:
debootstrap
is packaged for most common Linux distributions including non-Debian derived distributions like Arch Linux and Fedora.-L
and-I
to try to hack around this. We've all been there and made these kind of temporary workarounds to see if we can get further, but it's very hard to support someone who has problems with a setup that's improperly leaking between the host and target like this. The approach I describe with a clean sysroot and setting appropriateCMAKE_FIND_ROOT_PATH_MODE_*
settings doesn't have this issue.I've run through the instructions for AArch64, RISC-V (64-bit), and armhf.