This is an archive of the discontinued LLVM Phabricator instance.

Support for custom install dirs in CMake build
Needs ReviewPublic

Authored by nalimilan on Jan 3 2017, 7:29 AM.

Details

Reviewers
beanz
Summary

This restores support for custom install directories, which was lost when moving from autotools to CMake. This is needed in particular by distributions to build versioned LLVM packages which are parallel-installable with the latest version.

This introduces new CMAKE_INSTALL_* variables, following the naming convention used by the GNUInstallDirs CMake module: for BINDIR, LIBDIR, INCLUDEDIR, DOCDIR and MANDIR. Existing variables are preserved, and take their defaults from or determine the new ones' defaults as appropriate.

Diff Detail

Repository
rL LLVM

Event Timeline

nalimilan updated this revision to Diff 82885.Jan 3 2017, 7:29 AM
nalimilan retitled this revision from to Support for custom install dirs in CMake build.
nalimilan updated this object.
nalimilan set the repository for this revision to rL LLVM.
beanz edited edge metadata.Jan 9 2017, 6:45 PM

I think this patch is mostly going in the right direction, and I'm actually very happy to see this functionality coming in.

I have a bunch of inline comments below.

CMakeLists.txt
299

In keeping with our conventions these variables probably should start with LLVM instead of CMAKE.

301

Generally speaking having a CMake cache variable that is derived from the value of another cache variable is really bad. If you update LLVM_LIBDIR_SUFFIX in this case CMAKE_INSTALL_LIBDIR will not be updated.

305

This is also bad for similar reasons as above.

310

Again, don't set cached variables off other cached variables.

312

Here too.

tools/llvm-config/BuildVariables.inc.in
26

Maybe rather than LLVM_BINARY_DIR here you keep the INSTALL_BINDIR name. LLVM_BINARY_DIR has a very specific meaning elsewhere in our build system.

The same is true for LLVM_LIBRARY_DIR and LLVM_INCLUDE_DIR, so it is probably best to use the INSTALL_* names there too.

Adding LLVM-commits since it was left off.

mgorny added inline comments.Jan 10 2017, 1:43 AM
CMakeLists.txt
299

And anyway, why were you reinventing GNUInstallDirs? If we want the CMAKE* vars, I don't see a reason not to use the module completely.

Thanks for the review!

CMakeLists.txt
299

I'm not that familiar with CMake, so I don't really know how modules work.I thought I would propose the less invasive patch for a start. But I can use GNUInstallDirs if you think that's the best approach.

301

I see, but what should I do here? Drop LLVM_LIBDIR_SUFFIX? Stop caching it? Same for the other uses, I'm not sure what's the best approach.

305

Is there a reason to use ${project} here instead of just llvm?

mgorny added inline comments.Jan 10 2017, 5:36 AM
CMakeLists.txt
305

Yes. Those paths were used in e.g. clang, and there project evaluated to clang. At least for stand-alone builds, that is.

beanz added inline comments.Jan 10 2017, 11:16 AM
CMakeLists.txt
299

If you use the GNUInstallDirs module, then it makes sense to keep the CMAKE_* prefixes, however GNUInstallDirs may not play well with LLVM_LIBDIR_SUFFIX.

301

I would not include LLVM_LIBDIR_SUFFIX in the cached variable and instead include it at the use sites. CMake has no strategy for cache invalidation, so once a variable is cached it is very difficult to make it not cached.

305

I would drop ${project} from the cached variable and have the docs directory add project at the end in each sub-project.

nalimilan updated this revision to Diff 86946.Feb 3 2017, 2:49 AM
nalimilan marked 5 inline comments as done.

Sorry for the delay. I think this new revision takes into account all remarks. Though the naming of the variables is still open for discussion.

nalimilan marked an inline comment as done.Feb 3 2017, 2:56 AM
nalimilan added inline comments.
CMakeLists.txt
299

Yeah, so we cannot use GNUInstallDirs without breaking backward compatibility WRT LLVM_LIBDIR_SUFFIX. So I won't use it unless you're OK with dropping that variable.

Yet I suggest using the same names instead of adding an LLVM suffix: that's more discoverable for users; it doesn't really make sense to change the same of these standard variables for each project. In particular, Linux distributions offer convenience macros to build packages, which cannot work if variables don't follow a standard scheme.

Though e.g. Fedora currently uses a different convention, with LIB_INSTALL_DIR, INCLUDE_INSTALL_DIR and SHARE_INSTALL_PREFIX. I guess we could use that instead.

beanz added a comment.Feb 8 2017, 2:26 PM

A few comments inline. Since these variables are used by the LLVM CMake modules, they also need to be added to LLVMConfig.cmake.in. Otherwise they will break out-of-tree builds.

CMakeLists.txt
310

Since this isn't cached, you also shouldn't put a type or comment.

312

This one too.

nalimilan updated this revision to Diff 88093.Feb 11 2017, 9:32 AM
nalimilan marked 3 inline comments as done.

Done.

nalimilan updated this revision to Diff 164390.Sep 7 2018, 4:53 AM

Bump. I have rebased against current master.

Hmm, what about using GNUInstallDirs, and adding some extra initial logic to handle LLVM_LIBDIR_SUFFIX? I'm wondering how ugly would it be e.g. to detect when ${CMAKE_INSTALL_LIBDIR} ends with ${LLVM_LIBDIR_SUFFIX}, and unset the latter in that case (maybe with an informatory message).

Given that the PR works as-is and doesn't add too much code, I don't really see the advantage of moving to an "ugly" solution just to use GNUInstallDirs. Is there a strong advantage to that?

Are you seriously asking me if there's an advantage to reusing existing, standard, built-in solution that lets users apply the same solutions across different CMake projects over reinventing the wheel, poorly in an unpredictably incompatible way that's going to surprise everyone who knows GNUInstallDirs and sees CMAKE_INSTALL_LIBDIR that behaves differently than the one he knows?

Given that the PR works as-is and doesn't add too much code, I don't really see the advantage of moving to an "ugly" solution just to use GNUInstallDirs. Is there a strong advantage to that?

Please do use GNUInstallDirs. This way it will be compatible with all cmake users beyond few tested targets and it will be easy to tweak using standard cmake options.

Well, I'm all for using standard solutions in general. But I'm afraid GNUInstallDirs won't play very well with LLVM_LIBDIR_SUFFIX. Let me investigate that again (last time you asked me this was almost two years ago...).

Well, I'm all for using standard solutions in general. But I'm afraid GNUInstallDirs won't play very well with LLVM_LIBDIR_SUFFIX. Let me investigate that again (last time you asked me this was almost two years ago...).

libdir suffix is mostly linux thing and should be handled through GNUInstallDirs.

I am interested in this. Were it to be revived and, say, made to use GNUInstallDirs as others suggest, would this be something that might land?

I am interested in this. Were it to be revived and, say, made to use GNUInstallDirs as others suggest, would this be something that might land?

In general - i think so

r-burns added a subscriber: r-burns.Feb 1 2021, 1:32 AM

Rebase to latest main

Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2021, 7:52 AM

OK so anyone have any opinions on how GNUInstallDirs ought out to interact with the llvm prefix suffix, and build-mode?

Also, while I did the plain rebase reusing this diff, I suppose I should open a new one for the more creatively-different GNUInstallDirs-using version.

I made a new version of this D99484. I figured a separate revision was in order as I substantially revised it and it now supports many more LLVM projects than just llvm/ proper.