Bug 132819 – [CMake] Improve handling of LIB_INSTALL_DIR, EXEC_INSTALL_DIR, and LIBEXEC_INSTALL_DIR

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

I build webkit with -DLIB_INSTALL_DIR to set the libdir (as the equivalent to the --libdir configure option fo autotools). The libraries are installed in the correct library, since the install cmake command since to properly honor the passed LIB_INSTALL_DIR, but other commands using CMAKE_INSTALL_LIBDIR fail, because CMAKE_INSTALL_LIBDIR doesn't take the passed LIB_INSTALL_DIR into account. This affects the LIBDIR passed as compiler option and used to build the path of the builtin injected bundle lib and also the libdir used to build the pkg-config files.

[Carlos Garcia Campos]

Created attachment 231290 [details]
Patch

With this patch, when cmake install works (see bug #130188) everything is correctly installed.

[WebKit Commit Bot]

Attachment 231290 [details] did not pass style-queue:


ERROR: Source/cmake/OptionsGTK.cmake:9:  One space between command "endif" and its parentheses, should be "endif ("  [whitespace/parentheses] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 231292 [details]
Fix cmake (inconsistent) style

[Martin Robinson]

Comment on attachment 231292 [details]
Fix cmake (inconsistent) style

I don't see LIB_INSTALL_DIR anywhere in the CMake documentation. Why would one pass it to CMake?

[Martin Robinson]

(In reply to comment #4)
> (From update of attachment 231292 [details])
> I don't see LIB_INSTALL_DIR anywhere in the CMake documentation. Why would one pass it to CMake?

Oh, I see the description now. Hang on, I'll get a review.

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 231292 [details])
> I don't see LIB_INSTALL_DIR anywhere in the CMake documentation. Why would one pass it to CMake?

I think I found it in the jhbuild cmake code.

[Martin Robinson]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 231292 [details] [details])
> > I don't see LIB_INSTALL_DIR anywhere in the CMake documentation. Why would one pass it to CMake?
> 
> I think I found it in the jhbuild cmake code.

Looks like it is something that was added especially for the WebKit build. I wonder if the right thing to do here is for users to pass CMAKE_INSTALL_LIBDIR instead. We either need to override all the GNU variable's (CMAKE_INSTALL_FOO) manually or just rely on users passing them directly. Hard for me to say what is the right thing.

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (From update of attachment 231292 [details] [details] [details])
> > > I don't see LIB_INSTALL_DIR anywhere in the CMake documentation. Why would one pass it to CMake?
> > 
> > I think I found it in the jhbuild cmake code.
> 
> Looks like it is something that was added especially for the WebKit build.

No, I'm talking about the generic cmake support in jhbuild, it was added in 2010, see

https://git.gnome.org/browse/jhbuild/commit/?id=d665c74f984163e34f4ac2f5c273fd9cbf56e7af

> I wonder if the right thing to do here is for users to pass CMAKE_INSTALL_LIBDIR instead. We either need to override all the GNU variable's (CMAKE_INSTALL_FOO) manually or just rely on users passing them directly. Hard for me to say what is the right thing.

All other CMAKE_INSTALL_FOO variables seems to be correctly set by GNUInstallDirs, because they correctly combine the relative path with the CMAKE_INSTALL_PREFIX. But for the CMAKE_INSTALL_LIBDIR there's a special case to deal with lib vs lib64 and some other distro specific things. I'm not sure if it's possible to pass CMAKE_INSTALL_LIBDIR instead of LIB_INSTALL_DIR as cmakeargs, but I guess LIB_INSTALL_DIR exists for a reason. Even if change my build script to pass CMAKE_INSTALL_LIBDIR, tarballs won't work in jhbuild.

[Martin Robinson]

Okay. Retargeting this bug. LIB_INSTALL_DIR, EXEC_INSTALL_DIR, and LIBEXEC_INSTALL_DIR should be able to handle absolute paths and always take precedence over the variables from GNUInstallDirs.

[Martin Robinson]

Created attachment 231332 [details]
Patch

[Martin Robinson]

Created attachment 231337 [details]
Fix ChangeLog

[Carlos Garcia Campos]

Comment on attachment 231337 [details]
Fix ChangeLog

View in context: https://bugs.webkit.org/attachment.cgi?id=231337&action=review

This is not enough, we should also make sure we don't use CMAKE_INSTALL_FULL_[LIBDIR|BINDIR|LIBEXECDIR] variables and use LIB_INSTALL_DIR, EXEC_INSTALL_DIR and LIBEXEC_INSTALL_DIR instead. In Source/WebKit2/PlatformGTK.cmake we have:

add_definitions(-DLIBEXECDIR="${CMAKE_INSTALL_FULL_LIBEXECDIR}")
add_definitions(-DLIBDIR="${CMAKE_INSTALL_FULL_LIBDIR}")

We should use EXEC_INSTALL_DIR and LIB_INSTALL_DIR there.

> Source/JavaScriptCore/javascriptcoregtk.pc.in:2
> +exec_prefix=@CMAKE_INSTALL_PREFIX@

I prefer to use variables here, it makes the pc file easier to read and it's consistent with our autotools .pc files (and all others, I grepped exec_prefix in my system and *all* pc files use exec_prefix=${prefix}.

> Source/JavaScriptCore/javascriptcoregtk.pc.in:4
> +includedir=@CMAKE_INSTALL_PREFIX@/include

Same here, includedir=${prefix}/include

> Source/WebKit2/webkit2gtk-web-extension.pc.in:4
> +prefix=@CMAKE_INSTALL_PREFIX@
> +exec_prefix=@CMAKE_INSTALL_PREFIX@
> +libdir=@LIB_INSTALL_DIR@
> +includedir=@CMAKE_INSTALL_PREFIX@/include

Same comment here.

> Source/WebKit2/webkit2gtk.pc.in:4
> +prefix=@CMAKE_INSTALL_PREFIX@
> +exec_prefix=@CMAKE_INSTALL_PREFIX@
> +libdir=@LIB_INSTALL_DIR@
> +includedir=@CMAKE_INSTALL_PREFIX@/include

Ditto.

> Source/cmake/OptionsGTK.cmake:-109
> -# These are used to generate the pkg-config files.
> -set(prefix ${CMAKE_INSTALL_PREFIX})
> -set(exec_prefix ${CMAKE_INSTALL_PREFIX})
> -set(libdir "${prefix}/${CMAKE_INSTALL_LIBDIR}")
> -set(includedir "${prefix}/include")
> -set(VERSION ${PROJECT_VERSION})

Nice.

> CMakeLists.txt:150
> +get_filename_component(LIB_INSTALL_DIRNAME ${LIB_INSTALL_DIR} NAME_WE)
> +get_filename_component(EXEC_INSTALL_DIRNAME ${EXEC_INSTALL_DIR} NAME_WE)
> +set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LIB_INSTALL_DIRNAME})
> +set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LIB_INSTALL_DIRNAME})
> +set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${EXEC_INSTALL_DIRNAME})

I don't understand this change, because we are still overriding these in OptionsGTK.cmake no?

> ChangeLog:9
> +        * CMakeLists.txt: Set variables like CMAKE_ARCHIVE_OUTPUT_DIRECTORY after the parsing the platform-specific
> +        option files, so that GTK+ can read variables like LIB_INSTALL_DIR. Also set CMAKE_ARCHIVE_OUTPUT_DIRECTORY, etc

Is it because GTK+ needs to read those variables, or to ensure those variables are already set? What happens when set() is called multiple times for the same variable? Are these vales overriding the ones set in OptionsGTK.cmake?

[Carlos Garcia Campos]

We need an EFL reviwer for the EFL parts and to confirm they are fine with the new approach.

[Martin Robinson]

Created attachment 231375 [details]
Patch

[Martin Robinson]

(In reply to comment #12)
> (From update of attachment 231337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231337&action=review
> 

Thanks for the review.

> This is not enough, we should also make sure we don't use CMAKE_INSTALL_FULL_[LIBDIR|BINDIR|LIBEXECDIR] variables and use LIB_INSTALL_DIR, EXEC_INSTALL_DIR and LIBEXEC_INSTALL_DIR instead. In Source/WebKit2/PlatformGTK.cmake we have:
> 
> add_definitions(-DLIBEXECDIR="${CMAKE_INSTALL_FULL_LIBEXECDIR}")
> add_definitions(-DLIBDIR="${CMAKE_INSTALL_FULL_LIBDIR}")
> 
> We should use EXEC_INSTALL_DIR and LIB_INSTALL_DIR there.

Fixed.


> > Source/JavaScriptCore/javascriptcoregtk.pc.in:2
> > +exec_prefix=@CMAKE_INSTALL_PREFIX@
> 
> I prefer to use variables here, it makes the pc file easier to read and it's consistent with our autotools .pc files (and all others, I grepped exec_prefix in my system and *all* pc files use exec_prefix=${prefix}.

Sure. It's not a problem and it's a pretty harmless change to make.

> > CMakeLists.txt:150
> > +get_filename_component(LIB_INSTALL_DIRNAME ${LIB_INSTALL_DIR} NAME_WE)
> > +get_filename_component(EXEC_INSTALL_DIRNAME ${EXEC_INSTALL_DIR} NAME_WE)
> > +set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LIB_INSTALL_DIRNAME})
> > +set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LIB_INSTALL_DIRNAME})
> > +set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${EXEC_INSTALL_DIRNAME})
> 
> I don't understand this change, because we are still overriding these in OptionsGTK.cmake no?

In this case, I just forgot to remove the lines from OptionsGTK.cmake. set(...) just redefines the value.

[Carlos Garcia Campos]

Comment on attachment 231375 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231375&action=review

> CMakeLists.txt:150
> +get_filename_component(LIB_INSTALL_DIRNAME ${LIB_INSTALL_DIR} NAME_WE)
> +get_filename_component(EXEC_INSTALL_DIRNAME ${EXEC_INSTALL_DIR} NAME_WE)
> +set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LIB_INSTALL_DIRNAME})
> +set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LIB_INSTALL_DIRNAME})
> +set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${EXEC_INSTALL_DIRNAME})

I don't think this works for GTK. All our scripts assume bin and lib, I have my libraries in $prefix/lib64, so I'll end up with a lib64 dir in my $builddir, and our scripts will look for the libraries in a non existent lib dir. Or am I misunderstanding the patch?

[Martin Robinson]

Created attachment 231475 [details]
Patch

[Martin Robinson]

(In reply to comment #16)

> I don't think this works for GTK. All our scripts assume bin and lib, I have my libraries in $prefix/lib64, so I'll end up with a lib64 dir in my $builddir, and our scripts will look for the libraries in a non existent lib dir. Or am I misunderstanding the patch?


Oh nice catch. That should be fixed in the latest patch. I've moved these to the OptionsCommon.cmake and OptionsGTK.cmake files, the former protected by the conditional.

[Carlos Garcia Campos]

Comment on attachment 231475 [details]
Patch

Yes, this works, thanks! Please confirm with EFL guys they are fine with the changes before landing.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 231475 [details]
Patch

Would it be possible to:
- Unconditionally hardcode the value of CMAKE_{ARCHIVE,LIBRARY,RUNTIME}_OUTPUT_DIRECTORY.
- Only support absolute paths for LIB_INSTALL_DIR and friends.
- Refer to LIB_INSTALL_DIR instead of CMAKE_INSTALL_LIBDIR, EXEC_INSTALL_DIR instead of CMAKE_INSTALL_BINDIR etc when calling install() or in definitions.

It would allow us to avoid introducing MAKE_PATH_ABSOLUTE_RELATIVE_TO_INSTALLATION_PREFIX and fix Carlos' original problem, if I understood the situation correctly at this time of night :-)

[Martin Robinson]

Created attachment 231732 [details]
Address suggestions

[Martin Robinson]

(In reply to comment #20)

> Would it be possible to:
> - Unconditionally hardcode the value of CMAKE_{ARCHIVE,LIBRARY,RUNTIME}_OUTPUT_DIRECTORY.
> - Only support absolute paths for LIB_INSTALL_DIR and friends.

Okay.

> - Refer to LIB_INSTALL_DIR instead of CMAKE_INSTALL_LIBDIR, EXEC_INSTALL_DIR instead of CMAKE_INSTALL_BINDIR etc when calling install() or in definitions.

I double-checked, but I think I got them all in the previous patch.

[Carlos Garcia Campos]

Comment on attachment 231732 [details]
Address suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=231732&action=review

> Source/cmake/OptionsGTK.cmake:114
> +set(EXEC_INSTALL_DIR "${CMAKE_INSTALL_FULL_BINDIR}" CACHE PATH "Absolute path to executable intsallation directory")

intsallation -> installation

> CMakeLists.txt:142
>  include(OptionsCommon)
> -set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LIB_INSTALL_DIR})
> -set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LIB_INSTALL_DIR})
> -set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${EXEC_INSTALL_DIR})
> +set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib)
> +set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib)
> +set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)

Since this is common now, maybe it would be better to move to OptionsCommon?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 231732 [details]
Address suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=231732&action=review

> Source/cmake/OptionsCommon.cmake:57
> +    set(LIB_INSTALL_DIR "lib" CACHE PATH "Absolute path to library installation directory")
> +    set(EXEC_INSTALL_DIR "bin" CACHE PATH "Absolute path to executable intsallation directory")
> +    set(LIBEXEC_INSTALL_DIR "bin" CACHE PATH "Absolute path to install executables executed by the library")

Don't the defaults need to be absolute paths too then? If they do, you might need to edit EFL's .pc.in files as well, they have things like datadir=${prefix}/@DATA_INSTALL_DIR@.

[Martin Robinson]

(In reply to comment #23)
> (From update of attachment 231732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231732&action=review
> 
> > Source/cmake/OptionsGTK.cmake:114
> > +set(EXEC_INSTALL_DIR "${CMAKE_INSTALL_FULL_BINDIR}" CACHE PATH "Absolute path to executable intsallation directory")
> 
> intsallation -> installation

Thanks. I'll fix it.

> > CMakeLists.txt:142
> >  include(OptionsCommon)
> > -set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LIB_INSTALL_DIR})
> > -set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LIB_INSTALL_DIR})
> > -set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${EXEC_INSTALL_DIR})
> > +set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib)
> > +set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib)
> > +set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
> 
> Since this is common now, maybe it would be better to move to OptionsCommon?

It's true that it is common, but it isn't an option. :) There are other variables defined in this file.

[Martin Robinson]

(In reply to comment #24)
> (From update of attachment 231732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=231732&action=review
> 
> > Source/cmake/OptionsCommon.cmake:57
> > +    set(LIB_INSTALL_DIR "lib" CACHE PATH "Absolute path to library installation directory")
> > +    set(EXEC_INSTALL_DIR "bin" CACHE PATH "Absolute path to executable intsallation directory")
> > +    set(LIBEXEC_INSTALL_DIR "bin" CACHE PATH "Absolute path to install executables executed by the library")
> 
> Don't the defaults need to be absolute paths too then? If they do, you might need to edit EFL's .pc.in files as well, they have things like datadir=${prefix}/@DATA_INSTALL_DIR@.

Ooh! Nice catch. Not sure how I let that slip through. We don't need to modify the use of DATA_INSTALL_DIR, since it's still defined relatively to the installation root in OptionsEFL.cmake. I did go ahead and fix the hard-coded library installation paths for EFL though. This was apparently broken before.

[Martin Robinson]

Created attachment 231832 [details]
Fix typo and relative default values

[Carlos Garcia Campos]

Comment on attachment 231832 [details]
Fix typo and relative default values

View in context: https://bugs.webkit.org/attachment.cgi?id=231832&action=review

Please, land this if EFL guys have no objection

> Source/cmake/OptionsGTK.cmake:114
> +set(EXEC_INSTALL_DIR "${CMAKE_INSTALL_FULL_BINDIR}" CACHE PATH "Absolute path to executable instalation directory")

instalation -> installation :-)

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #26)
> We don't need to modify the use of DATA_INSTALL_DIR, since it's still defined relatively to the installation root in OptionsEFL.cmake.

Hmm indeed, it does some unnecessary steps and sets DATA_INSTALL_DIR as a relative directory.

The patch looks great to me, thanks for working on this.

[Martin Robinson]

Committed r169165: <http://trac.webkit.org/changeset/169165>