Support for custom install dirs in CMake build

[nalimilan]

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.

[beanz]

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 ↗

(On Diff #82885)

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

301 ↗

(On Diff #82885)

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 ↗

(On Diff #82885)

This is also bad for similar reasons as above.

310 ↗

(On Diff #82885)

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

312 ↗

(On Diff #82885)

Here too.

tools/llvm-config/BuildVariables.inc.in

26 ↗

(On Diff #82885)

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.

[beanz]

Adding LLVM-commits since it was left off.

[mgorny]

CMakeLists.txt

299 ↗

(On Diff #82885)

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.

[nalimilan]

Thanks for the review!

CMakeLists.txt

299 ↗

(On Diff #82885)

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 ↗

(On Diff #82885)

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 ↗

(On Diff #82885)

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

[mgorny]

CMakeLists.txt

305 ↗

(On Diff #82885)

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

[beanz]

CMakeLists.txt

299 ↗

(On Diff #82885)

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 ↗

(On Diff #82885)

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 ↗

(On Diff #82885)

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

[nalimilan]

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]

CMakeLists.txt

299 ↗

(On Diff #82885)

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]

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 ↗

(On Diff #86946)

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

312 ↗

(On Diff #86946)

This one too.

[nalimilan]

Done.

[nalimilan]

Bump. I have rebased against current master.

[nalimilan]

Re-bump. ;-)

[nalimilan]

Re-re-bump!

[mgorny]

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).

[nalimilan]

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?

[mgorny]

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?

[krytarowski]

In D28234#1337321, @nalimilan wrote:

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.

[nalimilan]

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...).

[krytarowski]

In D28234#1337338, @nalimilan wrote:

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.

[Ericson2314]

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?

[lebedev.ri]

In D28234#2532970, @Ericson2314 wrote:

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

[Ericson2314]

Rebase to latest main

[Ericson2314]

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.

[Ericson2314]

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.