[GStreamer][GTK][WPE] update-webkit-libs-jhbuild fails to detect changes in included moduleset files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Nael Ouedraogo]

[GStreamer][GTK][WPE] update-webkit-libs-jhbuild fails to detect changes in included moduleset files. For instance, it cannot detect modification of gstreamer/jhbuild.modules when updating dependencies of WebKitGtk.

[Michael Catanzaro]

This regressed when we split GStreamer stuff into a separate moduleset file (that update-webkit-libs-jhbuild does not know about).

[Adrian Perez]

Created attachment 323529 [details]
Patch

Tentative patch. Note that my Perl-fu is close to non-existant.

(But at least this did not seem to break anything locally for me.)

[Nael Ouedraogo]

I discussed with Michael on this by email and he told me that we don't want to have to modify the perl script every time we add a new moduleset file. So maybe we should find another fix. 

In the meantime I prepared a patch that parses jhbuild.modules to determine if other jhbuild.modules are included. If any, it computes MD5 sum for each included jhbuild.modules file. If you think it is a good way to fix this bug, I can upload the patch.

[Adrian Perez]

(In reply to Nael Ouedraogo from comment #3)

> I discussed with Michael on this by email and he told me that we don't want
> to have to modify the perl script every time we add a new moduleset file. So
> maybe we should find another fix. 

Well, not having to touch the Perl is certainly nicer.
 
> In the meantime I prepared a patch that parses jhbuild.modules to determine
> if other jhbuild.modules are included. If any, it computes MD5 sum for each
> included jhbuild.modules file. If you think it is a good way to fix this
> bug, I can upload the patch.

That sounds certainly *much better* than the patch I have posted earlier.
Thanks a lot for working on it!

[Adrian Perez]

Comment on attachment 323529 [details]
Patch

Marking my patch as obsolete, as Nael's solution will be much better.

[Nael Ouedraogo]

Created attachment 323535 [details]
Patch

[Xabier Rodríguez Calvar]

I'd prefer somebody with better Perl-fu could review this but the main ideas lgtm.

[Carlos Alberto Lopez Perez]

Comment on attachment 323535 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:49
> +use XML::LibXML;

This is a new runtime dependency. It should be added to the scripts Tools/gtk/install-dependencies and Tools/wpe/install-dependencies.
The name of the package in Debian is libxml-libxml-perl
I will be installing it on the bots so the EWS don't fail anymore due to this

[Adrian Perez]

(In reply to Carlos Alberto Lopez Perez from comment #8)
> Comment on attachment 323535 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323535&action=review
> 
> > Tools/Scripts/webkitdirs.pm:49
> > +use XML::LibXML;
> 
> This is a new runtime dependency. It should be added to the scripts
> Tools/gtk/install-dependencies and Tools/wpe/install-dependencies.
> The name of the package in Debian is libxml-libxml-perl
> I will be installing it on the bots so the EWS don't fail anymore due to this

For Arch Linux the package is “perl-xml-libxml”:

  https://www.archlinux.org/packages/extra/x86_64/perl-xml-libxml/

[Michael Catanzaro]

Comment on attachment 323535 [details]
Patch

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

r=me if you also update the two install-dependencies scripts. Thanks!

>>> Tools/Scripts/webkitdirs.pm:49
>>> +use XML::LibXML;
>> 
>> This is a new runtime dependency. It should be added to the scripts Tools/gtk/install-dependencies and Tools/wpe/install-dependencies.
>> The name of the package in Debian is libxml-libxml-perl
>> I will be installing it on the bots so the EWS don't fail anymore due to this
> 
> For Arch Linux the package is “perl-xml-libxml”:
> 
>   https://www.archlinux.org/packages/extra/x86_64/perl-xml-libxml/

The package name variations for this on the different distros are incredible... I believe the correct Fedora package is "perl-libxml-perl".

> Tools/Scripts/webkitdirs.pm:1888
> +sub getJhbuildIncludedFilePath

I think this should be called "getJhbuildIncludedFilePaths" since it returns multiple paths.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #10)
> The package name variations for this on the different distros are
> incredible... I believe the correct Fedora package is "perl-libxml-perl".

Not to be confused with Debian's libxml-libxml-perl and Arch's perl-xml-libxml....

[Nael Ouedraogo]

Created attachment 324009 [details]
Patch

[Nael Ouedraogo]

Thanks for all reviews and for helping me to find the good lib names for each distribution. 

In uploaded patch, I moved getJhbuildIncludedFilePaths sub from webkitdirs.pm to update-webkit-libs-jhbuild script to avoid issues when other ports (win port) use webkitdir.pm and perl-libXML is not installed. I also updated install-dependencies.

[WebKit Commit Bot]

Comment on attachment 324009 [details]
Patch

Clearing flags on attachment: 324009

Committed r223536: <https://trac.webkit.org/changeset/223536>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35028586>

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #10)
> The package name variations for this on the different distros are
> incredible... I believe the correct Fedora package is "perl-libxml-perl".

Wrong... it is perl-XML-LibXML.

I don't pretend to understand

[Michael Catanzaro]

Committed r223943: <https://trac.webkit.org/changeset/223943>