package/qt5/qt5webkit: fix generated artifacts

Generated artifacts of the installation process were wrongly located,
causing packages using qt5webkit (qt-webkit-kiosk and python-pyqt5) to
fail at build time. The changes aims at fixing this issue.

There were three main issues occuring during the build:

1. *.pri files were wrongly located in the host's and target's sysroot
   directores while buildroot implements its own mkspecs directory.
   By setting KDE_INSTALL_USE_QT_SYS_PATHS on, mkspecs modules are now
   being installed in host's data dir (ECM_MKSPECS_INSTALL_DIR is set
   accordingly @ line 102 in Source/cmake/ECMGeneratePriFile.cmake).
   This also required to prevent using the CMake package's default
   DATADIR variable as it enforces to install under the sysroot
   directory.

2. *.pri files' content had hardcoded include and library paths which
   has been corrected by setting on KDE_INSTALL_USE_QT_SYS_PATHS as
   their content is written according to this value (see line 514 and
   739 in file Source/WebKit/PlatformQt.cmake).

3. *.h files were located a directory below where supposed (inside qt5/
   directory). This was caused by using DATADIR which assumed include
   files were to be located in sysroot/usr/include/.
   Disabling this variable by removing it from build options leads to a
   correct behavior.

Regression happened when qt5webkit started using cmake-package at commit
df0b0fe6919c0d0f3750f439a3cfa765232bd569.

More info @ https://bugs.buildroot.org/show_bug.cgi?id=14606

Signed-off-by: Thomas Ballasi <thomas.ballasi@savoirfairelinux.com>
---
 ...-set-KDE_INSTALL_USE_QT_SYS_PATHS-on.patch | 42 +++++++++++++++++++
 package/qt5/qt5webkit/qt5webkit.mk            |  7 ++++
 2 files changed, 49 insertions(+)
 create mode 100644 package/qt5/qt5webkit/0007-cmake-set-KDE_INSTALL_USE_QT_SYS_PATHS-on.patch

Hi Thomas,

On 22/09/22 23:45, Thomas Ballasi wrote:
> Generated artifacts of the installation process were wrongly located,
> causing packages using qt5webkit (qt-webkit-kiosk and python-pyqt5) to
> fail at build time. The changes aims at fixing this issue.

"The changes aims at fixing this issue." should be:
"Let's add a patch that:"

and then you list the points below using the present verb

> There were three main issues occuring during the build:
> 
> 1. *.pri files were wrongly located in the host's and target's sysroot
>     directores while buildroot implements its own mkspecs directory.
>     By setting KDE_INSTALL_USE_QT_SYS_PATHS on, mkspecs modules are now
>     being installed in host's data dir (ECM_MKSPECS_INSTALL_DIR is set
>     accordingly @ line 102 in Source/cmake/ECMGeneratePriFile.cmake).
>     This also required to prevent using the CMake package's default
>     DATADIR variable as it enforces to install under the sysroot
>     directory.
> 
> 2. *.pri files' content had hardcoded include and library paths which
>     has been corrected by setting on KDE_INSTALL_USE_QT_SYS_PATHS as
>     their content is written according to this value (see line 514 and
>     739 in file Source/WebKit/PlatformQt.cmake).
> 
> 3. *.h files were located a directory below where supposed (inside qt5/
>     directory). This was caused by using DATADIR which assumed include
>     files were to be located in sysroot/usr/include/.
>     Disabling this variable by removing it from build options leads to a
>     correct behavior.
> 
> Regression happened when qt5webkit started using cmake-package at commit
> df0b0fe6919c0d0f3750f439a3cfa765232bd569.

What is the upstream status of this patch? Can you point here the URL
of the pending patch?

> More info @ https://bugs.buildroot.org/show_bug.cgi?id=14606

Here ^^^ it should be:
Fixes:
https://bugs.buildroot.org/show_bug.cgi?id=14606

> Signed-off-by: Thomas Ballasi <thomas.ballasi@savoirfairelinux.com>
> ---
>   ...-set-KDE_INSTALL_USE_QT_SYS_PATHS-on.patch | 42 +++++++++++++++++++
>   package/qt5/qt5webkit/qt5webkit.mk            |  7 ++++
>   2 files changed, 49 insertions(+)
>   create mode 100644 package/qt5/qt5webkit/0007-cmake-set-KDE_INSTALL_USE_QT_SYS_PATHS-on.patch
> 
> diff --git a/package/qt5/qt5webkit/0007-cmake-set-KDE_INSTALL_USE_QT_SYS_PATHS-on.patch b/package/qt5/qt5webkit/0007-cmake-set-KDE_INSTALL_USE_QT_SYS_PATHS-on.patch
> new file mode 100644
> index 0000000000..b65eb305b4
> --- /dev/null
> +++ b/package/qt5/qt5webkit/0007-cmake-set-KDE_INSTALL_USE_QT_SYS_PATHS-on.patch
> @@ -0,0 +1,42 @@
> +From f4950219005b487c18d41ce4e6bc11c4b0e3a20d Mon Sep 17 00:00:00 2001
> +From: Thomas Ballasi <thomas.ballasi@savoirfairelinux.com>
> +Date: Wed, 21 Sep 2022 14:46:40 -0400
> +Subject: [PATCH] cmake: set KDE_INSTALL_USE_QT_SYS_PATHS on
> +
> +This variable is used to save .pri files to directories relative to the
> +host (output/host/mkspecs/modules/) rather than relative to the target
> +itself, which is unwanted behavior.
> +
> +The changes also enables .pri files not to hardcode include and library
> +paths and to use $$QT_MODULE_INCLUDE_BASE and $$QT_MODULE_LIB_BASE.
> +
> +Signed-off-by: Thomas Ballasi <thomas.ballasi@savoirfairelinux.com>

Also here please add:
'[Upstream status: URL of this pending patch]

> +---
> + Source/cmake/OptionsQt.cmake | 8 +-------
> + 1 file changed, 1 insertion(+), 7 deletions(-)
> +
> +diff --git a/Source/cmake/OptionsQt.cmake b/Source/cmake/OptionsQt.cmake
> +index 1ee60b777106..607c69bd38fe 100644
> +--- a/Source/cmake/OptionsQt.cmake
> ++++ b/Source/cmake/OptionsQt.cmake
> +@@ -998,16 +998,10 @@ feature_summary(WHAT ALL FATAL_ON_MISSING_REQUIRED_PACKAGES)
> + include(ECMQueryQmake)
> +
> + query_qmake(qt_install_prefix_dir QT_INSTALL_PREFIX)
> +-if (CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
> +-    set(CMAKE_INSTALL_PREFIX "${qt_install_prefix_dir}" CACHE PATH "Install path prefix, prepended onto install directories." FORCE)
> +-endif ()
> ++set(CMAKE_INSTALL_PREFIX "${qt_install_prefix_dir}")
> +
> + include(KDEInstallDirs)
> +
> +-if (NOT qt_install_prefix_dir STREQUAL "${CMAKE_INSTALL_PREFIX}")
> +-    set(KDE_INSTALL_USE_QT_SYS_PATHS OFF)
> +-endif ()
> +-
> + # We split all installed files into 2 components: Code and Data. This is different from
> + # traditional approach with Runtime and Devel, but we need it to fix concurrent installation of
> + # debug and release builds in qmake-based build
> +--
> +2.25.1
> +
> diff --git a/package/qt5/qt5webkit/qt5webkit.mk b/package/qt5/qt5webkit/qt5webkit.mk
> index 6912359674..607c022568 100644
> --- a/package/qt5/qt5webkit/qt5webkit.mk
> +++ b/package/qt5/qt5webkit/qt5webkit.mk
> @@ -57,4 +57,11 @@ QT5WEBKIT_CONF_OPTS += \
>   	-DSHARED_CORE=ON \
>   	-DUSE_LIBHYPHEN=OFF
>   
> +QT5WEBKIT_INSTALL_STAGING_OPTS = install/fast
> +
> +define QT5WEBKIT_INSTALL_TARGET_CMDS
> +	$(TARGET_MAKE_ENV) $(BR2_CMAKE) --install $(QT5WEBKIT_BUILDDIR) \
> +		--prefix $(TARGET_DIR)/usr
> +endef
> +
>   $(eval $(cmake-package))

The patch works correctly, so with commit log improve and the local 
patch with Upstream status pointed:
Reviewed-by: Giulio Benetti <giulio.benetti@benettiengineering.com>

Thanks for contributing!

Best regards

Hello Giulio,

On Thu, 29 Sep 2022 12:43:22 +0200
Giulio Benetti <giulio.benetti@benettiengineering.com> wrote:
> > Generated artifacts of the installation process were wrongly
> > located, causing packages using qt5webkit (qt-webkit-kiosk and
> > python-pyqt5) to fail at build time. The changes aims at fixing
> > this issue.
> 
> "The changes aims at fixing this issue." should be:
> "Let's add a patch that:"

This isn't necessarily true. While the patch answers points 1 and 2,
the third one isn't really affected by it. I will try to rephrase the
message for more clarification as the way I phrased it is ambiguous,
thanks.

> What is the upstream status of this patch? Can you point here the URL
> of the pending patch?

This patch isn't pending for merge in the qt5webkit repository for the
reason that it may break other projects. At first, I made this patch to
counter the use of CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT, which
CMake's documentation seems to imply that it cannot be set by the user
but is set automatically by CMake, which leads to the unwanted
behavior. The patch makes it as if it was ON specifically for buildroot.

I reckon that it may be bad practice to create such patches, so I just
checked whether or not this variable can be set via CONF_OPTS and it
seems it can, leading to the same working behavior (it also doesn't seem
to be set in CMakeCache by CMake if not manually added).

I also made sure setting this variable doesn't have any potential side
effects as it is only used once in the case inside the patch.

Therefore, it seems to be a better fit to set INITIALIZED_TO_DEFAULT's
value in CONF_OPTS rather than bypassing its usage inside a patch. This
has been replaced.

> > More info @ https://bugs.buildroot.org/show_bug.cgi?id=14606
> 
> Here ^^^ it should be:
> Fixes:
> https://bugs.buildroot.org/show_bug.cgi?id=14606

Done!

> Also here please add:
> '[Upstream status: URL of this pending patch]

Patch is replaced, ignoring.

> The patch works correctly, so with commit log improve and the local 
> patch with Upstream status pointed:
> Reviewed-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
> 
> Thanks for contributing!
> 
> Best regards

Thanks a lot for the constructive review! I will send a v2 asap.

Kind regards,
Thomas Ballasi