Bug 221991 – [RFC] Remove many webkit-patch commands

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Sam Sneddon [:gsnedders]]

We have a lot of commands under webkit-patch, many of which exist primarily for the sake of EWS, though EWS doesn't use them any more. I'd like to avoid having to update many of them when refactoring code elsewhere in webkitpy.

I'm largely filing this to have somewhere to ask people to post what commands they do use, especially from forthcoming deprecation notices that I intend on adding.

Specifically, I'd like to get rid of all the build commands (build, build-and-test, build-and-test-attachment, build-attachment) as well as the --build and --test arguments for all the land commands (land, land-attachment, land-cowboy, land-cowhand, land-from-bug, land-from-url, land-safely).

Jonathan had previously said his first starting point was going to be all commands not shown in the default help. These are:

add-users-to-groups
analyze-changelog
apply-watchlist
apply-watchlist-local
assign-to-committer
attach-to-bug
bug-for-test
bug-search
bugs-to-commit
build
build-and-test
build-and-test-attachment
build-attachment
check-patch-relevance
check-style
check-style-local
clean
clean-pending-commit
clean-review-queue
commit-message
crash-log
create-bug
create-revert
create-rollout
failure-reason
find-flaky-tests
find-resolved-bugs
find-users
garden-o-matic
gtk-wk2-ews
has-landed
help
ios-ews
ios-sim-ews
jsc-armv7-ews
jsc-ews
jsc-i386-ews
jsc-mips-ews
land-cowboy
land-cowhand
land-from-url
mac-debug-ews
mac-ews
mac-wk2-ews
mark-bug-fixed
obsolete-attachments
open-bugs
patches-in-commit-queue
patches-to-commit-queue
patches-to-review
post
post-commits
prepare
prepare-revert
prepare-rollout
print-baselines
print-expectations
rebaseline
rebaseline-expectations
rebaseline-json
rebaseline-server
rebaseline-test-internal
results-for
rollout
setup-git-clone
suggest-nominations
suggest-reviewers
tree-status
update
validate-changelog
what-broke
win-ews
wincairo-ews
wpe-ews
wpt-change-export

[Sam Sneddon [:gsnedders]]

CCing basically everyone who's touched this code in recent years and who presumably cares about it.

I'm pretty sure the above list is _too_ complete, and some of those need kept (e.g., at least some of the rebaselining commands?), but I'd be more than happy to be told which of those do actually get used!

[Aakash Jain]

current EWS uses two of these commands:

apply-watchlist-local is used by current EWS in https://github.com/WebKit/WebKit/blob/main/Tools/CISupport/ews-build/steps.py#L2943

validate-changelog is used by current EWS in https://github.com/WebKit/WebKit/blob/main/Tools/CISupport/ews-build/steps.py#L900

[Simon Fraser (smfr)]

I see 'print-expectations' here, but that's a run-webkit-tests command, not a webkit-patch command. I also use it frequently.

[Fujii Hironori]

I'm using prepare-revert and check-style.

WebKitBot is using create-revert.
https://github.com/WebKit/WebKit/blob/3d2a79d4c0562fc0f2ea2c3198d6280ec55abc0f/Tools/WebKitBot/src/WebKitBot.mjs#L393

[Jonathan Bedard]

(In reply to Simon Fraser (smfr) from comment #3)
> I see 'print-expectations' here, but that's a run-webkit-tests command, not
> a webkit-patch command. I also use it frequently.

Inexplicably, `print-expectations` is also a webkit-patch command....

webkit-patch --all-commands | grep print-expectations
   print-expectations          Print the expected result for the given test(s) on the given port(s)

[Simon Fraser (smfr)]

OpenSource % ./Tools/Scripts/webkit-patch --print-expectations
Usage: webkit-patch [options] COMMAND [ARGS]

webkit-patch: error: no such option: --print-expectations

Type "webkit-patch --help" to see usage.

[Simon Fraser (smfr)]

Oh, it's a command, not an argument. Fine.

[Brent Fulgham]

I 100% object to removing "webkit-patch clean". I use it many times a day, every day.

[Aakash Jain]

It's fine to delete all these (ews related):

gtk-wk2-ews
ios-ews
ios-sim-ews
jsc-armv7-ews
jsc-ews
jsc-i386-ews
jsc-mips-ews
mac-debug-ews
mac-ews
mac-wk2-ews
win-ews
wincairo-ews
wpe-ews

[Carlos Alberto Lopez Perez]

I find the following commands useful:

> check-style
> check-style-local

I use this two ones to check the style before uploading the patch

> help

I guess we want to keep help

> rollout

I have used this to rollout patches.

> setup-git-clone

I have used this to setup the git clone from svn

> suggest-nominations
> suggest-reviewers

This two are useful. Specially the suggest-nominations one to check new comitter/reviewers that are candidate to be nominated

[Patrick Angle]

I've been using `webkit-patch prepare --update-changelogs 222006` regularly. I remember starting with `prepare-ChangeLog -b 222006` before  moving to `...prepare...` and can't remember why I switched.
I've also been eyeing `has-landed` to help me manage the proliferation of git branches on my machine, but it's not part of my workflow _yet_.

[Sam Weinig]

I use `webkit-patch post` for every patch.

[Jonathan Bedard]

(In reply to Carlos Alberto Lopez Perez from comment #10)
> I find the following commands useful:
> 
> > check-style
> > check-style-local

What are the reasons to use those commands over check-webkit-style?

[Jonathan Bedard]

(In reply to Sam Weinig from comment #12)
> I use `webkit-patch post` for every patch.

Would seem that `webkit-patch post` is just an alias for `webkit-patch upload` then.

[Peng Liu]

I frequently use "webkit-patch prepare" to file bugs and update(refresh) change logs.

[Diego Pino]

Some webkit-patch commands I regularly use are:

- land-cowhand

for landing patches that do not require review such as build fixes of test gardening patches. I'm fine with removing this command if there's an alternative for doing the same thing.

- mark-bug-fixed

for automatically closing bugs that have been fixed (for instance, I used it often for closing bugs reporting LayoutTests failures which are not in TestExpectations anymore).

[Philippe Normand]

I still use mark-bug-fixed.

[Jonathan Bedard]

(In reply to Diego Pino from comment #16)
> Some webkit-patch commands I regularly use are:
> 
> - land-cowhand
> 
> for landing patches that do not require review such as build fixes of test
> gardening patches. I'm fine with removing this command if there's an
> alternative for doing the same thing.
> 
> - mark-bug-fixed
> 
> for automatically closing bugs that have been fixed (for instance, I used it
> often for closing bugs reporting LayoutTests failures which are not in
> TestExpectations anymore).

Any reason you're using `land-cowhand` over `land`? My understanding was that `land-cowhand` skipped the pre-commit tests, but `land` no longer has pre-commit tests in it.

[Dean Jackson]

+1 to mark-bug-fixed
+1 to prepare

[Ryosuke Niwa]

I do use setup-git-clone all the time since I can never remember all the random git configuration commands I need to run to setup a WebKit checkout.

[Jonathan Bedard]

Some folks reached out and `webkit-patch setup-git-clone` is not entirely duplicated by `git-webkit setup-git-svn`, so we need to keep that around for a bit while we move the parts of it that we want to keep to `git-webkit`

[Diego Pino]

(In reply to Jonathan Bedard from comment #18)

> Any reason you're using `land-cowhand` over `land`? My understanding was
> that `land-cowhand` skipped the pre-commit tests, but `land` no longer has
> pre-commit tests in it.

No other reason than tradition. When I asked other more experienced WebKit developers how I could land a patch without filing a bug I was taught `land-cowhand` and I've been keep using it since then for that kind of task.

If it's actually better to use `land` I will stop using `land-cowhand`.

[Ryosuke Niwa]

I also use `webkit-patch clean` a lot.

[Radar WebKit Bug Importer]

<rdar://problem/74657982>

[Wenson Hsieh]

(In reply to Diego Pino from comment #22)
> (In reply to Jonathan Bedard from comment #18)
> 
> > Any reason you're using `land-cowhand` over `land`? My understanding was
> > that `land-cowhand` skipped the pre-commit tests, but `land` no longer has
> > pre-commit tests in it.
> 
> No other reason than tradition. When I asked other more experienced WebKit
> developers how I could land a patch without filing a bug I was taught
> `land-cowhand` and I've been keep using it since then for that kind of task.
> 
> If it's actually better to use `land` I will stop using `land-cowhand`.

FWIW, I generally prefer `land-cowhand` over `land`, since it automatically generates and presents a pretty-printed version of the patch that I can check for sanity.

[Jer Noble]

+1 for `webkit-patch prepare`

[Carlos Alberto Lopez Perez]

(In reply to Jonathan Bedard from comment #13)
> (In reply to Carlos Alberto Lopez Perez from comment #10)
> > I find the following commands useful:
> > 
> > > check-style
> > > check-style-local
> 
> What are the reasons to use those commands over check-webkit-style?

Mmm.. I didn't knew about the check-webkit-style script
That works for me as well.

[Sam Sneddon [:gsnedders]]

(In reply to Carlos Alberto Lopez Perez from comment #10)
> > rollout
> 
> I have used this to rollout patches.

Note that this has been marked as deprecated since bug 208775; this is not a new deprecation.

---

Bug 222745 should undeprecate the rest of everything mentioned above; still interested in anything else that people run into (especially more infrequently!).

[Ross Kirsling]

I make use of `land-cowhand` every time I fix a broken build.

[Sam Sneddon [:gsnedders]]

FYI: I've filed Bug 227980 for some of the land-cowhand v. land discussion above.

Aside from land-cowhand pending that other bug, my intention is to remove everything currently marked as deprecated at the end of this month.

[Ryosuke Niwa]

(In reply to Sam Sneddon [:gsnedders] from comment #30)
> FYI: I've filed Bug 227980 for some of the land-cowhand v. land discussion
> above.
> 
> Aside from land-cowhand pending that other bug, my intention is to remove
> everything currently marked as deprecated at the end of this month.

What are things currently marked as deprecated?

[Sam Sneddon [:gsnedders]]

(In reply to Ryosuke Niwa from comment #31)
> (In reply to Sam Sneddon [:gsnedders] from comment #30)
> > FYI: I've filed Bug 227980 for some of the land-cowhand v. land discussion
> > above.
> > 
> > Aside from land-cowhand pending that other bug, my intention is to remove
> > everything currently marked as deprecated at the end of this month.
> 
> What are things currently marked as deprecated?

They are:

add-users-to-groups
analyze-changelog
apply-watchlist
assign-to-committer
attach-to-bug
bug-for-test
bug-search
bugs-to-commit
build
build-attachment
check-patch-relevance
clean-pending-commit
clean-review-queue
commit-message
crash-log
create-bug
create-rollout
failure-reason
find-flaky-tests
find-resolved-bugs
find-users
garden-o-matic
has-landed
land-cowboy
land-cowhand
land-from-url
obsolete-attachments
open-bugs
patches-in-commit-queue
patches-to-commit-queue
patches-to-review
post-commits
prepare-rollout
print-baselines
print-expectations
rebaseline
rebaseline-expectations
rebaseline-json
rebaseline-server
rebaseline-test-internal
results-for
rollout
tree-status
update
what-broke
wpt-change-export

[zalan]

I use rebaseline-server occasionally.

[Antti Koivisto]

I just used rebaseline-server

[Ben Schwartz]

Obligatory "just used rebaseline-server"

[Sam Sneddon [:gsnedders]]

Pull request: https://github.com/WebKit/WebKit/pull/21363

[EWS]

Committed 271597@main (7eae473bde06): <https://commits.webkit.org/271597@main>

Reviewed commits have been landed. Closing PR #21363 and removing active labels.

[Jonathan Bedard]

Needed to be reverted because it broke all webkit-patch commands.

[Sam Sneddon [:gsnedders]]

(In reply to Jonathan Bedard from comment #38)
> Needed to be reverted because it broke all webkit-patch commands.

Reverted in bug 265939.

[Sam Sneddon [:gsnedders]]

Pull request: https://github.com/WebKit/WebKit/pull/21406

[EWS]

Committed 271648@main (ba8968d7e2b3): <https://commits.webkit.org/271648@main>

Reviewed commits have been landed. Closing PR #21406 and removing active labels.