Summary: | [CMake] Improve handling of LIB_INSTALL_DIR, EXEC_INSTALL_DIR, and LIBEXEC_INSTALL_DIR | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||||||||
Component: | WebKitGTK | Assignee: | Martin Robinson <mrobinson> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | bunhere, cdumez, commit-queue, gustavo, gyuyoung.kim, mrobinson, rakuco, sergio | ||||||||||||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2014-05-12 06:08:00 PDT
Created attachment 231290 [details] Patch With this patch, when cmake install works (see bug #130188) everything is correctly installed. 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.
Created attachment 231292 [details]
Fix cmake (inconsistent) style
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?
(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. (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. (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. (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. 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. Created attachment 231332 [details]
Patch
Created attachment 231337 [details]
Fix ChangeLog
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? We need an EFL reviwer for the EFL parts and to confirm they are fine with the new approach. Created attachment 231375 [details]
Patch
(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. 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? Created attachment 231475 [details]
Patch
(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. Comment on attachment 231475 [details]
Patch
Yes, this works, thanks! Please confirm with EFL guys they are fine with the changes before landing.
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 :-)
Created attachment 231732 [details]
Address suggestions
(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. 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? 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@. (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. (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. Created attachment 231832 [details]
Fix typo and relative default values
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 :-) (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. Committed r169165: <http://trac.webkit.org/changeset/169165> |