| Summary: | [RFC] Remove many webkit-patch commands | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Sam Sneddon [:gsnedders] <gsnedders> |
| Component: | Tools / Tests | Assignee: | Sam Sneddon [:gsnedders] <gsnedders> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | aakash_jain, ap, ben_schwartz, bfulgham, clopez, dino, dpino, Hironori.Fujii, jbedard, jean-yves.avenard, jer.noble, koivisto, pangle, peng.liu6, pnormand, rniwa, ross.kirsling, sam, simon.fraser, webkit-bug-importer, wenson_hsieh, zalan |
| Priority: | P2 | Keywords: | InRadar |
| Version: | WebKit Nightly Build | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=226230 | ||
| Bug Depends on: | 265939, 222058, 222106, 222617, 222745, 227980, 231032 | ||
| Bug Blocks: | |||
|
Description
Sam Sneddon [:gsnedders]
2021-02-16 13:20:34 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! 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 I see 'print-expectations' here, but that's a run-webkit-tests command, not a webkit-patch command. I also use it frequently. 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 (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) 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. Oh, it's a command, not an argument. Fine. I 100% object to removing "webkit-patch clean". I use it many times a day, every day. 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 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 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_. I use `webkit-patch post` for every patch. (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? (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. I frequently use "webkit-patch prepare" to file bugs and update(refresh) change logs. 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). I still use mark-bug-fixed. (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. +1 to mark-bug-fixed +1 to prepare 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. 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` (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`. I also use `webkit-patch clean` a lot. (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. +1 for `webkit-patch prepare` (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. (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!). I make use of `land-cowhand` every time I fix a broken build. 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. (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? (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 I use rebaseline-server occasionally. I just used rebaseline-server Obligatory "just used rebaseline-server" Pull request: https://github.com/WebKit/WebKit/pull/21363 Committed 271597@main (7eae473bde06): <https://commits.webkit.org/271597@main> Reviewed commits have been landed. Closing PR #21363 and removing active labels. Needed to be reverted because it broke all webkit-patch commands. (In reply to Jonathan Bedard from comment #38) > Needed to be reverted because it broke all webkit-patch commands. Reverted in bug 265939. Pull request: https://github.com/WebKit/WebKit/pull/21406 Committed 271648@main (ba8968d7e2b3): <https://commits.webkit.org/271648@main> Reviewed commits have been landed. Closing PR #21406 and removing active labels. |