[webkit-reviews] review denied: [Bug 132819] [CMake] Improve handling of LIB_INSTALL_DIR, EXEC_INSTALL_DIR, and LIBEXEC_INSTALL_DIR : [Attachment 231337] Fix ChangeLog

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 132819: [CMake] Improve handling of LIB_INSTALL_DIR, EXEC_INSTALL_DIR, and
LIBEXEC_INSTALL_DIR
https://bugs.webkit.org/show_bug.cgi?id=132819

Attachment 231337: Fix ChangeLog
https://bugs.webkit.org/attachment.cgi?id=231337&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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?