Page MenuHomePhabricator

[CMake] Add first CMake cache files
ClosedPublic

Authored by sgraenitz on May 15 2019, 11:03 AM.

Details

Summary

CMake cache scripts pre-populate the CMakeCache in a build directory with commonly used settings.
The CMake invocation from D61952 could look like this:

cmake -G Ninja -C /path/to/llvm-project/lldb/cmake/caches/Apple-lldb-osx.cmake -DLLVM_ENABLE_PROJECTS="clang;libcxx;libcxxabi;lldb" ../llvm-project/llvm

Options specified on the command line will override options in the cache files (as long as caches don't use FORCE).
What do you think? (This is a first proposal and not set in stone.)

Diff Detail

Repository
rL LLVM

Event Timeline

sgraenitz created this revision.May 15 2019, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 15 2019, 11:03 AM
Herald added a subscriber: mgorny. · View Herald Transcript
aprantl added inline comments.May 15 2019, 11:41 AM
lldb/cmake/caches/Apple-lldb-osx.cmake
1 ↗(On Diff #199645)

Filename: Apple-lldb-macOS or apple-lldb-macos?
MacOS X has been renamed to OS X which has been renamed to macOS :-)

No objections from me to checking in caches for common configurations.

I wouldn't want to over-proliferate these, but in general I think this is a good idea. What I would find particularly useful is if we checked in the configurations used by all the bots in here, as that would make it easier to figure out what's going on if one of them breaks and the reason is not obvious...

lldb/cmake/caches/Apple-lldb-osx.cmake
4 ↗(On Diff #199645)

Are you sure including {CMAKE_CURRENT_BINARY_DIR} here is a good idea? I think llvm tries to avoid that generally, but my experience from other projects tells me that the value of install prefix tends to end up embedded in the final binaries in various ways.

I believe the expected usage of this variable is to make it point to the final resting place of the executables, which in your case would be /System or whatever. Then, if you want to copy the to-be-installed files into a staging area first, you're expected to do that with "DESTDIR=whatever ninja install".

sgraenitz updated this revision to Diff 199817.May 16 2019, 6:59 AM
sgraenitz marked an inline comment as done.

Rename macOS specific cache file.
Add settings and comments.
Fix install destinations by appending Developer and SharedFrameworks respectively.

Thanks for your input.

I wouldn't want to over-proliferate these but in general I think this is a good idea.

Agreed, caches for most common configurations only. Also we may not over-proliferate includes across caches.
In a previous sketch I had extra variants for development and release builds. I preferred to keep only one variant for now and make notes for release builds instead, because actual release configurations are likely more complex anyway.
For downstream repositories I am not sure what's the best approach, but I'd tend to: create their own cache files, include one upstream cache file and add downstream options.

What I would find particularly useful is if we checked in the configurations used by all the bots in here, as that would make it easier to figure out what's going on if one of them breaks and the reason is not obvious...

Interesting idea. This would go in the direction of CI systems with checked-in build/test configurations (like .travis.yml or .gitlab-ci.yml)? In my experience this works great with docker containers, because they provide a stable environment, but I remember well that debugging Travis CI builds was a big time sink before containerization. Hence, I wonder whether it comes with drawbacks in our case. We don't use containers exclusively. We do have local differences between machines and bot configurations smooth them out explicitly. I am not fond of putting thing like this in a cache file:

-DPYTHON_EXECUTABLE=/usr/bin/python2.7
-DPYTHON_LIBRARY=/System/Library/Frameworks/Python.framework/Versions/2.7/lib/libpython2.7.dylib

Furthermore, fixing bot configurations can be painful and often enough experimental changes are committed (e.g. https://github.com/llvm/llvm-zorg/commits/master/zorg/jenkins/jobs/jobs/lldb-cmake-standalone). Not sure if we want this in the LLDB history? I guess there's a reason why zorg is not part of the monorepo? :-) Also, I think fixing bot configurations typically goes hand in hand with changes on build bot scripts (which we don't have in the repo either) more than with changes on the codebase itself. I don't see it getting easier by moving the configurations to the codebase..

With all this considered, I wouldn't prefer to check in concrete caches for build bots in lldb. We could have them in zorg right?
Instead lldb could have something like GreenDragon-lldb-base.cmake that provides and explains reasonable default settings for a range of bots/environments. Actual build bot configs in zorg could then be handled the same way as downstream repos.

Might that be a way forward? We may brainstorm a list of candidates or just handle them as they come up.

lldb/cmake/caches/Apple-lldb-macOS.cmake
10 ↗(On Diff #199817)

Follow-up from: https://reviews.llvm.org/D61956?id=199645#inline-550727

I believe the expected usage of this variable is to make it point to the final resting place of the executables, ...

It's been a pragmatic decision. Maybe we can improve this. The rationale was, that the default configuration should give the user something that works without touching caches or overriding parameters. In a previous sketch I used a real-world destination like:

set(CMAKE_INSTALL_PREFIX /Applications/Xcode.app/Contents/Developer/usr/)

But then ninja install would fail by default due to lack of permissions in the install destination. Actual release configurations tend to be more complex anyway and vendors will have their own downstream repos / caches for it. Thus, choosing a good default for developers sounded like a good way forward. What do you think?

Are you sure including {CMAKE_CURRENT_BINARY_DIR} here is a good idea? I think llvm tries to avoid that generally, ...

What exactly do you mean? Having absolute paths, or paths into the build-tree, or the CMAKE_CURRENT_BINARY_DIR specifically? I don't see problems with the two last points. Am I missing something?

For the first: choosing an absolute path was for consistency with LLDB_FRAMEWORK_INSTALL_DIR. In the current build logic, they can both be absolute paths. Otherwise:

  • if the install prefix is relative, it will be appended to the path of the root build directory
  • if the framework install dir is relative, it will be appended to the install prefix

Then, if you want to copy the to-be-installed files into a staging area first, you're expected to do that with "DESTDIR=whatever ninja install".

Clang cache scripts seem to accomplish it manually, which may look like this (but the default would again fail due to privileges):

if($ENV{DESTDIR})
  set(CMAKE_INSTALL_PREFIX $ENV{DESTDIR})
  set(LLDB_FRAMEWORK_INSTALL_DIR "../../SharedFrameworks" CACHE STRING "")
else()
  set(CMAKE_INSTALL_PREFIX /Applications/Xcode.app/Contents/Developer/usr)
  set(LLDB_FRAMEWORK_INSTALL_DIR /Applications/Xcode.app/Contents/SharedFrameworks CACHE STRING "")
endif()

Would you (and other reviewers) prefer this solution?

18 ↗(On Diff #199817)

Can / Should we add this? Here?

Instead lldb could have something like GreenDragon-lldb-base.cmake that provides and explains reasonable default settings for a range of bots/environments. Actual build bot configs in zorg could then be handled the same way as downstream repos.

Yeah, that's more or less what I had in mind. I didn't mean to include paths to various things, which are only valid on the single machine that the bot is running on, but more like if a bot is using some specific combo of LLVM_ENABLE_ASSERTS/LLVM_ENABLE_MODULES/LLVM_LINK_LLVM_DYLIB/... settings then these things might be helpful in figuring out why something is only failing there. Though I suppose these things are accessible through the buildbot UIs already, so it's already accessible in other ways. I think it still might be useful to have these configs accessible in a the repo for to make it easier to use them, but it's not a big deal either. The other llvm projects don't do that afaik, so it may be better to follow that example..

Having the buildbot config changes in the history is also an interesting question. On one hand, they do clutter the history, but on the other, they make it more obvious that something has changed -- if a bot mysteriously starts failing after one of my changes, i'd like to know that this could be due to a configuration change that happened at the same time -- this assumes that the configuration changes take effect instantly, and not after a bot reboot as is the case with buildbot (I'm not sure how greendragon stands on this front).

lldb/cmake/caches/Apple-lldb-macOS.cmake
10 ↗(On Diff #199817)

But then ninja install would fail by default due to lack of permissions in the install destination. Actual release configurations tend to be more complex anyway and vendors will have their own downstream repos / caches for it. Thus, choosing a good default for developers sounded like a good way forward. What do you think?

I don't think most developers actually run the "install" rule TBH. :) But if they do, I think we should encourage them to do the right thing, and run "DESTDIR=foo ninja install", which will work even with a real-world prefix. At least that is the right thing to do in the linuxy world -- on a mac I guess most people will not be able to install lldb to the system destination anyway, so I'm not sure what would be a sensible default.

Would you (and other reviewers) prefer this solution?

I don't care that much about this TBH. I just wanted to explain the difference between the install prefix and destdir, because in my experience, a lot of people get those two mixed up. ccing @mgorny, in case I'm saying something wrong, as he does a lot of building and installing..

Add back CACHE STRING "" for CMAKE variables

compnerd added inline comments.May 16 2019, 10:47 AM
lldb/cmake/caches/Apple-lldb-macOS.cmake
18 ↗(On Diff #199817)

This is fine to add assuming that you are building on Darwin since that implies clang and that will work. I would say that if you really want to be pedantically correct, it would be better to do:

if(CMAKE_CXX_COMPILER_ID MATCHES Clang)
  set(LLVM_ENABLE_MODULES_ON CACHE BOOL "")
endif()

Note that the MATCHES is required here to match both Clang and AppleClang.

sgraenitz added inline comments.May 16 2019, 10:49 AM
lldb/cmake/caches/Apple-lldb-macOS.cmake
10 ↗(On Diff #199817)

I just wanted to explain the difference between the install prefix and destdir

Yes, that was a good point: DESTDIR is the location of the install-tree on the build machine, PREFIX is the install location on the enduser machine. Right? Sorry, I didn't come back to it. The Clang cache script I referred to, doesn't respect that.

sgraenitz added inline comments.May 16 2019, 10:57 AM
lldb/cmake/caches/Apple-lldb-macOS.cmake
18 ↗(On Diff #199817)

That sounds reasonable. I would take your version and put it in the base cache?

The other question was, whether RelWithDebInfo is a good default. Personally, I use it far more often than other configurations. (Running the test suite with a debug Clang is just too slow.) Moving to the base cache too.

labath added inline comments.May 16 2019, 11:09 AM
lldb/cmake/caches/Apple-lldb-macOS.cmake
18 ↗(On Diff #199817)

Are you sure that CMAKE_CXX_COMPILER_ID is available this early in the initialization ?

I noticed you have lots of comments that say "Release has different values for these variables". I think that you could instead guard the Release configuration behind a check. For example:

if (CMAKE_BUILD_TYPE MATCHES Release)
  # Do the release configuration stuff here
else()
  # Do the development configuration stuff here
endif()
lldb/cmake/caches/Apple-lldb-macOS.cmake
10 ↗(On Diff #199817)

The example you posted above using the clang cache file is quite sketchy imo. I recommend reading the page on DESTDIR, as I think it clears up some things: https://cmake.org/cmake/help/latest/envvar/DESTDIR.html

Taking the example above that you posted, if you set CMAKE_INSTALL_PREFIX to $ENV{DESTDIR}, you could end up with some strange results. For example, let DESTDIR="/Users/foo/". Installing something would mean that it would end up relative to the path "/Users/foo/Users/foo", which is probably not what you want. DESTDIR is a way of relocating the entire install tree to a non-default location. It in some sense "re-roots" your install tree.

As for using CMAKE_CURRENT_BINARY_DIR inside the CMAKE_INSTALL_PREFIX and LLDB_FRAMEWORK_INSTALL_DIR, I would instead rely on DESTDIR here and make CMAKE_INSTALL_PREFIX something like Developer/usr and LLDB_FRAMEWORK_INSTALL_DIR something like SharedFrameworks.

18 ↗(On Diff #199817)

I think RelWithDebInfo is an okay default. I personally don't like to set build types in caches because I think it's reasonable to expect the user to specify their build type, but if you mostly use that one build type then it's fine.

xiaobai added inline comments.May 16 2019, 11:35 AM
lldb/cmake/caches/Apple-lldb-macOS.cmake
18 ↗(On Diff #199817)

I don't think CMAKE_CXX_COMPILER_ID is available at the cache processing stage of initialization, so this is probably not something you can do. Maybe you can guard with the condition that the OS is Darwin? Maybe that's not needed, since the cache is Apple-specific anyway.

sgraenitz updated this revision to Diff 200007.May 17 2019, 3:15 AM
sgraenitz marked 11 inline comments as done.

Fix default install locations and add comments on how to use DESTDIR

I noticed you have lots of comments that say "Release has different values for these variables".

Yes, I was arguing: In a previous sketch I had extra variants for development and release builds. I preferred to keep only one variant for now and make notes for release builds instead, because actual release configurations are likely more complex anyway.

I think that you could instead guard the Release configuration behind a check.

This would require setting CMAKE_BUILD_TYPE before loading the cache and it's not obvious when looking at the cache file.
I think we should avoid this, because the more useful command line order is: first load the cache, then override options on top of it. Let's not motivate people to mix the two. Thus, I'd keep it as is.

Thanks for all the input and discussions. I think this is something we have to iterate on. The current state looks like a good starting point to me. If there are no major concerns, I would land this some time soon.

lldb/cmake/caches/Apple-lldb-macOS.cmake
18 ↗(On Diff #199817)

Build type can still be set on the command line, it just feels like a better default then Debug.
Everything else: agreed. Will keep it as is.

This revision was not accepted when it landed; it landed in state Needs Review.May 17 2019, 12:17 PM
Closed by commit rL361069: [CMake] Add first CMake cache files (authored by stefan.graenitz, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2019, 12:17 PM

Btw, options on the command line always override what's in the cache. Has nothing to do with FORCE. All FORCE does is make sure the set command actually changes an existing cache value. So it's an ordering issue. If the -D comes before the -C then using FORCE would override, but if the -C comes before the -D, the -D always overrides.

I always use FORCE and put my overrides after the cache file.

BTW, given that we recommend using ninja pretty much everywhere, you might want to include set(CMAKE_GENERATOR Ninja)