Bug 221991 - [RFC] Remove many webkit-patch commands
Summary: [RFC] Remove many webkit-patch commands
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 227980 222058 222106 222617 222745 231032
Blocks:
  Show dependency treegraph
 
Reported: 2021-02-16 13:20 PST by Sam Sneddon [:gsnedders]
Modified: 2021-09-30 11:30 PDT (History)
20 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Sneddon [:gsnedders] 2021-02-16 13:20:34 PST
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
Comment 1 Sam Sneddon [:gsnedders] 2021-02-17 11:54:56 PST
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!
Comment 2 Aakash Jain 2021-02-17 11:58:51 PST
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
Comment 3 Simon Fraser (smfr) 2021-02-17 12:42:27 PST
I see 'print-expectations' here, but that's a run-webkit-tests command, not a webkit-patch command. I also use it frequently.
Comment 4 Fujii Hironori 2021-02-17 12:51:30 PST
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
Comment 5 Jonathan Bedard 2021-02-17 13:07:22 PST
(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)
Comment 6 Simon Fraser (smfr) 2021-02-17 13:13:22 PST
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.
Comment 7 Simon Fraser (smfr) 2021-02-17 13:14:07 PST
Oh, it's a command, not an argument. Fine.
Comment 8 Brent Fulgham 2021-02-17 17:25:56 PST
I 100% object to removing "webkit-patch clean". I use it many times a day, every day.
Comment 9 Aakash Jain 2021-02-17 17:42:56 PST
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
Comment 10 Carlos Alberto Lopez Perez 2021-02-18 17:09:15 PST
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
Comment 11 Patrick Angle 2021-02-18 18:57:18 PST
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_.
Comment 12 Sam Weinig 2021-02-18 20:03:36 PST
I use `webkit-patch post` for every patch.
Comment 13 Jonathan Bedard 2021-02-19 09:14:26 PST
(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?
Comment 14 Jonathan Bedard 2021-02-19 09:17:47 PST
(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.
Comment 15 Peng Liu 2021-02-20 10:58:36 PST
I frequently use "webkit-patch prepare" to file bugs and update(refresh) change logs.
Comment 16 Diego Pino 2021-02-21 22:35:27 PST
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).
Comment 17 Philippe Normand 2021-02-22 04:29:04 PST
I still use mark-bug-fixed.
Comment 18 Jonathan Bedard 2021-02-22 08:58:06 PST
(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.
Comment 19 Dean Jackson 2021-02-22 15:08:30 PST
+1 to mark-bug-fixed
+1 to prepare
Comment 20 Ryosuke Niwa 2021-02-22 15:55:21 PST
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.
Comment 21 Jonathan Bedard 2021-02-22 16:10:24 PST
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`
Comment 22 Diego Pino 2021-02-22 19:37:56 PST
(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`.
Comment 23 Ryosuke Niwa 2021-02-22 19:45:28 PST
I also use `webkit-patch clean` a lot.
Comment 24 Radar WebKit Bug Importer 2021-02-23 13:21:13 PST
<rdar://problem/74657982>
Comment 25 Wenson Hsieh 2021-02-23 16:11:37 PST
(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.
Comment 26 Jer Noble 2021-02-24 21:23:32 PST
+1 for `webkit-patch prepare`
Comment 27 Carlos Alberto Lopez Perez 2021-03-03 17:51:08 PST
(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.
Comment 28 Sam Sneddon [:gsnedders] 2021-03-04 09:52:02 PST
(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!).
Comment 29 Ross Kirsling 2021-03-22 10:44:22 PDT
I make use of `land-cowhand` every time I fix a broken build.
Comment 30 Sam Sneddon [:gsnedders] 2021-07-15 00:17:18 PDT
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.
Comment 31 Ryosuke Niwa 2021-07-15 00:26:21 PDT
(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?
Comment 32 Sam Sneddon [:gsnedders] 2021-07-15 05:34:02 PDT
(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
Comment 33 zalan 2021-09-06 20:38:51 PDT
I use rebaseline-server occasionally.