Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review wubkat scripts #180

Merged
merged 3 commits into from
Aug 23, 2022
Merged

Conversation

dpino
Copy link
Contributor

@dpino dpino commented Aug 22, 2022

Following up on WebKit/WebKit#1323 (comment)

This PR is not neccesary meant to be merged.

First thing is flatpak is not actually needed. This is a wrong understanding on my side about how things work. Before digging into this let me explain what are the 3 different ways of building WebKitGTK/WPE:

  • Using Flatpak SDK. This is the most common way of building WebKitGTK/WPE and it's what most building bots use. Take for instance this build from the GTK-Linux-64-bit-Release-Build. Step 'jhbuild' runs the Tools/Scripts/update-webkitgtk-libs script, which by default uses the Flatpak SDK (notice env var WEBKIT_JHBUILD=0). The Flatpak SDK is a way of building WebKitGTK/WPE inside a container that features all needed dependencies. This container is prebuilt for x86-64. The dependencies are listed in .bst files like for instance Tools/buildstream/elements/sdk/libsoup3.bst. Flatpak SDK can even feature updated versions of GCC or clang not available in the host OS. The advantage of building using the Flatpak SDK is that everyone gets the same results regardless of their host system. In other words, building with Flatpak SDK is completely the opposite to building using system libraries.
  • Using system libraries + JHBuild. Until Flatpak, this was the most common way of building WebKitGTK/WPE and what the build bots used. It can be undestood as trying to satisfy dependencies through system libraries but when those dependencies cannot be satisfied those libraries should be provided through JHBuild. The JHBuild provided dependencies are defined at Tools/gtk/jhbuild.modules. Since Flapak became the default way of building WebKitGTK/WPE the dependencies listed in jhbuild.modules are not so well maintained.
  • Using system libraries only. This is another way of building WebKitGTK/WPE. Basically it means to not run the script Tools/Scripts/update-webkitgtk-libs and simply go ahead and build WebKit. This is actually what the Ubuntu LTS & Debian Stable build bots do, as it can be seen in this build. Notice there is not jhbuild step (the step that installs library dependencies). How does it work? By default, these bots build with the flag --no-experimetal-features, which is equivalent to set all the features listed WebPreferencesExperimental.yaml to disable. Since no experimental features are used is likely everything will build OK using system libraries.

I can explain now why flatpak is not actually needed. The current build script builds WebKitGTK using cmake directly with CC set to clang. My understanding is that this the right way to build if we want mozsearch's clang-plugin to step in and get the code indexed. When I used Tools/Script/build-webkit to build instead, I didn't see the clang-plugin being used. And since the script builds using cmake directly and not the tooling provided in the WebKit code base, I think neither Flatpak SDK is used neither the dependencies provided through JHBuild would be used.

Another conclusion is that building with the --no-experimental-features flag would make the mozsearch builds more resilient and less likely to break. The main disadvantage is that less code would get indexed, specially features that are currently being developed.

I've been discussing with some colleagues what would be the best solution to make mozsearch builds more resilient. We think adding mozsearch's clang-plugin in the Flatpak SDK would alllow to build using flatpak, which will make the builds very unlikely to fail since Flatpak SDK is what the build bots use by default.

About the gi-docgen hack, an alternative way to solve it would have been to build using -DENABLE_DOCUMENTATION=OFF. I think Ubuntu 22.04 features the right version of gi-docgen so this dependency can now be satisfied through system libraries (gi-docgen should be added to Tools/gtk/dependencies/apt). Something similar happens -DENABLE_GAMEPAD and -DUSE_LIBSOUP2=ON. Gamepad support is provided through the library libmanette-0.2-dev and libsoup3 support (default version now) is provided through library libsoup-3.0-dev. Both libraries are featured in Ubuntu 22.04 and should be added to Tools/gtk/dependencies/apt.

I'm going to open a PR to fix the error you pointed in WebKit/WebKit#1323 (comment) and also add the missing system dependencies in Tools/Scripts/gtk/apt.

In WebKitGTK gamepad support if provided by the library 'libmanette-0.2-dev'. This dependency is
provided through Flatpak SDK and JHBuild. However, Ubuntu 22.04 features the required version
of this package so it should be safe to add it to 'gtk/dependencies/atk'.
By default, WebKitGTK uses libsoup-3.0. This dependency is provided through Flatpak SDK and JHBuild.
The build flag 'USE_SOUP2=ON' was useful to build WebKitGTK using system libraries only in OSes
which didn't feature yet libsoup-3.0 (i.e Ubuntu 18.04). Ubuntu 22.04 features 'libsoup-3.0' so
'USE_SOUP2=ON' is not needed if package 'libsoup-3.0-dev' is installed in the system (currently is
not listed at 'gtk/dependencies/atk').
@asutherland
Copy link
Contributor

Thanks for getting the appIfExists fix in so quickly! The wubkat build got about 44% through today before it broke on:

[ 44%] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp.o
MOZSEARCH: /mnt/index-scratch/wubkat/git /mnt/index-scratch/wubkat/analysis/ /mnt/index-scratch/wubkat/objdir/
[ 44%] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/platform/audio/gstreamer/FFTFrameGStreamer.cpp.o
/mnt/index-scratch/wubkat/git/Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:256:22: error: use of undeclared identifier 'm_pipeline'
    GST_DEBUG_OBJECT(m_pipeline.get(), "Setting up client %p (previous: %p)", newClient, client());
                     ^
/mnt/index-scratch/wubkat/git/Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:344:22: error: use of undeclared identifier 'm_pipeline'
    GST_DEBUG_OBJECT(m_pipeline.get(), "New pad %" GST_PTR_FORMAT, pad);
                     ^
MOZSEARCH: /mnt/index-scratch/wubkat/git /mnt/index-scratch/wubkat/analysis/ /mnt/index-scratch/wubkat/objdir/

With any luck m_pipeline is conditional on an ifdef that your changes here will help with!

I should be able to take a look at this PR a few hours later today.

Copy link
Contributor

@asutherland asutherland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the in-depth context about the build mechanisms! As was probably obvious, what I did was a mash-up of https://trac.webkit.org/wiki/BuildingGtk and what I found in your scripts. It's great to have a better idea of how all of this works now!

One thing to note for anyone reading along is that currently we are leaving the flatpaks in the "wubkat" lz4 tarball. We could manually add an idempotent removal step here, but I think given the initial sync time for these and that it sounds like the flatpak might become an option in the future for improved stability, I think it makes sense to leave those around for at least a few months in terms of the marginal costs at play (S3 storage costs versus human time required to re-establish their presence in the face of our failsafe timeouts, etc.)

@asutherland
Copy link
Contributor

@dpino One other stability thing that you might want to consider is whether it's practical to add jobs to webkit's CI system that can be responsible for running clang with the mozsearch indexer plugin active and then publish a resulting compressed archive of the resulting analysis files.

This is how Firefox's mozilla-central (m-c) indexing works for searchfox. This historically helped with searchfox's m-c stability because the decision of which revision to make was based on the "latest" linux64 searchfox build artifact published to taskcluster. If the build was broken, we'd just use the previous build that had valid data. We've changed this up a bit now to instead pick a revision based on available code coverage, but the idea is the same.

This approach also makes it somewhat more straightforward for the m-c tree's support of having analysis data for all of linux64, win64, osx64, and arm(64?) since we just download their analysis files and run merge-analyses on them. I think in practice all of the jobs are cross-compiled so they may all run on our linux64 builders, so in theory you could have a single machine run all four builds, but by having CI atomically do each build independently things are much saner and the searchfox taskcluster jobs are almost verbatim derived from the normal builders so anyone changing the builders can also easily change the searchfox jobs too.

@asutherland asutherland merged commit e913aee into mozsearch:master Aug 23, 2022
@dpino
Copy link
Contributor Author

dpino commented Aug 24, 2022

Thanks for getting the appIfExists fix in so quickly! The wubkat build got about 44% through today before it broke on:

[ 44%] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp.o
MOZSEARCH: /mnt/index-scratch/wubkat/git /mnt/index-scratch/wubkat/analysis/ /mnt/index-scratch/wubkat/objdir/
[ 44%] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/platform/audio/gstreamer/FFTFrameGStreamer.cpp.o
/mnt/index-scratch/wubkat/git/Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:256:22: error: use of undeclared identifier 'm_pipeline'
    GST_DEBUG_OBJECT(m_pipeline.get(), "Setting up client %p (previous: %p)", newClient, client());
                     ^
/mnt/index-scratch/wubkat/git/Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:344:22: error: use of undeclared identifier 'm_pipeline'
    GST_DEBUG_OBJECT(m_pipeline.get(), "New pad %" GST_PTR_FORMAT, pad);
                     ^
MOZSEARCH: /mnt/index-scratch/wubkat/git /mnt/index-scratch/wubkat/analysis/ /mnt/index-scratch/wubkat/objdir/

This regression broke the WebKitGTK Ubuntu LTS bot yesterday. It's already fixed.

The WebKitGTK Ubuntu LTS (Ubuntu 22.04) bot is the best bot to observe in case there's a build failure in WebKit mozsearch. It builds with the --no-experimental-features flag, but it's still the closest bot to how WebKit is built in mozsearch..

One potential fix to prevent building a commit that may fail is to query the last successful build from the WebKitGTK Ubuntu LTS bot and checkout the corresponding commit before starting building. I attempted to do that some time ago when I started with WebKitSearch but I didn't manage to get it working. I wrote a script on how to get the corresponding git commit of the last successful build in WebKitGTK Ubuntu LTS bot.

The idea of deploying a build bot for WebKit mozsearch, which can also generate some of the reusable artifacts like analysis index, etc, sounds very interesting. Likely I will explore this idea further.

1 similar comment
@dpino
Copy link
Contributor Author

dpino commented Aug 24, 2022

Thanks for getting the appIfExists fix in so quickly! The wubkat build got about 44% through today before it broke on:

[ 44%] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp.o
MOZSEARCH: /mnt/index-scratch/wubkat/git /mnt/index-scratch/wubkat/analysis/ /mnt/index-scratch/wubkat/objdir/
[ 44%] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/platform/audio/gstreamer/FFTFrameGStreamer.cpp.o
/mnt/index-scratch/wubkat/git/Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:256:22: error: use of undeclared identifier 'm_pipeline'
    GST_DEBUG_OBJECT(m_pipeline.get(), "Setting up client %p (previous: %p)", newClient, client());
                     ^
/mnt/index-scratch/wubkat/git/Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:344:22: error: use of undeclared identifier 'm_pipeline'
    GST_DEBUG_OBJECT(m_pipeline.get(), "New pad %" GST_PTR_FORMAT, pad);
                     ^
MOZSEARCH: /mnt/index-scratch/wubkat/git /mnt/index-scratch/wubkat/analysis/ /mnt/index-scratch/wubkat/objdir/

This regression broke the WebKitGTK Ubuntu LTS bot yesterday. It's already fixed.

The WebKitGTK Ubuntu LTS (Ubuntu 22.04) bot is the best bot to observe in case there's a build failure in WebKit mozsearch. It builds with the --no-experimental-features flag, but it's still the closest bot to how WebKit is built in mozsearch..

One potential fix to prevent building a commit that may fail is to query the last successful build from the WebKitGTK Ubuntu LTS bot and checkout the corresponding commit before starting building. I attempted to do that some time ago when I started with WebKitSearch but I didn't manage to get it working. I wrote a script on how to get the corresponding git commit of the last successful build in WebKitGTK Ubuntu LTS bot.

The idea of deploying a build bot for WebKit mozsearch, which can also generate some of the reusable artifacts like analysis index, etc, sounds very interesting. Likely I will explore this idea further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants