Bug 73230 – ews bots should pass --force to update-webkit-chromium

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Chang]

ews bots should pass --force to update-webkit-chromium

[Tony Chang]

Created attachment 116785 [details]
Patch

[Adam Barth]

Comment on attachment 116785 [details]
Patch

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?

[Tony Chang]

Created attachment 116792 [details]
Patch

[Tony Chang]

(In reply to comment #2)
> (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.

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.

[Adam Barth]

Comment on attachment 116792 [details]
Patch

Great!  Thanks.

[WebKit Review Bot]

Comment on attachment 116792 [details]
Patch

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

[Tony Chang]

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?

[Adam Barth]

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.

[Tony Chang]

Committed r101279: <http://trac.webkit.org/changeset/101279>

[Tony Chang]

Committed r101285: <http://trac.webkit.org/changeset/101285>