| Summary: | [GTK][WPE] Migrate to Flatpak-based dev SDK | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||||||||||||||||||||||||||||||
| Component: | Tools / Tests | Assignee: | Philippe Normand <pnormand> | ||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | aakash_jain, agomez, aperez, clopez, dbates, dpino, ews-watchlist, fred.wang, glenn, jbedard, ltilve, pgriffis, tsaunier, twilco.o, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=208106 https://bugs.webkit.org/show_bug.cgi?id=209221 https://bugs.webkit.org/show_bug.cgi?id=209238 |
||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 205775 | ||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||
|
Description
Philippe Normand
2019-12-31 11:23:48 PST
Flipping the switch for all bots in one go seems quite a challenge, hence the step-by-step idea... (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. (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 :) (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? 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). (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. (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 :) Created attachment 386766 [details]
Patch
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" 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... Comment on attachment 386766 [details]
Patch
anoter thing I noticed: flatpak packages are missing from Tools/gtk/install-dependencies and the wpe counterpart
(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 (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 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. 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. (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. 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...
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 Great, many thanks Carlos! (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. 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. Created attachment 387513 [details]
Patch
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 :) GTK EWS green now. (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. 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. 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? 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. 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. 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? 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... (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. Created attachment 387674 [details]
Patch
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). 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'? 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 :( Created attachment 387764 [details]
Patch
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. (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? 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? :(
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. (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). (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 :/ (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. 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... 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? 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. Created attachment 387924 [details]
Patch
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.
Created attachment 389122 [details]
Patch
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. Created attachment 389125 [details]
Patch
(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 I'll add the ones explicitly mentioned. Dunno about the etc part :) Created attachment 389255 [details]
Patch
(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. (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 Created attachment 389334 [details]
Patch
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.
(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 :\ (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) (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 Created attachment 389341 [details]
Patch
Created attachment 389532 [details]
Patch
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)
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. 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. (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 :\ Comment on attachment 389532 [details]
Patch
Setting r- to not forget that this version of the patch still doesn't fix run-perf-tests.
Created attachment 391654 [details]
Patch
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. Could we detect old versions in Ubuntu and recommend to install this PPA? https://launchpad.net/~alexlarsson/+archive/ubuntu/flatpak (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 OK so do you want me to bump the version requirement to 1.6.2 then? I see it's available in the PPA. (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. Created attachment 392288 [details]
Patch
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.
(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 (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). (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. (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. (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 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. (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. 👍 Created attachment 393154 [details]
Patch
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" ... Created attachment 393214 [details]
Patch
run-javascriptcore-tests needs a few updates, as detected by EWS. Created attachment 393223 [details]
Patch
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? Created attachment 393225 [details]
Patch
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...
Are these --update-gtk and --update-wpe switches still needed? The buildbots don't seem to use that. (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 ¯\_(ツ)_/¯ (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 :) (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. Created attachment 393236 [details]
Patch
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!
Committed r258626: <https://trac.webkit.org/changeset/258626> *** Bug 205656 has been marked as a duplicate of this bug. *** (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 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> 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. |