Bug 73230 - ews bots should pass --force to update-webkit-chromium
: ews bots should pass --force to update-webkit-chromium
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-11-28 11:01 PST by
Modified: 2011-11-28 14:53 PST (History)


Attachments
Patch (7.29 KB, patch)
2011-11-28 11:04 PST, Tony Chang
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.95 KB, patch)
2011-11-28 11:37 PST, Tony Chang
abarth: review+
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-28 11:01:46 PST
ews bots should pass --force to update-webkit-chromium
------- Comment #1 From 2011-11-28 11:04:59 PST -------
Created an attachment (id=116785) [details]
Patch
------- Comment #2 From 2011-11-28 11:10:28 PST -------
(From update of attachment 116785 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=116785&action=review

> Tools/Scripts/webkitdirs.pm:970
> +    if ($isChromium) {
> +        $forceChromiumUpdate = checkForArgumentAndRemoveFromARGV("--force");
> +    }

Should we use a more specific flag name?  Some script that calls determineIsChromium() might use --force for something else.

> Tools/Scripts/webkitdirs.pm:2020
> -        system("perl", "Tools/Scripts/update-webkit-chromium") == 0 or die $!;
> +        system("perl", "Tools/Scripts/update-webkit-chromium", "--force") == 0 or die $!;

Should this be conditional on forceChromiumUpdate() ?

> Tools/Scripts/webkitpy/tool/steps/update.py:51
> +        update_command = self._tool.port().update_webkit_command()
> +        if self._options.non_interactive and "--chromium" in update_command:
> +            update_command.append("--force")

Generally we like to encapsulate knowledge about the various ports behind the port abstraction.  Can we pass a bool to update_webkit_command and have the ChromiumPort add this flag?
------- Comment #3 From 2011-11-28 11:37:43 PST -------
Created an attachment (id=116792) [details]
Patch
------- Comment #4 From 2011-11-28 11:39:59 PST -------
(In reply to comment #2)
> (From update of attachment 116785 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116785&action=review
> 
> > Tools/Scripts/webkitdirs.pm:970
> > +    if ($isChromium) {
> > +        $forceChromiumUpdate = checkForArgumentAndRemoveFromARGV("--force");
> > +    }
> 
> Should we use a more specific flag name?  Some script that calls determineIsChromium() might use --force for something else.

Sure, renamed to --force-update.

> > Tools/Scripts/webkitdirs.pm:2020
> > -        system("perl", "Tools/Scripts/update-webkit-chromium") == 0 or die $!;
> > +        system("perl", "Tools/Scripts/update-webkit-chromium", "--force") == 0 or die $!;
> 
> Should this be conditional on forceChromiumUpdate() ?

No, this is for when someone calls build-webkit --chromium --update-chromium, which doesn't call determineIsChromium().  Even if it did, it would then need to be build-webkit --chromium --update-chromium --force-update, which seems redundant.

> > Tools/Scripts/webkitpy/tool/steps/update.py:51
> > +        update_command = self._tool.port().update_webkit_command()
> > +        if self._options.non_interactive and "--chromium" in update_command:
> > +            update_command.append("--force")
> 
> Generally we like to encapsulate knowledge about the various ports behind the port abstraction.  Can we pass a bool to update_webkit_command and have the ChromiumPort add this flag?

Done.
------- Comment #5 From 2011-11-28 11:42:28 PST -------
(From update of attachment 116792 [details])
Great!  Thanks.
------- Comment #6 From 2011-11-28 12:36:37 PST -------
(From update of attachment 116792 [details])
Rejecting attachment 116792 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:

Usage: update-webkit [options]
  --chromium          also update dependencies of the chromium port
  --make              generate the Makefile-based build system (Chromium only)
  --chromium-android  also update dependencies of the chromium port for Android
  -h|--help           show the help message
  -q|--quiet          pass -q to svn update for quiet updates
  --gyp               generate project files from gyp after update
  --wincairo          also update dependencies of the WinCairo port

Full output: http://queues.webkit.org/results/10643469
------- Comment #7 From 2011-11-28 13:06:57 PST -------
It looks like a version with the patch applied was run against code without the patch applied.

Adam, is that expected?  Should I just land this patch by hand?
------- Comment #8 From 2011-11-28 13:52:35 PST -------
Yeah, you might need to land it by hand.  I haven't thought throw the transition issues, but if we have trouble we can kick things.
------- Comment #9 From 2011-11-28 13:59:54 PST -------
Committed r101279: <http://trac.webkit.org/changeset/101279>
------- Comment #10 From 2011-11-28 14:53:08 PST -------
Committed r101285: <http://trac.webkit.org/changeset/101285>