Parametrize run-with-jhbuild and update-webkitgtk-libs with platform --gtk/--efl

[Dominik Röttsches (drott)]

My proposal for solving bug 79480 is to reuse the scripts for jhbuild on the gtk side, and parametrize run-with-jhbuild as well as update-webkitgtk-libs with a --gtk/--efl platform parameter.

Your feedback is welcome whether that's a sensible idea.

[Dominik Röttsches (drott)]

Created attachment 129052 [details]
Proposal for patching jhbuild related scripts

[Gustavo Noronha (kov)]

Comment on attachment 129052 [details]
Proposal for patching jhbuild related scripts

Attachment 129052 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11639162

[Dominik Röttsches (drott)]

Created attachment 129203 [details]
Proposal for patching jhbuild related scripts, v2 fixing EWS error.

[Gustavo Noronha (kov)]

Comment on attachment 129203 [details]
Proposal for patching jhbuild related scripts, v2 fixing EWS error.

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

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:136
> +    command = ["perl", "./Tools/Scripts/update-webkit-libs-jhbuild --gtk"]

How about this: instead of removing update-webkitgtk-libs, keep it as a thin wrapper that calls update-webkitlibs-jhbuild with --gtk? The name of the script seems to be wrong here, btw, there's a dash between webkit and libs. By keeping the update-webkitgtk-libs script you can just remove this change, though.

> Tools/Scripts/update-webkitlibs-jhbuild:56
> +print "Updating GTK+ port dependencies using jhbuild...\n";

You probably want to change GTK+ in here by a variable set in the platform check above?

> Tools/Scripts/webkitdirs.pm:1825
> -    unshift(@buildArgs, "$sourceDir/Tools/gtk/run-with-jhbuild");
> +    unshift(@buildArgs, "--gtk");
> +    unshift(@buildArgs, "$sourceDir/Tools/jhbuild/run-with-jhbuild");

Same for this, I would prefer to keep gtk/run-with-jhbuild as a thin wrapper that calls the other one with the appropriate option.

[Raphael Kubo da Costa (:rakuco)]

I wonder if using WebKitBuild/Dependencies for both gtk and efl can't cause problems when one builds both ports (ie. different checkout revisions etc).

[Thiago Marcos P. Santos]

Agree, it should respect WEBKITOUTPUTDIR.

[Martin Robinson]

(In reply to comment #6)
> Agree, it should respect WEBKITOUTPUTDIR.

This will require two jhbuild trees for a Debug and Release build.

[Dominik Röttsches (drott)]

Created attachment 129242 [details]
Proposal for patching jhbuild related scripts, v3, script-wrappers.

[Dominik Röttsches (drott)]

(In reply to comment #4)

Thanks for your review!

> (From update of attachment 129203 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129203&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:136
> > +    command = ["perl", "./Tools/Scripts/update-webkit-libs-jhbuild --gtk"]
> 
> How about this: instead of removing update-webkitgtk-libs, keep it as a thin wrapper that calls update-webkitlibs-jhbuild with --gtk? The name of the script seems to be wrong here, btw, there's a dash between webkit and libs. By keeping the update-webkitgtk-libs script you can just remove this change, though.

Sounds good to me, thought about this approach as well and wasn't sure which way would be preferred. Done in v3 of the patch.

> 
> > Tools/Scripts/update-webkitlibs-jhbuild:56
> > +print "Updating GTK+ port dependencies using jhbuild...\n";
> 
> You probably want to change GTK+ in here by a variable set in the platform check above?

Done.

> 
> > Tools/Scripts/webkitdirs.pm:1825
> > -    unshift(@buildArgs, "$sourceDir/Tools/gtk/run-with-jhbuild");
> > +    unshift(@buildArgs, "--gtk");
> > +    unshift(@buildArgs, "$sourceDir/Tools/jhbuild/run-with-jhbuild");
> 
> Same for this, I would prefer to keep gtk/run-with-jhbuild as a thin wrapper that calls the other one with the appropriate option.

Done.

[Dominik Röttsches (drott)]

(In reply to comment #5)
> I wonder if using WebKitBuild/Dependencies for both gtk and efl can't cause problems when one builds both ports (ie. different checkout revisions etc).

I would say, we can handle this in a separate bug/patch added as a blocker to bug 79480.

[Gyuyoung Kim]

Comment on attachment 129242 [details]
Proposal for patching jhbuild related scripts, v3, script-wrappers.

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

> Tools/Scripts/update-webkitgtk-libs:-2
> -# Copyright (C) 2011 Igalia S.L.

AFAIK, we need to remain previous copyright.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #11)
> (From update of attachment 129242 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129242&action=review
> 
> > Tools/Scripts/update-webkitgtk-libs:-2
> > -# Copyright (C) 2011 Igalia S.L.
> 
> AFAIK, we need to remain previous copyright.

IANAL, but the copyright just got moved with the bulk of the file contents to the new one.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 129242 [details]
Proposal for patching jhbuild related scripts, v3, script-wrappers.

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

Some additional minor comments while taking a look at the patch again. The style nitpicks come from WebKit following Python's PEP8, but apparently not actively checking for deviations.

> Tools/jhbuild/jhbuild-wrapper:24
> +import sys
> +
> +top_level_dir = None

Style nit: please separate imports and global variables with two blank lines.

> Tools/jhbuild/jhbuild-wrapper:26
> +def top_level_path(*args):

Isn't it possible to use common.top_level_path() here?

> Tools/jhbuild/jhbuild-wrapper:40
> +platform = None;
> +
> +def jhbuild_installed():

Style nit: please separate variables and functions with two empty lines.

> Tools/jhbuild/jhbuild-wrapper:108
> +        raise Exception('jhbuild configure failed with return code: %i' % process.returncode)
> +
> +def determine_platform():

Style nit: please separate functions with two empty lines.

[Dominik Röttsches (drott)]

Created attachment 131848 [details]
Proposal for patching jhbuild related scripts, v4

Copyright kept, remaining style nits addressed.

[Dominik Röttsches (drott)]

(In reply to comment #13)

> > Tools/jhbuild/jhbuild-wrapper:26
> > +def top_level_path(*args):
> 
> Isn't it possible to use common.top_level_path() here?

I am not copying common.py from Tools/gtk to Tools/jhbuild - it didn't seem worthwhile just for using top_level_path.

[Philippe Normand]

Comment on attachment 131848 [details]
Proposal for patching jhbuild related scripts, v4

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

Thanks! This looks good overall but needs another iteration I think. See below:

> Tools/Scripts/update-webkit-libs-jhbuild:41
> +    if ($platformEfl) {
> +        $platform = "efl";
> +        $prettyPlatform = "EFL";
> +    }

I'm no Perl monkey (at all), but isn't there a nicer way to handle the option name with the prettyPlatform? Some kind of mapping/dictionary maybe?

> Tools/jhbuild/jhbuild-wrapper:118
> +    sys.exit("No platform specified for jhbuild-wrapper.")

Please raise a ValueError  here (or an Exception like it's done in the other functions above).

> Tools/jhbuild/jhbuild-wrapper:133
> +platform = determine_platform()

Wrap this in a try/except and sys.exit() if you catch a ValueError.

[Dominik Röttsches (drott)]

Created attachment 132008 [details]
Proposal for patching jhbuild related scripts, v5

Raising ValueError, using hash for pretty printing platform name. 
Additional cleanup on failure exit in update-webkit-libs-jhbuild.

[Philippe Normand]

Comment on attachment 132008 [details]
Proposal for patching jhbuild related scripts, v5

Looks good, I'll cq+ after EWS turned green.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 132008 [details]
Proposal for patching jhbuild related scripts, v5

Clearing flags on attachment: 132008

Committed r110846: <http://trac.webkit.org/changeset/110846>

[Raphael Kubo da Costa (:rakuco)]

All reviewed patches have been landed.  Closing bug.