Re: [Buildroot] [PATCH v2] package/qt5/qt5webkit: fix generated artifacts

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Thomas, All,

Sorry for the awfully long delay in looking at this patch; it has all
the words that make it scary to review: qt5. cmake. webkit...

On 2022-09-29 14:13 -0400, Thomas Ballasi spake thusly:
> Generated artifacts of the installation process were wrongly located,
> causing packages using qt5webkit (qt-webkit-kiosk and python-pyqt5) to
> fail at build time.

It would have been good to provide an example of such a failure, and
they are pretty rare:

    http://autobuild.buildroot.org/?reason=qt-webkit-kiosk%

The last one is from 2023-09-04, pretty recent, but the one before that
was in 2022-12-14, but it was more common before that (a few a month on
average).

We usually add a reference to such a build failure directly in the
commit log:

    http://autobuild.buildroot.org/results/91a/91a2d87eb42bf62f8d4f2b24788deef6c5e866f6/

> Firstly, *.h files are wrongly located a directory below where supposed
> (inside qt5/ directory). This is 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.

I don't see where/how you are disabling the use of DATADIR.

> Secondly, in order to locate *.pri artifacts correctly, we set the conf
> option CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT on,

From the cmake documentation, it looks like that variable i set by
cmake, and is not to be set manually like you do:

    https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT.html

qt5webkit looks for the value of CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT
to set CMAKE_INSTALL_PREFIX:

    qt5webkit-5.212.0-alpha4/Source/cmake/OptionsQt.cmake
      1004 query_qmake(qt_install_prefix_dir QT_INSTALL_PREFIX)
      1005 if (CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
      1006     set(CMAKE_INSTALL_PREFIX "${qt_install_prefix_dir}" CACHE PATH "Install path prefix, prepended onto install›
      1007 endif ()

And this looks to me very close to what 

Looking a bit around, I only noticed jpeg-turbo doing something similar,
but I don't understand why it looks if the path s set to its efault
value to just then set it to its default value...

I think the proper solution in pur case would be to drop that
conditional block...

> which in turn
> sets the variable KDE_INSTALL_USE_QT_SYS_PATHS on, for the following
> reasons:
> 
> 1. *.pri files are 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 done previously, as it enforced to install
>    artifacts under the sysroot directory.
> 
> 2. *.pri files' content have hardcoded include and library paths. This
>    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).

I think both issues should be fixed by removing that conditional
block...

[--SNIP--]
> diff --git a/package/qt5/qt5webkit/qt5webkit.mk b/package/qt5/qt5webkit/qt5webkit.mk
> index 6912359674..8310ef20c8 100644
> --- a/package/qt5/qt5webkit/qt5webkit.mk
> +++ b/package/qt5/qt5webkit/qt5webkit.mk
> @@ -51,10 +51,18 @@ QT5WEBKIT_CONF_OPTS += -DENABLE_SAMPLING_PROFILER=OFF
>  endif
>  
>  QT5WEBKIT_CONF_OPTS += \
> +	-DCMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT=ON \
>  	-DENABLE_TOOLS=OFF \
>  	-DPORT=Qt \
>  	-DPYTHON_EXECUTABLE=$(HOST_DIR)/bin/python3 \
>  	-DSHARED_CORE=ON \
>  	-DUSE_LIBHYPHEN=OFF
>  
> +QT5WEBKIT_INSTALL_STAGING_OPTS = install/fast

The default _INSTALL_STAGING_OPTS contains DESTDIR and isntall/fast:

    https://gitlab.com/buildroot.org/buildroot/-/blob/68b68518a8cc372cd6bfb34414a91311e7266044/package/pkg-cmake.mk#L56

So what you're doing is to rem ove setting DESTDIR, which feels horribly
wrong.

> +define QT5WEBKIT_INSTALL_TARGET_CMDS
> +	$(TARGET_MAKE_ENV) $(BR2_CMAKE) --install $(QT5WEBKIT_BUILDDIR) \
> +		--prefix $(TARGET_DIR)/usr

Ditto, not using DESTDIR as the install step for target looks highly
suspicious.

Note that the current state of affairs with cmake has slightly evolved
since you posted your patch. as we recently merged support for using
ninja as the backend (it is opt-in to that backend, the default is still
Makefiles), and we may have a bit of a fallout. Still, I was able to
reproduce the issue...

So, I started a build (wee, 3 hours!) that have removed the conditional
block in qt5webkit, let's see what that does...

Regards,
Yann E. MORIN.

> +endef
> +
>  $(eval $(cmake-package))

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot