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.
Created attachment 129052 [details] Proposal for patching jhbuild related scripts
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
Created attachment 129203 [details] Proposal for patching jhbuild related scripts, v2 fixing EWS error.
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.
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).
Agree, it should respect WEBKITOUTPUTDIR.
(In reply to comment #6) > Agree, it should respect WEBKITOUTPUTDIR. This will require two jhbuild trees for a Debug and Release build.
Created attachment 129242 [details] Proposal for patching jhbuild related scripts, v3, script-wrappers.
(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.
(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 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.
(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 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.
Created attachment 131848 [details] Proposal for patching jhbuild related scripts, v4 Copyright kept, remaining style nits addressed.
(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 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.
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 on attachment 132008 [details] Proposal for patching jhbuild related scripts, v5 Looks good, I'll cq+ after EWS turned green.
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>
All reviewed patches have been landed. Closing bug.