This is an archive of the discontinued LLVM Phabricator instance.

[docs] More CMAKE variable documentation
ClosedPublic

Authored by urnathan on Jul 12 2021, 9:00 AM.

Details

Summary

This follows from my D102481 collation of cmake variables by breaking out some (more) common llvm-specific variables. Controlling the subprojects and target architectures, along with clues about restricting build parallelism when linking. 'more common' is somewhat subjective, I take what I use defines that? :)

ok?

Diff Detail

Event Timeline

urnathan created this revision.Jul 12 2021, 9:00 AM
urnathan requested review of this revision.Jul 12 2021, 9:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2021, 9:00 AM
aaron.ballman added inline comments.Jul 14 2021, 12:45 PM
llvm/docs/CMake.rst
187 ↗(On Diff #357959)
224 ↗(On Diff #357959)

I'd move this up above the rarely-used section so that the list goes in order of most-used to least-used. WDYT?

225 ↗(On Diff #357959)

Underlines are off here, so sphinx will complain about this.

228 ↗(On Diff #357959)

One thing that's interesting about this is that I don't know that they're LLVM variables or not. LLVM_ENABLE_PROJECTS isn't really LLVM-specific, it decides what to build as part of the whole project. Similar logic applies to a lot of these items. The only one that's really LLVM-specific in this list is LLVM_TARGETS_TO_BUILD, but even that one impacts Clang.

261 ↗(On Diff #357959)
urnathan marked 3 inline comments as done.Jul 14 2021, 1:59 PM
urnathan added inline comments.
llvm/docs/CMake.rst
224 ↗(On Diff #357959)

I wasn;t sure myself, your comment is enough to do the ordering you suggest.

225 ↗(On Diff #357959)

I don't know what that means -- I tried to figure out how to sphinx it, but failed. I (attempted to) match the formatting of other section headers.

228 ↗(On Diff #357959)

Agreed, there's an unfortunate use of LLVM to mean both 'LLVM library' and 'LLVM project'. I don't know the best way to disambiguate that -- or how to categorize these variables.

261 ↗(On Diff #357959)

muscle memory for the lose!

urnathan updated this revision to Diff 358740.Jul 14 2021, 2:00 PM
urnathan marked 2 inline comments as done.

thanks for the comments. I've addressed what I can, but I don't understand the sphinx thing.

aaron.ballman added inline comments.Jul 15 2021, 3:50 AM
llvm/docs/CMake.rst
213–214 ↗(On Diff #358740)
225 ↗(On Diff #357959)

The count of underlines needs to match the count of characters on the line. I made a suggested edit to show what I mean.

228 ↗(On Diff #357959)

How about using LLVM-related instead of LLVM-specific in the section title and prose? Alternatively, we could just stick them under frequently used and not call out LLVM at all?

xgupta added a subscriber: xgupta.Jul 15 2021, 4:45 AM

I think newcomers first visit getting started to know the common practice or frequently used variable. It will be duplication to write a similar heading in CMake.rst

llvm/docs/CMake.rst
211 ↗(On Diff #358740)

I think similar documentation is available on getting started page - https://llvm.org/docs/GettingStarted.html#getting-the-source-code-and-building-llvm, https://llvm.org/docs/GettingStarted.html#local-llvm-configuration and https://llvm.org/docs/GettingStarted.html#common-problems.

And again the same variables are listed on the same cmake.rst page below so again a duplication.

urnathan marked 3 inline comments as done.Jul 15 2021, 6:06 AM
urnathan added inline comments.
llvm/docs/CMake.rst
225 ↗(On Diff #357959)

ah, I see, thanks.

urnathan updated this revision to Diff 358944.Jul 15 2021, 6:06 AM
urnathan marked an inline comment as done.

I think you may have uploaded the wrong patch, @urnathan. ;-)

urnathan updated this revision to Diff 358950.Jul 15 2021, 6:48 AM

Let's try that again

aaron.ballman accepted this revision.Jul 15 2021, 6:55 AM

LGTM, thank you for the improvements!

llvm/docs/CMake.rst
195–198 ↗(On Diff #358950)

You can re-flow these lines to 80 col.

211 ↗(On Diff #358740)

Good catch on the repeated documentation in places. As a follow-up, it might be nice to have all of those places point to this documentation so we have everything funneled into one place. But I don't think that needs to happen as part of this patch.

This revision is now accepted and ready to land.Jul 15 2021, 6:55 AM
This revision was landed with ongoing or failed builds.Jul 15 2021, 6:58 AM
This revision was automatically updated to reflect the committed changes.
xgupta added inline comments.Jul 15 2021, 6:58 AM
llvm/docs/CMake.rst
211 ↗(On Diff #358740)

Sounds good to me