Bug 205658 – [GTK][WPE] Migrate to Flatpak-based dev SDK

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Philippe Normand]

With the patch I will upload we could progressively migrate away from jhbuild to flatpak. By default flatpak would be used and initially the bots would set the WEBKIT_JHBUILD env var to keep the existing setup. Then one by one we could switch the bots and EWS.

[Philippe Normand]

Flipping the switch for all bots in one go seems quite a challenge, hence the step-by-step idea...

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #0)
> With the patch I will upload we could progressively migrate away from
> jhbuild to flatpak. By default flatpak would be used and initially the bots
> would set the WEBKIT_JHBUILD env var to keep the existing setup. Then one by
> one we could switch the bots and EWS.

Please do the other way around.

Leave the default to be the current one (jhbuild), and use flatpak if a env var like WEBKIT_FLATPAK is set.

We then start setting this env var on the bots, one by one, and testing them.

Finally, if we get to the point were things work as expected with flatpak we make the default be flatpak.

[Philippe Normand]

(In reply to Carlos Alberto Lopez Perez from comment #2)
> (In reply to Philippe Normand from comment #0)
> > With the patch I will upload we could progressively migrate away from
> > jhbuild to flatpak. By default flatpak would be used and initially the bots
> > would set the WEBKIT_JHBUILD env var to keep the existing setup. Then one by
> > one we could switch the bots and EWS.
> 
> Please do the other way around.
> 
> Leave the default to be the current one (jhbuild), and use flatpak if a env
> var like WEBKIT_FLATPAK is set.
> 
> We then start setting this env var on the bots, one by one, and testing them.
> 
> Finally, if we get to the point were things work as expected with flatpak we
> make the default be flatpak.

Seems to be more work to me, I'd rather try as initially proposed?
By switching build-webkit by default initially we would get more feedback. People are lazy and might be reluctant to set an env var :)

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #3)
> (In reply to Carlos Alberto Lopez Perez from comment #2)
> > (In reply to Philippe Normand from comment #0)
> > > With the patch I will upload we could progressively migrate away from
> > > jhbuild to flatpak. By default flatpak would be used and initially the bots
> > > would set the WEBKIT_JHBUILD env var to keep the existing setup. Then one by
> > > one we could switch the bots and EWS.
> > 
> > Please do the other way around.
> > 
> > Leave the default to be the current one (jhbuild), and use flatpak if a env
> > var like WEBKIT_FLATPAK is set.
> > 
> > We then start setting this env var on the bots, one by one, and testing them.
> > 
> > Finally, if we get to the point were things work as expected with flatpak we
> > make the default be flatpak.
> 
> Seems to be more work to me, I'd rather try as initially proposed?
> By switching build-webkit by default initially we would get more feedback.
> People are lazy and might be reluctant to set an env var :)

I don't think its more work, but I buy your argument of getting more feedback as you suggest. So let's try as you propose.

What is the implementation idea? update-webkitgtk-libs to execute update-webkitgtk-flatpak if the environment variable is not set?

[Philippe Normand]

On a first run build-webkit will bootstrap everything itself :)
But yeah right, I need to check the update-*flatpak scripts indeed, they would update the flatpak user-tree (run some `flatpak update` on it).

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #5)
> On a first run build-webkit will bootstrap everything itself :)

I think that's not ok.

build-webkit should only use the jhbuild or the flatpak if it detects that the jhbuild or flatpak has been previously built (Check shouldUseJhbuild() and shouldUseFlatpak() in Tools/Scripts/webkitdirs.pm)

If the user (or the bot) its not using the internal jhbuild, then build-webkit should continue building without it (or without flatpak).


> But yeah right, I need to check the update-*flatpak scripts indeed, they
> would update the flatpak user-tree (run some `flatpak update` on it).

Currently all bots that build with jhbuild run a step called "updated gtk dependencies" before running build-webkit. That step simply executes update-webkitgtk-libs.

I think its simply a matter of making this script call into update-webkitgtk-flatpak if the env var is not set.

[Philippe Normand]

(In reply to Carlos Alberto Lopez Perez from comment #6)
> (In reply to Philippe Normand from comment #5)
> > On a first run build-webkit will bootstrap everything itself :)
> 

Maybe I was wrong here, I'll double-check...

> I think that's not ok.
> 
> build-webkit should only use the jhbuild or the flatpak if it detects that
> the jhbuild or flatpak has been previously built (Check shouldUseJhbuild()
> and shouldUseFlatpak() in Tools/Scripts/webkitdirs.pm)
> 

Yes of course, I do know about those routines :)

[Philippe Normand]

Created attachment 386766 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 386766 [details]
Patch

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

> Tools/ChangeLog:16
> +        Flatpak repository hosted at https://software.igalia.com. The
> +        repository can be regenerated with the scripts from the
> +        webkit-flatpak-sdk Github project
> +        (https://github.com/Igalia/webkit-flatpak-sdk). This is done

I don't like the idea of having a separate repository in github for hosting the SDK.
I think the SDK source code should be moved to inside the WebKit repository (likely under the Tools directory)
so the usual review and commit policies can be followed

> Tools/BuildSlaveSupport/ews-build/steps.py:745
>      description = ['updating gtk dependencies']
>      descriptionDone = ['Updated gtk dependencies']
>      command = ['perl', 'Tools/Scripts/update-webkitgtk-libs']
> +    env = {'WEBKIT_JHBUILD': '1'}
>      haltOnFailure = False

I don't think we should set this environment variable from the EWS master.
This is something each worker should set in its on local environment if needed.

> Tools/BuildSlaveSupport/ews-build/steps.py:756
> +    env = {'WEBKIT_JHBUILD': '1'}

Ditto

> Tools/BuildSlaveSupport/ews-build/steps.py:828
> +        if platform in ('gtk', 'wpe'):
> +            self.env['WEBKIT_JHBUILD'] = '1'

Ditto

> Tools/BuildSlaveSupport/gtk/buildbot/run:70
> +		WEBKIT_JHBUILD=1 \

Ditto.
Actually this file is not used anywhere. Its just legacy.

> Tools/Scripts/webkitpy/common/system/executive.py:353
> +        if sys.platform.startswith('linux'):
> +            command = ["pkill", "-u", os.getenv("USER"), "-x", process_name]
> +        else:
> +            command = ["killall", "-TERM", "-u", os.getenv("USER"), process_name]

Why this change? Isn't killall available on the flatpak environ?

> Tools/Scripts/webkitpy/port/base.py:1223
> -        for path in ["/usr/sbin/httpd", "/usr/sbin/apache2", "/app/bin/httpd"]:
> +        for path in ["/usr/sbin/httpd", "/usr/sbin/apache2", "/usr/bin/httpd"]:

This looks wrong. Please add "/app/bin/httpd" but don't remove "/usr/bin/httpd"

[Philippe Normand]

Comment on attachment 386766 [details]
Patch

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

>> Tools/BuildSlaveSupport/gtk/buildbot/run:70
>> +		WEBKIT_JHBUILD=1 \
> 
> Ditto.
> Actually this file is not used anywhere. Its just legacy.

Let's remove it then. It's confusing :)

>> Tools/Scripts/webkitpy/common/system/executive.py:353
>> +            command = ["killall", "-TERM", "-u", os.getenv("USER"), process_name]
> 
> Why this change? Isn't killall available on the flatpak environ?

No. killall is part of psmisc, not shipped by default in the FDO runtime. Perhaps I can add it in ours though...

[Carlos Alberto Lopez Perez]

Comment on attachment 386766 [details]
Patch

anoter thing I noticed: flatpak packages are missing from Tools/gtk/install-dependencies and the wpe counterpart

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #11)
> Comment on attachment 386766 [details]
> Patch
> 
> anoter thing I noticed: flatpak packages are missing from
> Tools/gtk/install-dependencies and the wpe counterpart

Please do. Otherwise there are more scripts to be modified like Tools/BuildSlaveSupport/kill-old-processes

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #10)
> 
> >> Tools/Scripts/webkitpy/common/system/executive.py:353
> >> +            command = ["killall", "-TERM", "-u", os.getenv("USER"), process_name]
> > 
> > Why this change? Isn't killall available on the flatpak environ?
> 
> No. killall is part of psmisc, not shipped by default in the FDO runtime.
> Perhaps I can add it in ours though...

Please do. Otherwise there are more scripts to be modified like Tools/BuildSlaveSupport/kill-old-processes

[Philippe Normand]

Comment on attachment 386766 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/base.py:1223
>> +        for path in ["/usr/sbin/httpd", "/usr/sbin/apache2", "/usr/bin/httpd"]:
> 
> This looks wrong. Please add "/app/bin/httpd" but don't remove "/usr/bin/httpd"

No, it's correct. In the SDK the /app mount-point only contains the WebKit/ directory... Apache is installed in /usr.

[Carlos Alberto Lopez Perez]

Comment on attachment 386766 [details]
Patch

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

>>> Tools/Scripts/webkitpy/port/base.py:1223
>>> +        for path in ["/usr/sbin/httpd", "/usr/sbin/apache2", "/usr/bin/httpd"]:
>> 
>> This looks wrong. Please add "/app/bin/httpd" but don't remove "/usr/bin/httpd"
> 
> No, it's correct. In the SDK the /app mount-point only contains the WebKit/ directory... Apache is installed in /usr.

Ok. I see.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #13)
> (In reply to Philippe Normand from comment #10)
> > 
> > >> Tools/Scripts/webkitpy/common/system/executive.py:353
> > >> +            command = ["killall", "-TERM", "-u", os.getenv("USER"), process_name]
> > > 
> > > Why this change? Isn't killall available on the flatpak environ?
> > 
> > No. killall is part of psmisc, not shipped by default in the FDO runtime.
> > Perhaps I can add it in ours though...
> 
> Please do. Otherwise there are more scripts to be modified like
> Tools/BuildSlaveSupport/kill-old-processes

on a second thought: kill-old-processes runs outside of the flatpak environment so it should not be affected.

[Philippe Normand]

Comment on attachment 386766 [details]
Patch

This needs a bit more work, I detected some test failures related with drag&drop support, I'll try to investigate soon...

[Carlos Alberto Lopez Perez]

Done:

Exported the env var WEBKIT_JHBUILD=1 for all the relevant GTK and WPE buildbots. For the record:
- GTK Linux 64-bit Debug (Build)
- GTK Linux 64-bit Debug (Tests)
- GTK Linux 64-bit Release (Build)
- GTK Linux 64-bit Release (Perf)
- GTK Linux 64-bit Release (Tests)
- GTK Linux 64-bit Release Wayland (Tests)
- WPE Linux 64-bit Debug (Build)
- WPE Linux 64-bit Debug (Tests)
- WPE Linux 64-bit Release (Build)
- WPE Linux 64-bit Release (Tests)


Not exported for the following two bots because those don't run the jhbuild step:
- GTK Linux 64-bit Release Debian Stable (Build)
- GTK Linux 64-bit Release Ubuntu LTS (Build)


Done:

Exported the env var WEBKIT_JHBUILD=1 for all the Igalia EWS:

- igalia1-gtk-wk2-ews
- igalia2-gtk-wk2-ews
- igalia3-gtk-wk2-ews
- igalia4-gtk-wk2-ews
- igalia-wpe-ews


TODO (please talk with Adrian): Export the env var for the following EWS:

 - aperez-gtk-ews
 - aperez-wpe-ews

[Philippe Normand]

Great, many thanks Carlos!

[Adrian Perez]

(In reply to Carlos Alberto Lopez Perez from comment #18)
 
> TODO (please talk with Adrian): Export the env var for the following EWS:
> 
>  - aperez-gtk-ews
>  - aperez-wpe-ews

Done, now these two builders are using WEBKIT_JHBUILD=1 as well
since yesterday.

[Philippe Normand]

I'm afraid a massive rebaseline will be needed :( I've tried to downgrade Pango and Harfbuzz in the SDK to the versions pinned in JHBuild, but without success so far.

OTOH I think it would be good to finally bump these libs (and GTK). Perhaps it could be done in JHBuild along with the rebaseline?

In the SDK I currently have Pango 1.44.6, Harfbuzz 2.6.2, Freetype 2.10.1 and GTK 3.24.13.

[Philippe Normand]

Created attachment 387513 [details]
Patch

[Philippe Normand]

Comment on attachment 387513 [details]
Patch

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

> Tools/Scripts/update-webkitgtk-libs:25
> +if (defined $ENV{'WEBKIT_JHBUILD'} and $ENV{'WEBKIT_JHBUILD'}) {

This failed on the igalia2-gtk-wk2 EWS. Is the env var set on the bots? I can't find out what you did there :)

[Philippe Normand]

GTK EWS green now.

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #21)
> I'm afraid a massive rebaseline will be needed :( I've tried to downgrade
> Pango and Harfbuzz in the SDK to the versions pinned in JHBuild, but without
> success so far.
> 
> OTOH I think it would be good to finally bump these libs (and GTK). Perhaps
> it could be done in JHBuild along with the rebaseline?
> 
> In the SDK I currently have Pango 1.44.6, Harfbuzz 2.6.2, Freetype 2.10.1
> and GTK 3.24.13.

It will be needed indeed.

I think its maybe better to start with WPE, because it has less things to rebaseline (runs less tests) and also the WPE requirement of having a GPU is a good stress test for the flatpak suitability for running layout tests.

[Philippe Normand]

I agree, I locally checked the WPE tests with the SDK and didn't notice a huge amount of failures compared to the current status.

[Carlos Alberto Lopez Perez]

Comment on attachment 387513 [details]
Patch

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

> Tools/ChangeLog:16
> +        Flatpak repository hosted at https://software.igalia.com. The
> +        repository can be regenerated with the scripts from the
> +        webkit-flatpak-sdk Github project
> +        (https://github.com/Igalia/webkit-flatpak-sdk). This is done

What its your plan with this repository at https://github.com/Igalia/webkit-flatpak-sdk? Move it inside WebKit or leave it apart?

[Philippe Normand]

For now I'd like to keep it Github so we can iterate on its development faster. At some point I'd like to migrate it to Buildstream and once that's done it could move to the WebKit tree.

[Carlos Alberto Lopez Perez]

Comment on attachment 387513 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:-2094
> -    if (! -e getFlatpakPath()) {
> -        return 0;
> -    }

I think this should be kept?

AFAIK runInFlatpakIfAvailable() should not proceed if the flatpak subdir doesn't exist? should it?

> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:61
> -        log_directory = os.environ.get("WEBKIT_CORE_DUMPS_DIRECTORY")
> -        errors = []
> -        crash_log = ''
> -        expected_crash_dump_filename = "core-pid_%s.dump" % pid_representation
> -        proc_name = "%s" % (self.name)
> -
> -        def match_filename(filesystem, directory, filename):
> -            if self.pid:
> -                return filename == expected_crash_dump_filename
> -            return filename.find(self.name) > -1
> -
> -        # Poor man which, ignore any failure.
> -        for coredumpctl in [['coredumpctl'], ['flatpak-spawn', '--host', 'coredumpctl'], []]:
> -            try:
> -                if not self._executive.run_command(coredumpctl, return_exit_code=True):
> -                    break
> -            except:
> -                continue
> -
> -        if log_directory:
> -            dumps = self._filesystem.files_under(
> -                log_directory, file_filter=match_filename)
> -            if dumps:
> -                # Get the most recent coredump matching the pid and/or process name.
> -                coredump_path = list(reversed(sorted(dumps)))[0]
> -                if not self.newer_than or self._filesystem.mtime(coredump_path) > self.newer_than:
> -                    crash_log, errors = self._get_gdb_output(coredump_path)
> -        elif coredumpctl:
> -            crash_log, errors = self._get_trace_from_systemd(coredumpctl, pid_representation)
> -
> -        stderr_lines = errors + decode_if_necessary(str(stderr or '<empty>'), errors='ignore').splitlines()
> -        errors_str = '\n'.join(('STDERR: ' + stderr_line) for stderr_line in stderr_lines)
> +        proc_name = str(self.name)
> +
> +        if self.newer_than:
> +            coredump_since = "--gdb-stack-trace=@%f" % self.newer_than
> +        else:
> +            coredump_since = "--gdb-stack-trace"
> +        webkit_flatpak_path = self._webkit_finder.path_to_script('webkit-flatpak')
> +        cmd = ['flatpak-spawn', '--host', webkit_flatpak_path, '--%s' % self._port_name,
> +               "--%s" % self._configuration.lower(), coredump_since]
> +        crash_log = self._executive.run_command(cmd, ignore_errors=True)
> +
> +        stderr_lines = decode_if_necessary(str(stderr or '<empty>'), errors='ignore').splitlines()
> +        errors_str = '\n'.join(['STDERR: ' + stderr_line for stderr_line in stderr_lines])

I think this will break GDB backtrace generation on current bots, that are using the code you delete here (WEBKIT_CORE_DUMPS_DIRECTORY).
Also I think the automatic GDB backtrace generation code should still work for developers that opt to work outside of flatpak (both with coredumpctl and without it), so please add the new functionality without deleting the non-flatpak one.

[Carlos Alberto Lopez Perez]

Comment on attachment 387513 [details]
Patch

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

> Tools/flatpak/flatpakutils.py:-863
> -    def setup_icecc(self):
> -        with tempfile.NamedTemporaryFile() as tmpfile:
> -            self.run_in_sandbox('icecc', '--build-native', stdout=tmpfile, cwd=self.build_path)
> -            tmpfile.flush()
> -            tmpfile.seek(0)
> -            icc_version_filename, = re.findall(r'.*creating (.*)', tmpfile.read())
> -            self.icc_version = os.path.join(self.build_path, icc_version_filename)

Why this isn't needed anymore? is icecc supported in the new SDK? If so, how the native toolchain to pass to build hosts is now created?

[Philippe Normand]

Comment on attachment 387513 [details]
Patch

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

>> Tools/Scripts/webkitdirs.pm:-2094
>> -    }
> 
> I think this should be kept?
> 
> AFAIK runInFlatpakIfAvailable() should not proceed if the flatpak subdir doesn't exist? should it?

Right, this can be restored now that the SDK isn't automatically installed anymore.

>> Tools/Scripts/webkitpy/port/linux_get_crash_log.py:61
>> +        errors_str = '\n'.join(['STDERR: ' + stderr_line for stderr_line in stderr_lines])
> 
> I think this will break GDB backtrace generation on current bots, that are using the code you delete here (WEBKIT_CORE_DUMPS_DIRECTORY).
> Also I think the automatic GDB backtrace generation code should still work for developers that opt to work outside of flatpak (both with coredumpctl and without it), so please add the new functionality without deleting the non-flatpak one.

Sure thing!

>> Tools/flatpak/flatpakutils.py:-863
>> -            self.icc_version = os.path.join(self.build_path, icc_version_filename)
> 
> Why this isn't needed anymore? is icecc supported in the new SDK? If so, how the native toolchain to pass to build hosts is now created?

I didn't bother with icecc because I don't use it here. Do you? If not perhaps I can take a look...

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #31)
> >> Tools/flatpak/flatpakutils.py:-863
> >> -            self.icc_version = os.path.join(self.build_path, icc_version_filename)
> > 
> > Why this isn't needed anymore? is icecc supported in the new SDK? If so, how the native toolchain to pass to build hosts is now created?
> 
> I didn't bother with icecc because I don't use it here. Do you? If not
> perhaps I can take a look...

I do. I use it in combination with ccache.
In my honest opinion, no support for icecc would be a blocker for me for adopting a flatpak-based workflow.
Looking at the code, it seems to be already supported with the current flatpak SDK, so keep it please.

[Philippe Normand]

Created attachment 387674 [details]
Patch

[Philippe Normand]

Please test the icecreem stuff, I can't set it up here :)

update-webkitgtk-libs --debug
webkit-flatpak --gtk --debug --use-icecream -b

should enable it (I think).

[Jonathan Bedard]

Comment on attachment 387674 [details]
Patch

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

> Tools/flatpak/flatpakutils.py:837
> +                except subprocess.CalledProcessError, err:

We don't have Python 3 in EWS yet, but we're about to.

Can you make sure that you aren't breaking Python 3 compatibility with 'python3 Tools/Scripts/test-webkitpy'?

[Philippe Normand]

Thanks for the suggestion Jonathan, I'll check it. Right now I'm not sure much (if any) of that module is unit-tested though :(

[Philippe Normand]

Created attachment 387764 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 387764 [details]
Patch

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

> Tools/flatpak/flatpakutils.py:712
> +        env_var_prefixes_to_keep = [
> +            "GST",
> +            "GTK",
> +            "G",
> +            "JSC",
> +            "WEBKIT",
> +            "WEBKIT2",
> +            "WPE",
> +            "GIGACAGE",
> +        ]
> +
> +        env_vars_to_keep = [
> +            "JavaScriptCoreUseJIT",
> +            "Malloc",
> +            "WAYLAND_DISPLAY",
> +            "WAYLAND_SOCKET",
> +            "DISPLAY",
> +            "LANG",
> +            "NUMBER_OF_PROCESSORS",
> +            "CCACHE_PREFIX",
> +            "QML2_IMPORT_PATH",
> +        ]

This misses some interesting environment variables like:
CC, CXX, CFLAGS, CXXFLAGS, LDFLAGS, etc...
Sometimes I set this variables to test different things or with different compilers (clang).
Also for X11 users I think we also need to also pass the environment variable XAUTHORITY.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #38)
> Comment on attachment 387764 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387764&action=review
> 
> > Tools/flatpak/flatpakutils.py:712
> > +        env_var_prefixes_to_keep = [
> > +            "GST",
> > +            "GTK",
> > +            "G",
> > +            "JSC",
> > +            "WEBKIT",
> > +            "WEBKIT2",
> > +            "WPE",
> > +            "GIGACAGE",
> > +        ]
> > +
> > +        env_vars_to_keep = [
> > +            "JavaScriptCoreUseJIT",
> > +            "Malloc",
> > +            "WAYLAND_DISPLAY",
> > +            "WAYLAND_SOCKET",
> > +            "DISPLAY",
> > +            "LANG",
> > +            "NUMBER_OF_PROCESSORS",
> > +            "CCACHE_PREFIX",
> > +            "QML2_IMPORT_PATH",
> > +        ]
> 
> This misses some interesting environment variables like:
> CC, CXX, CFLAGS, CXXFLAGS, LDFLAGS, etc...
> Sometimes I set this variables to test different things or with different
> compilers (clang).
> Also for X11 users I think we also need to also pass the environment
> variable XAUTHORITY.

Ah.. it also misses the environment variables:

BUILD_JSC_ARGS
BUILD_WEBKIT_ARGS
TEST_JSC_ARGS

The bots may set those to influence the build parameters of webkit or jsc. For example we set those on the debian/ubuntu stable bots to disable features when they fail to build there.

Perhaps "WEBKIT" and "JSC" should not be a prefix, but a string in any part of the environment variable name?

[Carlos Alberto Lopez Perez]

Comment on attachment 387764 [details]
Patch

I tested this now on buildbox2 and I can't get the layout tests to work. They always timeout, even for basic tests like fast/css/word-space-extra.html
Does it work for you? :(

[Adrian Perez]

Comment on attachment 387764 [details]
Patch

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

I have just a couple of additional comments on top of Carlos' — otherwise it's
looking pretty good and I have been able to check that things work well here
applying the patch in my checkout 👍

> Source/WebCore/platform/text/hyphen/HyphenationLibHyphen.cpp:124
> +    // Try alternative dictionaries path for people using Flatpak.

I would guard trying to use /usr/share/webkitgtk-test-dicts with ENABLE(DEVELOPER_MODE).
Such a path should not be used in a normal build.

> Tools/Scripts/test262-runner:49
> +                # module... Perl. ¯\_(ã)_/¯

Usage of ¯\_(ツ)_/¯ gladly agreed upon when referring to Perl =}

> Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:69
> +    GUniquePtr<char>fontsPath(g_build_filename("/usr", "share", "webkitgtk-test-fonts", NULL));

For this I also think that it could be guarded with ENABLE(DEVELOPER_MODE).
I know that it was not between guards already before, but these changes
could be a good chance to add them.

[Adrian Perez]

(In reply to Carlos Alberto Lopez Perez from comment #40)
> Comment on attachment 387764 [details]
> Patch
> 
> I tested this now on buildbox2 and I can't get the layout tests to work.
> They always timeout, even for basic tests like fast/css/word-space-extra.html
> Does it work for you? :(

I ran just fine a few tests earlier with this patch applied, and to
make sure I have just ran all the tests under fast/css/ (which also
went well).

[Philippe Normand]

(In reply to Adrian Perez from comment #42)
> (In reply to Carlos Alberto Lopez Perez from comment #40)
> > Comment on attachment 387764 [details]
> > Patch
> > 
> > I tested this now on buildbox2 and I can't get the layout tests to work.
> > They always timeout, even for basic tests like fast/css/word-space-extra.html
> > Does it work for you? :(
> 
> I ran just fine a few tests earlier with this patch applied, and to
> make sure I have just ran all the tests under fast/css/ (which also
> went well).

There's an issue, as Carlos pointed out. The layout tests run on Debian stretch and bullseye but not on buster. I'm trying to debug it but so far, no solution :/

[Adrian Perez]

(In reply to Philippe Normand from comment #43)
> (In reply to Adrian Perez from comment #42)
> > (In reply to Carlos Alberto Lopez Perez from comment #40)
> > > Comment on attachment 387764 [details]
> > > Patch
> > > 
> > > I tested this now on buildbox2 and I can't get the layout tests to work.
> > > They always timeout, even for basic tests like fast/css/word-space-extra.html
> > > Does it work for you? :(
> > 
> > I ran just fine a few tests earlier with this patch applied, and to
> > make sure I have just ran all the tests under fast/css/ (which also
> > went well).
> 
> There's an issue, as Carlos pointed out. The layout tests run on Debian
> stretch and bullseye but not on buster. I'm trying to debug it but so far,
> no solution :/

Right. I should have mentioned that here I was using Arch Linux.

[Philippe Normand]

The issue in Buster headless hosts is that xdg-desktop-portal doesn't automatically start up, unless I manually start it up at least once. There goes my day debugging this...

[Philippe Normand]

Comment on attachment 387764 [details]
Patch

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

>> Source/WebCore/platform/text/hyphen/HyphenationLibHyphen.cpp:124
>> +    // Try alternative dictionaries path for people using Flatpak.
> 
> I would guard trying to use /usr/share/webkitgtk-test-dicts with ENABLE(DEVELOPER_MODE).
> Such a path should not be used in a normal build.

I know it's not explicitly noticeable here but this code is already guarded with ENABLE(DEVELOPER_MODE) :)

>> Tools/WebKitTestRunner/InjectedBundle/gtk/ActivateFontsGtk.cpp:69
>> +    GUniquePtr<char>fontsPath(g_build_filename("/usr", "share", "webkitgtk-test-fonts", NULL));
> 
> For this I also think that it could be guarded with ENABLE(DEVELOPER_MODE).
> I know that it was not between guards already before, but these changes
> could be a good chance to add them.

Do you mean the whole file?

[Philippe Normand]

Comment on attachment 387764 [details]
Patch

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

>>> Tools/flatpak/flatpakutils.py:712
>>> +        ]
>> 
>> This misses some interesting environment variables like:
>> CC, CXX, CFLAGS, CXXFLAGS, LDFLAGS, etc...
>> Sometimes I set this variables to test different things or with different compilers (clang).
>> Also for X11 users I think we also need to also pass the environment variable XAUTHORITY.
> 
> Ah.. it also misses the environment variables:
> 
> BUILD_JSC_ARGS
> BUILD_WEBKIT_ARGS
> TEST_JSC_ARGS
> 
> The bots may set those to influence the build parameters of webkit or jsc. For example we set those on the debian/ubuntu stable bots to disable features when they fail to build there.
> 
> Perhaps "WEBKIT" and "JSC" should not be a prefix, but a string in any part of the environment variable name?

XAUTHORITY is already magically set by flatpak. I can pass-through the other vars.

[Philippe Normand]

Created attachment 387924 [details]
Patch

[Philippe Normand]

Comment on attachment 387924 [details]
Patch

I have a working ICECC setup now so I can debug this further. The patch likely needs a few more changes.

[Philippe Normand]

Created attachment 389122 [details]
Patch

[Philippe Normand]

IceCC working now (hopefully): build-webkit --gtk --use-icecream
The host needs a running icecc-scheduler. I'm not sure sandboxing this would be useful.

[Philippe Normand]

Created attachment 389125 [details]
Patch

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #38)
> This misses some interesting environment variables like:
> CC, CXX, CFLAGS, CXXFLAGS, LDFLAGS, etc...
> Sometimes I set this variables to test different things or with different
> compilers (clang).

I don't see this on the last version of the patch. Would be it possible to add this variables to the list? Thanks

[Philippe Normand]

I'll add the ones explicitly mentioned. Dunno about the etc part :)

[Philippe Normand]

Created attachment 389255 [details]
Patch

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #54)
> I'll add the ones explicitly mentioned. Dunno about the etc part :)

Thanks.

We need to add also this one: "WEBKIT_TEST_CHILD_PROCESSES"

Its set by the bots to tell run-jsc-stress-tests how much cores it can use.

Sorry for not commenting about it before.

[Philippe Normand]

(In reply to Carlos Alberto Lopez Perez from comment #56)
> (In reply to Philippe Normand from comment #54)
> > I'll add the ones explicitly mentioned. Dunno about the etc part :)
> 
> Thanks.
> 
> We need to add also this one: "WEBKIT_TEST_CHILD_PROCESSES"
> 

All WEBKIT_* env vars are forwarded into the sandbox environment. More prefixes are white-listed via the env_var_prefixes_to_keep variable in flatpakutils.py

[Philippe Normand]

Created attachment 389334 [details]
Patch

[Carlos Alberto Lopez Perez]

From a empty build state (WebKitBuild directory wiped), if I run update-webkitgtk-libs (no env var WEBKIT_JHBUILD set) I get this

$ Tools/Scripts/update-webkitgtk-libs
Adding repo webkit-sdk
Traceback (most recent call last):
  File "Tools/Scripts/update-webkit-flatpak", line 28, in <module>
    WebkitFlatpak.load_from_args(["--update"] + sys.argv[1:]).run()
  File "./Tools/flatpak/flatpakutils.py", line 757, in run
    if not self.clean_args():
  File "./Tools/flatpak/flatpakutils.py", line 560, in clean_args
    FlatpakRepo("flathub", repo_file="https://dl.flathub.org/repo/flathub.flatpakrepo")
  File "./Tools/flatpak/flatpakutils.py", line 290, in __init__
    self.url = repo["Flatpak Repo"]["Url"]
AttributeError: ConfigParser instance has no attribute '__getitem__'
Died at Tools/Scripts/update-webkitgtk-libs line 28.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #59)
> From a empty build state (WebKitBuild directory wiped), if I run
> update-webkitgtk-libs (no env var WEBKIT_JHBUILD set) I get this
> 
> $ Tools/Scripts/update-webkitgtk-libs
> Adding repo webkit-sdk
> Traceback (most recent call last):
>   File "Tools/Scripts/update-webkit-flatpak", line 28, in <module>
>     WebkitFlatpak.load_from_args(["--update"] + sys.argv[1:]).run()
>   File "./Tools/flatpak/flatpakutils.py", line 757, in run
>     if not self.clean_args():
>   File "./Tools/flatpak/flatpakutils.py", line 560, in clean_args
>     FlatpakRepo("flathub",
> repo_file="https://dl.flathub.org/repo/flathub.flatpakrepo")
>   File "./Tools/flatpak/flatpakutils.py", line 290, in __init__
>     self.url = repo["Flatpak Repo"]["Url"]
> AttributeError: ConfigParser instance has no attribute '__getitem__'
> Died at Tools/Scripts/update-webkitgtk-libs line 28.

mmm, strange. I can't reproduce this on buildbox2. But it happens on my laptop :\

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #60)
> (In reply to Carlos Alberto Lopez Perez from comment #59)
> > From a empty build state (WebKitBuild directory wiped), if I run
> > update-webkitgtk-libs (no env var WEBKIT_JHBUILD set) I get this
> > 
> > $ Tools/Scripts/update-webkitgtk-libs
> > Adding repo webkit-sdk
> > Traceback (most recent call last):
> >   File "Tools/Scripts/update-webkit-flatpak", line 28, in <module>
> >     WebkitFlatpak.load_from_args(["--update"] + sys.argv[1:]).run()
> >   File "./Tools/flatpak/flatpakutils.py", line 757, in run
> >     if not self.clean_args():
> >   File "./Tools/flatpak/flatpakutils.py", line 560, in clean_args
> >     FlatpakRepo("flathub",
> > repo_file="https://dl.flathub.org/repo/flathub.flatpakrepo")
> >   File "./Tools/flatpak/flatpakutils.py", line 290, in __init__
> >     self.url = repo["Flatpak Repo"]["Url"]
> > AttributeError: ConfigParser instance has no attribute '__getitem__'
> > Died at Tools/Scripts/update-webkitgtk-libs line 28.
> 
> mmm, strange. I can't reproduce this on buildbox2. But it happens on my
> laptop :\

Ok, the issue its with different configparser modules.

python-configparse its able to correctly parse the flatpak repo definition, but  ConfigParse from python-core (/usr/lib/python2.7/ConfigParser.py) fails to get a section with space

ipdb> repo.sections()
['Flatpak Repo']
ipdb> repo['Flatpak Repo']
*** AttributeError: ConfigParser instance has no attribute '__getitem__'
ipdb> configparser
<module 'ConfigParser' from '/usr/lib/python2.7/ConfigParser.pyc'>


Something like this should fix it:

--- a/Tools/flatpak/flatpakutils.py
+++ b/Tools/flatpak/flatpakutils.py
@@ -16,10 +16,7 @@
 # Boston, MA 02110-1301, USA.
 import argparse
 import logging
-try:
-    import configparser
-except ImportError:
-    import ConfigParser as configparser
+import configparser
 from contextlib import contextmanager
 import errno
 import json


and then we have to add python-configparser to the list of packages to be installed (I had not installed it)

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #61)
> Ok, the issue its with different configparser modules.
> 
> python-configparse its able to correctly parse the flatpak repo definition,
> but  ConfigParse from python-core (/usr/lib/python2.7/ConfigParser.py) fails
> to get a section with space
> 
> 

Its not that. Its that ConfigParse has different semantics/syntax to get this.

You have to use get(section, option) instead of accessing it like a dict.

This patch will fix the issue:

--- a/Tools/flatpak/flatpakutils.py
+++ b/Tools/flatpak/flatpakutils.py
@@ -287,7 +287,10 @@ class FlatpakRepo(FlatpakObject):
         if repo_file and not url:
             repo = configparser.ConfigParser()
             repo.read(self.repo_file.name)
-            self.url = repo["Flatpak Repo"]["Url"]
+            try:
+                self.url = repo["Flatpak Repo"]["Url"]
+            except AttributeError:
+                self.url = repo.get("Flatpak Repo", "Url")
         else:
             assert url

[Philippe Normand]

Created attachment 389341 [details]
Patch

[Philippe Normand]

Created attachment 389532 [details]
Patch

[Carlos Alberto Lopez Perez]

It is looking good to me. I think this its almost ready to land. But let's give it a final round of testing :)
I have tested to run with this all the test steps that we test for GTK, and all worked but one (see at bottom)

Let'ts see:

1) Steps for the GTK release test bot:

* jscore-test: OK

* layout-test: OK

I have run it and I get this:

Regressions: Unexpected text-only failures (204)
Regressions: Unexpected image-only failures (28)
Regressions: Unexpected audio failures (1)
Regressions: Unexpected crashes (10)
Regressions: Unexpected timeouts (8)

Which is not bad at all, I was expecting to get much more failures.

One thing I noticed its there are lot of errors logged with this:

AL lib: (EE) ALCplaybackOSS_open: Could not open /dev/dsp: No such file or directory


* webkitpy-test: OK
* webkitperl-test: OK
* bindings-generation-tests: OK
* builtins-generator-tests: OK
* dashboard-tests: OK

* api-tests: OK

The step runs ok, but it gives this extra errors:

Extra API tests failures:

Unexpected failures (4)
    /WebKit2Gtk/TestSSL
        /webkit/WebKitWebView/tls-errors-policy
    /WebKit2Gtk/TestDownloads
        /webkit/Downloads/remote-file-error
    /WebKit2Gtk/TestLoaderClient
        /webkit/WebKitWebView/is-loading
        /webkit/WebKitWebView/unfinished-subresource-load

Unexpected timeouts (1)
    /WebKit2Gtk/TestWebKitFaviconDatabase
        /webkit/WebKitFaviconDatabase/get-favicon


* webdriver-tests: OK
* test262-tests: OK


2) Steps for the GTK performance bot:

* benchmark-test: OK

* performance-tests: FAIL

This is the failure I get:

$ Tools/Scripts/run-perf-tests --no-build --release --platform gtk
ImageDiff was not found at /home/clopez/webkit/webkit-flatpak/WebKitBuild/Release/bin/ImageDiff
Failed to run "['Tools/Scripts/build-imagediff', '--release', '--gtk']" exit_code: 254 cwd: /home/clopez/webkit/webkit-flatpak

sysctl: cannot stat /proc/sys/hw/activecpu: No such file or directory
Can't exec "xcodebuild": No such file or directory at /home/clopez/webkit/webkit-flatpak/Tools/Scripts/webkitdirs.pm line 1972.

Build not up to date for /home/clopez/webkit/webkit-flatpak/WebKitBuild/Release/bin/WebKitTestRunner


So, summarizing, before landing this I think we should try to fix this broken step of performance-tests (script run-perf-tests)

[Philippe Normand]

Oh, TIL run-perf-tests :) No wonder it fails, I didn't know it exists ;)

I'll have a look at it and also check the API tests again. The /dev/dsp warnings could easily be fixed as well.

[Carlos Alberto Lopez Perez]

Another thing i just noticed its not working its the ability of build-webkit to clean the build.

This fails:

$ Tools/Scripts/build-webkit --gtk --release --clean
Building flatpak based environment
+ cmake --build /app/webkit/WebKitBuild/Release --config Release --target clean
Error: could not load cache
Command '['flatpak', 'build', '--die-with-parent', '--talk-name=org.a11y.Bus', '--talk-name=org.gtk.vfs', '--talk-name=org.gtk.vfs.*', '--bind-mount=/run/shm=/dev/shm', '--bind-mount=/run/host//tmp=/tmp', '--bind-mount=/app/webkit=/home/igalia/clopez/webkit/webkit-flatpak', '--bind-mount=/app/webkit/WebKitBuild/Release=/home/igalia/clopez/webkit/webkit-flatpak/WebKitBuild/GTK/Release', '--env=LANG=en_US.UTF-8', '--env=WEBKIT_TOP_LEVEL=/app/', '--env=TEST_RUNNER_INJECTED_BUNDLE_FILENAME=/app/webkit/WebKitBuild/Release/lib/libTestRunnerInjectedBundle.so', '/home/igalia/clopez/webkit/webkit-flatpak/WebKitBuild/UserFlatpak', '/app/webkit/Tools/Scripts/build-webkit', '--release', '--gtk', '--clean', '--prefix=/app']' returned non-zero exit status 1


And was working before this.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #67)
> Another thing i just noticed its not working its the ability of build-webkit
> to clean the build.
> 
> This fails:
> 
> $ Tools/Scripts/build-webkit --gtk --release --clean
> Building flatpak based environment

mmm, forget that.. now its working for me... no idea why it failed before :\

[Carlos Alberto Lopez Perez]

Comment on attachment 389532 [details]
Patch

Setting r- to not forget that this version of the patch still doesn't fix run-perf-tests.

[Philippe Normand]

Created attachment 391654 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 391654 [details]
Patch

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

> Tools/flatpak/flatpakutils.py:51
>  FLATPAK_REQ = [
> -    ("flatpak", "0.10.0"),
> -    ("flatpak-builder", "0.10.0"),
> +    ("flatpak", "1.2.5"),

....

> Tools/flatpak/flatpakutils.py:-306
> -        if FLATPAK_VERSION["flatpak"] < (1, 1, 2):
> -            out = self.flatpak("list", "-d", *args)
> -            package_defs = [line for line in out.split("\n") if line]
> -            for package_def in package_defs:
> -                splited_packaged_def = package_def.split()
> -                name, arch, branch = splited_packaged_def[0].split("/")
> -
> -                # If installed from a file, the package is in no repo
> -                repo_name = splited_packaged_def[1]
> -                repo = self.repos.repos.get(repo_name)
> -
> -                packs.append(FlatpakPackage(name, branch, repo, arch))
> -        else:
> -            out = self.flatpak("list", "--columns=application,arch,branch,origin", *args)
> -            package_defs = [line for line in out.split("\n") if line]
> -            for package_def in package_defs:
> -                name, arch, branch, origin = package_def.split("\t")

We have to support at least flatpak 1.0.8 (which is what Ubuntu 18.04 ships).
This change was not in the previous patch.

[Philippe Normand]

Could we detect old versions in Ubuntu and recommend to install this PPA? https://launchpad.net/~alexlarsson/+archive/ubuntu/flatpak

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #72)
> Could we detect old versions in Ubuntu and recommend to install this PPA?
> https://launchpad.net/~alexlarsson/+archive/ubuntu/flatpak

Works for me considering that 1. the PPA its provided by upstream itself <https://flatpak.org/setup/Ubuntu/> and that 2. it will dramatically improve the first download time due to https://github.com/flatpak/flatpak/issues/3412

[Philippe Normand]

OK so do you want me to bump the version requirement to 1.6.2 then? I see it's available in the PPA.

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #74)
> OK so do you want me to bump the version requirement to 1.6.2 then? I see
> it's available in the PPA.

I see 1.6.2 its available for Debian 10 also on the buster-backports repository. 

So you can bump the requirement if you wish.

[Philippe Normand]

Created attachment 392288 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 392288 [details]
Patch

Sorry for the r-, I really wanted to give r+ this time... but I gave this another round of testing and this is breaking users opting to not use the flatpak environment.
Not sure if this was happening with previous versions of this patch or its new behaviour, but I really think this needs to be fixed before landing.

Let's see. I tested this

Test A) User that don't uses the JHBuild neither the flatpak (just builds webkit against the system libraries).
1. Builds WebKit.
$ rm -fr WebKitBuild
$ Tools/Scripts/build-webkit --gtk --cmakeargs=-DUSE_WPE_RENDERER=OFF
2. Tries to run layout-tests or perf-tests or api-tests or any test script. Example:
$ Tools/Scripts/run-gtk-tests

Expected behaviour:
- it runs the test script (without doing any flatpak thing)

Current behaviour:
- it fails with:
$ Tools/Scripts/run-gtk-tests 
Adding repo webkit-sdk
Traceback (most recent call last):
  File "Tools/Scripts/run-gtk-tests", line 132, in <module>
    flatpakutils.run_in_sandbox_if_available(sys.argv)
  File "./Tools/flatpak/flatpakutils.py", line 853, in run_in_sandbox_if_available
    sys.exit(flatpak_runner.run_in_sandbox(*args))
  File "./Tools/flatpak/flatpakutils.py", line 551, in run_in_sandbox
    self.setup_builddir(stdout=kwargs.get("stdout", sys.stdout))
  File "./Tools/flatpak/flatpakutils.py", line 522, in setup_builddir
    self.sdk.branch,
AttributeError: 'NoneType' object has no attribute 'branch'


Test B) User that wants to keep using the JHBuild by opting to it by setting WEBKIT_JHBUILD=1 when calling update-webkitgtk-libs
1. Builds the JHBuild
$ rm -fr WebKitBuild/
$ WEBKIT_JHBUILD=1 Tools/Scripts/update-webkitgtk-libs
2. Tries to builds webKit
$ Tools/Scripts/build-webkit --gtk

Expected behaviour:
- It builds fine with the JHBuild, even when the environment variable WEBKIT_JHBUILD=1 its not passed to the script build-webkit.

Current behaviour:
- It tries to use the system libraries instead of the JHBuild (aka: doesn't enable the JHBuild) and the build fails (in my case becasue libwpe is required for USE_WPE_RENDERER and I have not libwpe installed system-wide)


So, to summarize, this is how I think this should work:

1) I think the flatpak environment should *only* be enabled for the test scripts (run-webkit-tests, run-minibrowser, run-perf-tests, etc) *IF* the flatpak has been built previously (you can detect the user-flatpak directory being present in WebKitBuild/).

2) And to keep using JHBuild, it should *only* be needed to set the environment variable WEBKIT_FLATPAK=1 when executing the script update-webkitgtk-libs. All the other scripts (build-webkit, run-layout-tests, run-minibrowser, etc) should not rely on this environment variable to decide if enabling flatpak or JHbuild. They should simply use a directory-based checking to known what kind of third-party library system has been enabled (if any):
 - 1. If the flatpak directories are in WebKitBuild: Enable flatpak
 - 2. If the JHBuild directories are in WebKitBuild: Enable JHBuild
 - 3. Else: don't enable anything.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #77)
> 
> So, to summarize, this is how I think this should work:
> 
> 1) I think the flatpak environment should *only* be enabled for the test
> scripts (run-webkit-tests, run-minibrowser, run-perf-tests, etc) *IF* the
> flatpak has been built previously (you can detect the user-flatpak directory
> being present in WebKitBuild/).
> 
> 2) And to keep using JHBuild, it should *only* be needed to set the
> environment variable WEBKIT_FLATPAK=1 when executing the script
> update-webkitgtk-libs. All the other scripts (build-webkit,
> run-layout-tests, run-minibrowser, etc) should not rely on this environment
> variable to decide if enabling flatpak or JHbuild. They should simply use a
> directory-based checking to known what kind of third-party library system
> has been enabled (if any):
>  - 1. If the flatpak directories are in WebKitBuild: Enable flatpak
>  - 2. If the JHBuild directories are in WebKitBuild: Enable JHBuild
>  - 3. Else: don't enable anything.

Proposed patch on top of the current one: http://sprunge.us/T3Qir7?diff

[Philippe Normand]

(In reply to Carlos Alberto Lopez Perez from comment #77)

> So, to summarize, this is how I think this should work:
> 
> 1) I think the flatpak environment should *only* be enabled for the test
> scripts (run-webkit-tests, run-minibrowser, run-perf-tests, etc) *IF* the
> flatpak has been built previously (you can detect the user-flatpak directory
> being present in WebKitBuild/).
> 
> 2) And to keep using JHBuild, it should *only* be needed to set the
> environment variable WEBKIT_FLATPAK=1 when executing the script
> update-webkitgtk-libs.

I guess you mean WEBKIT_JHBUILD ?

> All the other scripts (build-webkit,
> run-layout-tests, run-minibrowser, etc) should not rely on this environment
> variable to decide if enabling flatpak or JHbuild. They should simply use a
> directory-based checking to known what kind of third-party library system
> has been enabled (if any):
>  - 1. If the flatpak directories are in WebKitBuild: Enable flatpak
>  - 2. If the JHBuild directories are in WebKitBuild: Enable JHBuild
>  - 3. Else: don't enable anything.

This won't encourage anybody to try Flatpak though, because devs already have a JHBuild or nothing (Hi Carlos Garcia).

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #79)
> (In reply to Carlos Alberto Lopez Perez from comment #77)
> 
> > So, to summarize, this is how I think this should work:
> > 
> > 1) I think the flatpak environment should *only* be enabled for the test
> > scripts (run-webkit-tests, run-minibrowser, run-perf-tests, etc) *IF* the
> > flatpak has been built previously (you can detect the user-flatpak directory
> > being present in WebKitBuild/).
> > 
> > 2) And to keep using JHBuild, it should *only* be needed to set the
> > environment variable WEBKIT_FLATPAK=1 when executing the script
> > update-webkitgtk-libs.
> 
> I guess you mean WEBKIT_JHBUILD ?
> 

yes, i meant that

> > All the other scripts (build-webkit,
> > run-layout-tests, run-minibrowser, etc) should not rely on this environment
> > variable to decide if enabling flatpak or JHbuild. They should simply use a
> > directory-based checking to known what kind of third-party library system
> > has been enabled (if any):
> >  - 1. If the flatpak directories are in WebKitBuild: Enable flatpak
> >  - 2. If the JHBuild directories are in WebKitBuild: Enable JHBuild
> >  - 3. Else: don't enable anything.
> 
> This won't encourage anybody to try Flatpak though, because devs already
> have a JHBuild or nothing (Hi Carlos Garcia).

I think it will do. We are changing the default. Until now the default was JHBuild. Now it will be flatpak. We are even changing the default script to update third-party libs (update-webkitgtk-libs) to use flatpak.

I don't think that by making this more annoying to devs opting-out to not use flatpak (by forcing them to set WEBKIT_JHBUILD=0 in their .bashrc) is a good idea; neither I think will encourage anyone.

The idea of flatpak is to give it a serious try and if it works ok for almost everybody and for the bots then remove JHBuild.. and if not remove flatpak.. we can't live with the two third-party systems forever, we have to make a choice at some point and go with that.

And no matter what system for third-party libraries we choose, building WebKit against the system libraries and running tests with that has to be supported always.

[Philippe Normand]

(In reply to Carlos Alberto Lopez Perez from comment #78)

> > 2) And to keep using JHBuild, it should *only* be needed to set the
> > environment variable WEBKIT_FLATPAK=1 when executing the script
> > update-webkitgtk-libs. All the other scripts (build-webkit,
> > run-layout-tests, run-minibrowser, etc) should not rely on this environment
> > variable to decide if enabling flatpak or JHbuild. They should simply use a
> > directory-based checking to known what kind of third-party library system
> > has been enabled (if any):
> >  - 1. If the flatpak directories are in WebKitBuild: Enable flatpak
> >  - 2. If the JHBuild directories are in WebKitBuild: Enable JHBuild
> >  - 3. Else: don't enable anything.
> 
> Proposed patch on top of the current one: http://sprunge.us/T3Qir7?diff

OK so I've tried this patch. The build failed, g-ir-scanner wanted to load a libpcre that is not in the flatpak SDK. An hour later, after trying to debug this in the flatpak SDK, I notice the JHBuild sysroot mentioned in the build log...

So I don't know... what should we do when both the jhbuild and the flatpak dirs are present? I'd rather keep the explicitly defined behavior based on WEBKIT_JHBUILD.

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #81)
> (In reply to Carlos Alberto Lopez Perez from comment #78)
> 
> > > 2) And to keep using JHBuild, it should *only* be needed to set the
> > > environment variable WEBKIT_FLATPAK=1 when executing the script
> > > update-webkitgtk-libs. All the other scripts (build-webkit,
> > > run-layout-tests, run-minibrowser, etc) should not rely on this environment
> > > variable to decide if enabling flatpak or JHbuild. They should simply use a
> > > directory-based checking to known what kind of third-party library system
> > > has been enabled (if any):
> > >  - 1. If the flatpak directories are in WebKitBuild: Enable flatpak
> > >  - 2. If the JHBuild directories are in WebKitBuild: Enable JHBuild
> > >  - 3. Else: don't enable anything.
> > 
> > Proposed patch on top of the current one: http://sprunge.us/T3Qir7?diff
> 
> OK so I've tried this patch. The build failed, g-ir-scanner wanted to load a
> libpcre that is not in the flatpak SDK. An hour later, after trying to debug
> this in the flatpak SDK, I notice the JHBuild sysroot mentioned in the build
> log...
> 

Do you know why it entered into the jhbuild path when you had present the flatpak directories?

> So I don't know... what should we do when both the jhbuild and the flatpak
> dirs are present? 

As i see this, if both directories are present, then the flatpak environment should be enabled. The only way to go back to a JHBuild-based build environment would be to wipe the flatpak directories from WebKitBuild.

> I'd rather keep the explicitly defined behavior based on WEBKIT_JHBUILD.

If you prefer that, then I'm fine with being able to force one or the other via the value of that environment variable, but I think we still have to define a default way to operate when that environment variable its not present, and that way to operate its to check what directories for third-party libs are present inside WebKitBuild: 1) if flatpak dir is present: enable flatpak 2) if jhbuild dir is present: enable jhbuild 3) else: don't enable anything.


So the final algorithm could be something like this:

if (defined WEBKIT_JHBUILD and WEBKIT_JHBUILD==1): enable jhbuild
if (defined WEBKIT_JHBUILD and WEBKIT_JHBUILD==0): enable flatpak
if (flatpakDir in WebKitBuild): enable flatpak
if (jhBuildDir in WebKitBuild): enable jhbuild
(default): no enable anything, use system libraries

[Carlos Alberto Lopez Perez]

Comment on attachment 392288 [details]
Patch

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

> Tools/flatpak/flatpakutils.py:51
> +    ("flatpak", "1.6.2"),

By the way, I have remembered that Fedora 31 ships flatpak 1.4.4
We have to lower this, as we support developers using Fedora.

[Philippe Normand]

(In reply to Carlos Alberto Lopez Perez from comment #82)
> 
> Do you know why it entered into the jhbuild path when you had present the
> flatpak directories?
> 

Yes, in webkitdirs.pm, the wrapperPrefixIfNeeded() enabled jhbuild.

> > So I don't know... what should we do when both the jhbuild and the flatpak
> > dirs are present? 
> 
> As i see this, if both directories are present, then the flatpak environment
> should be enabled. The only way to go back to a JHBuild-based build
> environment would be to wipe the flatpak directories from WebKitBuild.
> 
> > I'd rather keep the explicitly defined behavior based on WEBKIT_JHBUILD.
> 
> If you prefer that, then I'm fine with being able to force one or the other
> via the value of that environment variable, but I think we still have to
> define a default way to operate when that environment variable its not
> present, and that way to operate its to check what directories for
> third-party libs are present inside WebKitBuild: 1) if flatpak dir is
> present: enable flatpak 2) if jhbuild dir is present: enable jhbuild 3)
> else: don't enable anything.
> 
> 
> So the final algorithm could be something like this:
> 
> if (defined WEBKIT_JHBUILD and WEBKIT_JHBUILD==1): enable jhbuild
> if (defined WEBKIT_JHBUILD and WEBKIT_JHBUILD==0): enable flatpak
> if (flatpakDir in WebKitBuild): enable flatpak
> if (jhBuildDir in WebKitBuild): enable jhbuild
> (default): no enable anything, use system libraries

I'll give this a try :)

(In reply to Carlos Alberto Lopez Perez from comment #83)
> Comment on attachment 392288 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392288&action=review
> 
> > Tools/flatpak/flatpakutils.py:51
> > +    ("flatpak", "1.6.2"),
> 
> By the way, I have remembered that Fedora 31 ships flatpak 1.4.4
> We have to lower this, as we support developers using Fedora.

👍

[Philippe Normand]

Created attachment 393154 [details]
Patch

[Philippe Normand]

Comment on attachment 393154 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:2146
> +        } else {
> +            # or let Flatpak take precedence over JHBuild.
> +            if (-e getUserFlatpakPath()) {

This can be simplified a bit, I didn't know perl has a "elsif" ...

[Philippe Normand]

Created attachment 393214 [details]
Patch

[Philippe Normand]

run-javascriptcore-tests needs a few updates, as detected by EWS.

[Philippe Normand]

Created attachment 393223 [details]
Patch

[Philippe Normand]

I'm adding support in build-jsc too, but I wonder, what should I do for JSCOnly? On Linux platforms would it be OK to use Flatpak?

[Philippe Normand]

Created attachment 393225 [details]
Patch

[Philippe Normand]

Another bug:

env CC=clang CXX=clang++ build-webkit --gtk --update-gtk
Building flatpak based environment

You need to install flatpak >= 1.4.4 to be able to use the 'Tools/Scripts/update-webkit-flatpak' script.

You can find some informations about how to install it for your distribution at:
    * http://flatpak.org/

+  cmake --build /app/webkit/WebKitBuild/Release --config Release -- -j12


We execute the update-webkitgtk-libs script within the sandbox...

[Philippe Normand]

Are these --update-gtk and --update-wpe switches still needed? The buildbots don't seem to use that.

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #93)
> Are these --update-gtk and --update-wpe switches still needed? 

No idea if some developer uses them.

but fixing it may be simple? you can exit with zero status early on the scripts update-webkitwpe-libs/update-webkitgtk-libs if you detect on them you are inside the sandbox.

> The buildbots don't seem to use that.

right, it sems they don't ... and maybe they should because we have lot of ifdef code there to call the steps InstallGtkDependencies() for "platform == gtk". And instead of that we could maybe just simply pass this parameter to build-webkit instead of adding a previous step to call update-webkitgtk-libs script ¯\_(ツ)_/¯

[Philippe Normand]

(In reply to Carlos Alberto Lopez Perez from comment #94)
> (In reply to Philippe Normand from comment #93)
> > Are these --update-gtk and --update-wpe switches still needed? 
> 
> No idea if some developer uses them.
> 
> but fixing it may be simple? you can exit with zero status early on the
> scripts update-webkitwpe-libs/update-webkitgtk-libs if you detect on them
> you are inside the sandbox.
> 

I'm not sure it's simple. We could use flatpak-spawn maybe but then updating the sysroot while already inside the sandbox might be a foot-gun.

I think that ideally the update script should be called before entering the sandbox, so by checking the argument in build-webkit.

> > The buildbots don't seem to use that.
> 
> right, it sems they don't ... and maybe they should because we have lot of
> ifdef code there to call the steps InstallGtkDependencies() for "platform ==
> gtk". And instead of that we could maybe just simply pass this parameter to
> build-webkit instead of adding a previous step to call update-webkitgtk-libs
> script ¯\_(ツ)_/¯

Yeah maybe, let's do that in follow-up though? This patch is already been going on for months :)

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #95)
> Yeah maybe, let's do that in follow-up though? This patch is already been
> going on for months :)

Yes, sure.. or even not do it.. at this point we already have all that code if-defed

Also it seems fine to me to remove support for "--update-gtk" on build-webkit if fixing it is complicated. That can be re-added later if someone misses it or someone wants to take on unifying the code for the buildbots and remove the extra step for installgtkdependencies.

[Philippe Normand]

Created attachment 393236 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 393236 [details]
Patch

r=me
I set cq- because this patch changes the default method for handling third-party libraries and can cause big surprise to developers.
So I think we should send a mail to webkit-dev (or both gtk-dev+wpe-dev) webkit.org mailing lists telling about this before landing it (and at the same time encouraging everyone to test it and report any issue found with it)
Its also a good idea telling that Ubuntu LTS users can find the required flatpak version on that ppa, and debian stable users have the required version on the backports repository.
Thanks for all!

[Philippe Normand]

https://lists.webkit.org/pipermail/webkit-dev/2020-March/031147.html :)

[Philippe Normand]

Committed r258626: <https://trac.webkit.org/changeset/258626>

[Radar WebKit Bug Importer]

<rdar://problem/60577737>

[Philippe Normand]

*** Bug 205656 has been marked as a duplicate of this bug. ***

[Ryan Haddad]

(In reply to Philippe Normand from comment #100)
> Committed r258626: <https://trac.webkit.org/changeset/258626>
This has broken macOS test262 bots: https://bugs.webkit.org/show_bug.cgi?id=209238

[David Kilzer (:ddkilzer)]

I'm not sure how, but this may have caused:

Bug 209455: [Win] http/tests/misc/last-modified-parsing.html always fails on Windows EWS
<https://bugs.webkit.org/show_bug.cgi?id=209455>

[Philippe Normand]

Comment on attachment 393236 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:896
> +            'TZ',

This might be the cause of the regression, David. If this variable has any effect on windows bots anyway.