[webkit-reviews] review denied: [Bug 205658] [GTK][WPE] Migrate to Flatpak-based dev SDK : [Attachment 392288] Patch

[bugzilla-daemon at webkit.org]

Carlos Alberto Lopez Perez <clopez at igalia.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 205658: [GTK][WPE] Migrate to Flatpak-based dev SDK
https://bugs.webkit.org/show_bug.cgi?id=205658

Attachment 392288: Patch

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




--- Comment #77 from Carlos Alberto Lopez Perez <clopez at igalia.com> ---
Comment on attachment 392288
  --> https://bugs.webkit.org/attachment.cgi?id=392288
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.