Bug 238452 – [Flatpak SDK] Local dependencies override support

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Philippe Normand]

.

[Philippe Normand]

Created attachment 455917 [details]
Patch

[Philippe Normand]

Created attachment 456111 [details]
Patch

[Adrian Perez]

Comment on attachment 456111 [details]
Patch

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

> Tools/flatpak/local-projects/meson.build:2
> +        version : '0.1.0')

Nice trick, using a dummy project with the only goal of letting Meson
handle things for us 🤯️

Even when it works only for Meson projects, that will cover a good amount
of our dependencies these days, including GTK, GLib, glib-networking and a
few others -- we can add later on .wrap files when needed for them.

[Philippe Normand]

Thanks for the review! I wanted to update the patch to allow the user to tune meson options with an env var, it's not working yet (for unknown reasons) but whenever I manage to get it working I'd like to update this patch and ask a new review, if that's OK with you. Or I can follow-up in a new patch.

[Philippe Normand]

(In reply to Adrian Perez from comment #3)

> Even when it works only for Meson projects

It could be made to work with different build-systems as well, Meson has an "externalproject" module that I successfully used in another context: https://mesonbuild.com/External-Project-module.html#external-project-module

[Philippe Normand]

Created attachment 456586 [details]
Patch

[Adrian Perez]

(In reply to Philippe Normand from comment #5)
> (In reply to Adrian Perez from comment #3)
> 
> > Even when it works only for Meson projects
> 
> It could be made to work with different build-systems as well, Meson has an
> "externalproject" module that I successfully used in another context:
> https://mesonbuild.com/External-Project-module.html#external-project-module

Nice!

[Adrian Perez]

Comment on attachment 456586 [details]
Patch

Patch LGTM, with a small nit that would be nice to tackle before landing :)

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

> Tools/flatpak/flatpakutils.py:685
> +            options = [o for o in os.environ.get('WEBKIT_SDK_LOCAL_DEPS_OPTIONS', '').strip().split(' ') if o]

It would be better to use shlex.split() here, to allow passing options
that themselves contain spaces by quoting them inside the environment
variable, e.g:

   export WEBKIT_SDK_LOCAL_DEPS_OPTIONS='-Dsomeproject:someoption="value with spaces"'

[Adrian Perez]

(In reply to Philippe Normand from comment #4)
> Thanks for the review! I wanted to update the patch to allow the user to
> tune meson options with an env var, it's not working yet (for unknown
> reasons) but whenever I manage to get it working I'd like to update this
> patch and ask a new review, if that's OK with you. Or I can follow-up in a
> new patch.

I would say it is up to you: if you prefer to update the patch for this
bug, I can re-review; if you would rather do a follow-up that's fine as
well. The latter approach would allow to get the base functionality in
trunk faster, maybe ;-)

[Philippe Normand]

Created attachment 456689 [details]
[fast-cq] Patch

[EWS]

Committed r292390 (249255@main): <https://commits.webkit.org/249255@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456689 [details].

[Radar WebKit Bug Importer]

<rdar://problem/91286192>