Bug 229152 – REGRESSION(r280382): [GTK] 2.33.3 does not build with gtk-doc enabled, installs broken pkg-config files

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

2.33.3 does not build with -DENABLE_GTK_DOC=ON:

jsc-glib-4.0-scan.c:34: error: undefined reference to 'jsc_class_get_type'
jsc-glib-4.0-scan.c:35: error: undefined reference to 'jsc_context_get_type'
jsc-glib-4.0-scan.c:36: error: undefined reference to 'jsc_exception_get_type'
jsc-glib-4.0-scan.c:37: error: undefined reference to 'jsc_value_get_type'
jsc-glib-4.0-scan.c:38: error: undefined reference to 'jsc_virtual_machine_get_type'
jsc-glib-4.0-scan.c:39: error: undefined reference to 'jsc_weak_value_get_type'

Example failures:

1. https://gnome.pages.gitlab.gnome.org/-/gnome-build-meta/-/jobs/1457787/artifacts/logs/gnome/sdk-webkitgtk/31ad2288-build.14200.log
2. https://kojipkgs.fedoraproject.org//work/tasks/3009/73963009/build.log

I don't immediately see what's wrong, so I'll probably wind up bisecting it, I suppose. Note that -DENABLE_DEVELOPER_MODE=ON (used by build-webkit) would hide this error since that exports everything.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> 2.33.3 does not build with -DENABLE_GTK_DOC=ON:

The right flag is -DENABLE_GTKDOC=ON. And for some reason my build from trunk did not fail, even though both GNOME and Fedora builds failed. I'll need to investigate harder....

[Carlos Garcia Campos]

And Berto reported the same error in debian.

[Alberto Garcia]

I made some preliminary investigation, this is the command line that fails:

   /usr/bin/cc jsc-glib-4.0-scan.o -L/tmp/webkit2gtk-2.33.3/build/lib -L -ljavascriptcoregtk-4.0 -lgobject-2.0 -lglib-2.0 -Wl,-z,relro -Wl,-z,now -fuse-ld=gold -Wl,--disable-new-dtags -o jsc-glib-4.0-scan

The problem seems to be the " -L " option with no value, if you remove that and run the command manually it builds. This comes from the .pc file:

   $ grep Libs /tmp/webkit2gtk-2.33.3/build/Source/JavaScriptCore/javascriptcoregtk-4.0.pc
   Libs: -L -ljavascriptcoregtk-4.0

This is -L${libdir} , so ${libdir} is empty

[Michael Catanzaro]

I haven't tested it yet, but I suspect it broke in r280382 "[WPE][GTK] SVN_REVISION drifting away if bots don't re-run cmake"

[Philippe Normand]

(In reply to Michael Catanzaro from comment #4)
> I haven't tested it yet, but I suspect it broke in r280382 "[WPE][GTK]
> SVN_REVISION drifting away if bots don't re-run cmake"

That patch didn't change anything related to LIB_INSTALL_DIR though.

OTOH LIB_INSTALL_DIR should be either lib or lib64 IIUC?

Why do we have this in OptionsCommon?

# GTK and WPE use the GNU installation directories as defaults.
if (NOT PORT STREQUAL "GTK" AND NOT PORT STREQUAL "WPE")
    set(LIB_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/lib" CACHE PATH "Absolute path to library installation directory")
    set(EXEC_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/bin" CACHE PATH "Absolute path to executable installation directory")
    set(LIBEXEC_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/bin" CACHE PATH "Absolute path to install executables executed by the library")
endif ()

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #4)
> I haven't tested it yet, but I suspect it broke in r280382 "[WPE][GTK]
> SVN_REVISION drifting away if bots don't re-run cmake"

Squinting closer at this commit, I no longer think it's to blame. Hmmm

[Michael Catanzaro]

(In reply to Philippe Normand from comment #5)
> Why do we have this in OptionsCommon?
> 
> # GTK and WPE use the GNU installation directories as defaults.
> if (NOT PORT STREQUAL "GTK" AND NOT PORT STREQUAL "WPE")
>     set(LIB_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/lib" CACHE PATH "Absolute
> path to library installation directory")
>     set(EXEC_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/bin" CACHE PATH "Absolute
> path to executable installation directory")
>     set(LIBEXEC_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}/bin" CACHE PATH
> "Absolute path to install executables executed by the library")
> endif ()

GTK and WPE ports are a bit smarter, see e.g. OptionsGTK.cmake:

# These are shared variables, but we special case their definition so that we can use the
# CMAKE_INSTALL_* variables that are populated by the GNUInstallDirs macro.
set(LIB_INSTALL_DIR "${CMAKE_INSTALL_FULL_LIBDIR}" CACHE PATH "Absolute path to library installation directory")
set(EXEC_INSTALL_DIR "${CMAKE_INSTALL_FULL_BINDIR}" CACHE PATH "Absolute path to executable installation directory")
set(LIBEXEC_INSTALL_DIR "${CMAKE_INSTALL_FULL_LIBEXECDIR}/webkit2gtk-${WEBKITGTK_API_VERSION}" CACHE PATH "Absolute path to install executables executed by the library")

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #6)
> Squinting closer at this commit, I no longer think it's to blame. Hmmm

Ah no, that's definitely it. Problem is you removed @ONLY from the configure_file() calls in order to allow CMake to replace ${BUILD_REVISION}, but now it also replaces the other placeholders like ${libdir} which we don't want. We'll need to switch back to using @ONLY.

[Philippe Normand]

(In reply to Michael Catanzaro from comment #8)
> (In reply to Michael Catanzaro from comment #6)
> > Squinting closer at this commit, I no longer think it's to blame. Hmmm
> 
> Ah no, that's definitely it. Problem is you removed @ONLY from the
> configure_file() calls in order to allow CMake to replace ${BUILD_REVISION},
> but now it also replaces the other placeholders like ${libdir} which we
> don't want. We'll need to switch back to using @ONLY.

Or use @..@ instead ${..} ?

[Michael Catanzaro]

Well we definitely need @ONLY because we do not want to expand the pkg-config variables like ${prefix} or ${libdir}. So yes, that means we'll need to switch from ${BUILD_REVISION} to @BUILD_REVISION@.

[Michael Catanzaro]

I think we'll need to run apply-build-revision-to-files.py first, since updating @BUILD_REVISION@ is the first step that needs to happen, *then* do CMake configure in a separate step. Yes? Because the way it is now, if we switch from ${BUILD_REVISION} to @BUILD_REVISION@, then CMake is going to replace @BUILD_REVISION@ before apply-build-revision-to-files.py runs, and then the python script will do nothing. So we'd need to switch from *.in suffix to *.in.in. Phil, do you agree?

[Philippe Normand]

(In reply to Michael Catanzaro from comment #11)
> I think we'll need to run apply-build-revision-to-files.py first, since
> updating @BUILD_REVISION@ is the first step that needs to happen, *then* do
> CMake configure in a separate step. Yes? Because the way it is now, if we
> switch from ${BUILD_REVISION} to @BUILD_REVISION@, then CMake is going to
> replace @BUILD_REVISION@ before apply-build-revision-to-files.py runs, and
> then the python script will do nothing. So we'd need to switch from *.in
> suffix to *.in.in. Phil, do you agree?

The python script actually has logic now to restore the revision=${BUILD_REVISION} template in already expanded .pc files.

[Philippe Normand]

Another issue is that the python script is not included in tarballs, initially it was meant only for developer builds.

[Michael Catanzaro]

(In reply to Philippe Normand from comment #12)
> The python script actually has logic now to restore the
> revision=${BUILD_REVISION} template in already expanded .pc files.

So in theory, all we need to do is switch back to @@ and everything ought to work. Yes? I'm going to attach a patch that does this. Unfortunately, my testing indicates this patch reintroduces bug #228290, although I'm not sure why. At least it should fix the build failures, though. Distros: you'll need to remove the changes to apply-build-revision-to-files.py because this file is not included in release tarballs.

[Michael Catanzaro]

Created attachment 435702 [details]
Patch

[Philippe Normand]

Comment on attachment 435702 [details]
Patch

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

> Source/JavaScriptCore/javascriptcoregtk.pc.in:5
> +revision=@BUILD_REVISION@

is this set in CMake now for tarballs? `set(BUILD_REVISION "tarball")` somewhere?

[Michael Catanzaro]

Created attachment 435705 [details]
Patch for downstreams

[Michael Catanzaro]

Created attachment 435706 [details]
Patch for downstreams (which actually works this time)

[Michael Catanzaro]

(In reply to Philippe Normand from comment #16)
> > Source/JavaScriptCore/javascriptcoregtk.pc.in:5
> > +revision=@BUILD_REVISION@
> 
> is this set in CMake now for tarballs? `set(BUILD_REVISION "tarball")`
> somewhere?

Yes, that is handled in OptionsGTK.cmake and OptionsWPE.cmake.

[Philippe Normand]

(In reply to Michael Catanzaro from comment #14)
> Unfortunately, my
> testing indicates this patch reintroduces bug #228290, although I'm not sure
> why. 

That's likely an orthogonal issue. Your patch shouldn't introduce change in behavior in dev builds, and actually fixes tarball builds, so let's get it in IMHO.

[Alberto Garcia]

(In reply to Michael Catanzaro from comment #18)
> Created attachment 435706 [details]
> Patch for downstreams (which actually works this time)

I just wanted to confirm that this fixed the build in Debian, thanks!

[EWS]

Committed r281192 (240636@main): <https://commits.webkit.org/240636@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435702 [details].