Bug 79673

Summary: Parametrize run-with-jhbuild and update-webkitgtk-libs with platform --gtk/--efl
Product: WebKit Reporter: Dominik Röttsches (drott) <d-r>
Component: Tools / TestsAssignee: Dominik Röttsches (drott) <d-r>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, gns, gyuyoung.kim, mrobinson, ojan, pnormand, rakuco, tmpsantos, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Linux   
Bug Depends on:    
Bug Blocks: 79480, 79904, 80025, 80026    
Attachments:
Description Flags
Proposal for patching jhbuild related scripts
none
Proposal for patching jhbuild related scripts, v2 fixing EWS error.
none
Proposal for patching jhbuild related scripts, v3, script-wrappers.
none
Proposal for patching jhbuild related scripts, v4
pnormand: review-, pnormand: commit-queue-
Proposal for patching jhbuild related scripts, v5 none

Description Dominik Röttsches (drott) 2012-02-27 07:47:51 PST
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.
Comment 1 Dominik Röttsches (drott) 2012-02-27 09:23:02 PST
Created attachment 129052 [details]
Proposal for patching jhbuild related scripts
Comment 2 Gustavo Noronha (kov) 2012-02-27 11:36:11 PST
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
Comment 3 Dominik Röttsches (drott) 2012-02-28 00:20:04 PST
Created attachment 129203 [details]
Proposal for patching jhbuild related scripts, v2 fixing EWS error.
Comment 4 Gustavo Noronha (kov) 2012-02-28 04:41:55 PST
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.
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-02-28 04:51:48 PST
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).
Comment 6 Thiago Marcos P. Santos 2012-02-28 05:12:26 PST
Agree, it should respect WEBKITOUTPUTDIR.
Comment 7 Martin Robinson 2012-02-28 06:43:39 PST
(In reply to comment #6)
> Agree, it should respect WEBKITOUTPUTDIR.

This will require two jhbuild trees for a Debug and Release build.
Comment 8 Dominik Röttsches (drott) 2012-02-28 07:09:47 PST
Created attachment 129242 [details]
Proposal for patching jhbuild related scripts, v3, script-wrappers.
Comment 9 Dominik Röttsches (drott) 2012-02-28 07:11:49 PST
(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.
Comment 10 Dominik Röttsches (drott) 2012-02-28 07:13:21 PST
(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.
Comment 11 Gyuyoung Kim 2012-02-28 16:41:16 PST
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.
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-02-28 16:47:10 PST
(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.
Comment 13 Raphael Kubo da Costa (:rakuco) 2012-03-04 17:15:48 PST
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.
Comment 14 Dominik Röttsches (drott) 2012-03-14 08:04:54 PDT
Created attachment 131848 [details]
Proposal for patching jhbuild related scripts, v4

Copyright kept, remaining style nits addressed.
Comment 15 Dominik Röttsches (drott) 2012-03-14 08:06:14 PDT
(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.
Comment 16 Philippe Normand 2012-03-15 01:30:18 PDT
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.
Comment 17 Dominik Röttsches (drott) 2012-03-15 02:57:04 PDT
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.
Comment 18 Philippe Normand 2012-03-15 03:03:07 PDT
Comment on attachment 132008 [details]
Proposal for patching jhbuild related scripts, v5

Looks good, I'll cq+ after EWS turned green.
Comment 19 Raphael Kubo da Costa (:rakuco) 2012-03-15 07:59:12 PDT
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>
Comment 20 Raphael Kubo da Costa (:rakuco) 2012-03-15 07:59:22 PDT
All reviewed patches have been landed.  Closing bug.