Bug 229152

Summary: REGRESSION(r280382): [GTK] 2.33.3 does not build with gtk-doc enabled, installs broken pkg-config files
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, berto, bugs-noreply, cgarcia, ews-watchlist, gyuyoung.kim, keith_miller, mark.lam, mcatanzaro, msaboff, pnormand, ryuan.choi, sbarati, sergio, tzagallo
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch for downstreams
none
Patch for downstreams (which actually works this time) none

Description Michael Catanzaro 2021-08-16 10:43:14 PDT
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.
Comment 1 Michael Catanzaro 2021-08-16 15:19:28 PDT
(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....
Comment 2 Carlos Garcia Campos 2021-08-16 23:31:05 PDT
And Berto reported the same error in debian.
Comment 3 Alberto Garcia 2021-08-17 02:17:43 PDT
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
Comment 4 Michael Catanzaro 2021-08-17 06:45:20 PDT
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"
Comment 5 Philippe Normand 2021-08-17 06:59:19 PDT
(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 ()
Comment 6 Michael Catanzaro 2021-08-17 07:58:15 PDT
(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
Comment 7 Michael Catanzaro 2021-08-17 07:58:55 PDT
(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")
Comment 8 Michael Catanzaro 2021-08-17 08:17:14 PDT
(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.
Comment 9 Philippe Normand 2021-08-17 08:40:00 PDT
(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 ${..} ?
Comment 10 Michael Catanzaro 2021-08-17 08:42:57 PDT
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@.
Comment 11 Michael Catanzaro 2021-08-17 08:51:56 PDT
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?
Comment 12 Philippe Normand 2021-08-17 09:08:46 PDT
(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.
Comment 13 Philippe Normand 2021-08-17 09:11:34 PDT
Another issue is that the python script is not included in tarballs, initially it was meant only for developer builds.
Comment 14 Michael Catanzaro 2021-08-17 12:42:04 PDT
(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.
Comment 15 Michael Catanzaro 2021-08-17 12:48:24 PDT
Created attachment 435702 [details]
Patch
Comment 16 Philippe Normand 2021-08-17 12:53:05 PDT
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?
Comment 17 Michael Catanzaro 2021-08-17 13:14:57 PDT
Created attachment 435705 [details]
Patch for downstreams
Comment 18 Michael Catanzaro 2021-08-17 13:18:10 PDT
Created attachment 435706 [details]
Patch for downstreams (which actually works this time)
Comment 19 Michael Catanzaro 2021-08-17 13:24:46 PDT
(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.
Comment 20 Philippe Normand 2021-08-18 05:50:34 PDT
(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.
Comment 21 Alberto Garcia 2021-08-18 07:18:48 PDT
(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!
Comment 22 EWS 2021-08-18 09:46:41 PDT
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].