servoshell: Replace getopts with bpaf for argument parsing
#37194
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Narfinger
force-pushed
the
using-bpaf
branch
2 times, most recently
from
92096de to
e9e6f59
Compare
mrobinson
requested changes
May 30, 2025
mrobinson
changed the title
getopts with bpaf for argument parsing
pacak
reviewed
May 31, 2025
Narfinger
force-pushed
the
using-bpaf
branch
3 times, most recently
from
cae7d2c to
3f1ecde
Compare
pacak
reviewed
Jun 2, 2025
Narfinger
force-pushed
the
using-bpaf
branch
6 times, most recently
from
753850b to
5c4c0f1
Compare
pacak
reviewed
Jun 3, 2025
There was a problem hiding this comment.
Some more potential improvements for user experience:
Right now your "Usage" line contains all the things with full names - a few lines of text. You can remove less important bits with hide_usage [1] - items will still show up in the full help and will work as usual.
Right now everything goes into a single big product type which is a good start, but it might be useful to split that big structure into smaller sections like I'm doing in cargo-show-asm: [2]. Benefits to me as a developer is that it makes it easier to decide where to put a new option and decouples running and rendering parts - I know for sure that anything in cargo options shouldn't affect rendering. Benefits to me as a user is that in --help output I don't have to go though a wall of text and can figure out what section I want first and then look though that section.
Narfinger
force-pushed
the
using-bpaf
branch
5 times, most recently
from
308cb20 to
590e88c
Compare
Narfinger
force-pushed
the
using-bpaf
branch
2 times, most recently
from
78c3f1d to
878ed1f
Compare
mrobinson
requested changes
Aug 22, 2025
|
🔨 Triggering try run (#17432746321) for Linux (Unit Tests, Build libservo, WPT, Bencher), MacOS (Unit Tests, Build libservo), Windows (Unit Tests, Build libservo), Android, OpenHarmony, Lint |
FYI, you can do a full try run in your fork with |
Bencher Report
Narfinger
force-pushed
the
using-bpaf
branch
from
8cfceeb to
2cc65f0
Compare
|
| Branch | 37194/PR |
| Testbed | HUAWEI Mate 60 Pro |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Data | Measure (units) x 1e3 | Latency | milliseconds (ms) | Memory | Bytes | score | Measure (units) |
|---|---|---|---|---|---|---|---|---|
| release/E2E/file:///parse_from_string.html/ | 📈 view plot | 1.74 units x 1e3 | ||||||
| release/E2E/https://www.google.com/JS/gc-heap/admin | 📈 view plot | 26,752.00 | ||||||
| release/E2E/https://www.google.com/JS/gc-heap/decommitted | 📈 view plot | 409,600.00 | ||||||
| release/E2E/https://www.google.com/JS/gc-heap/unused | 📈 view plot | 140,256.00 | ||||||
| release/E2E/https://www.google.com/JS/gc-heap/used | 📈 view plot | 471,968.00 | ||||||
| release/E2E/https://www.google.com/JS/malloc-heap | 📈 view plot | 5,051,251.00 | ||||||
| release/E2E/https://www.google.com/JS/non-heap | 📈 view plot | 262,144.00 | ||||||
| release/E2E/https://www.google.com/LayoutThread/box-tree | 📈 view plot | 107,659.00 | ||||||
| release/E2E/https://www.google.com/LayoutThread/display-list | 📈 view plot | 0.00 | ||||||
| release/E2E/https://www.google.com/LayoutThread/font-context | 📈 view plot | 8,632.00 | ||||||
| release/E2E/https://www.google.com/LayoutThread/fragment-tree | 📈 view plot | 112.00 | ||||||
| release/E2E/https://www.google.com/LayoutThread/stacking-context-tree | 📈 view plot | 14,272.00 | ||||||
| release/E2E/https://www.google.com/LayoutThread/stylist | 📈 view plot | 5,104.00 | ||||||
| release/E2E/https://www.google.com/Load | 📈 view plot | 686.22 ms | ||||||
| release/E2E/https://www.google.com/Resident | 📈 view plot | 180,427,161.00 | ||||||
| release/E2E/https://www.google.com/image-cache | 📈 view plot | 35,760.00 | ||||||
| release/E2E/https://www.google.com/resident-smaps | 📈 view plot | 183,337,779.00 | ||||||
| release/E2E/https://www.servo.org/Load | 📈 view plot | 1,208.94 ms | ||||||
| release/E2E/https://www.servo.org/Resident | 📈 view plot | 260,559,667.00 | ||||||
| release/E2E/https://www.servo.org/resident-smaps | 📈 view plot | 261,590,220.00 | ||||||
| release/Speedometer/Charts-observable-plot | 📈 view plot | 766.83 ms | ||||||
| release/Speedometer/Charts-observable-plot/Dotted | 📈 view plot | 88.76 ms | ||||||
| release/Speedometer/Charts-observable-plot/Dotted/Async | 📈 view plot | 8.86 ms | ||||||
| release/Speedometer/Charts-observable-plot/Dotted/Sync | 📈 view plot | 79.91 ms | ||||||
| release/Speedometer/Charts-observable-plot/Stacked by 20 | 📈 view plot | 375.47 ms | ||||||
| release/Speedometer/Charts-observable-plot/Stacked by 20/Async | 📈 view plot | 16.87 ms | ||||||
| release/Speedometer/Charts-observable-plot/Stacked by 20/Sync | 📈 view plot | 358.59 ms | ||||||
| release/Speedometer/Charts-observable-plot/Stacked by 6 | 📈 view plot | 302.60 ms | ||||||
| release/Speedometer/Charts-observable-plot/Stacked by 6/Async | 📈 view plot | 9.33 ms | ||||||
| release/Speedometer/Charts-observable-plot/Stacked by 6/Sync | 📈 view plot | 293.26 ms | ||||||
| release/Speedometer/Geomean | 📈 view plot | 667.52 ms | ||||||
| release/Speedometer/Iteration-0-Total | 📈 view plot | 811.76 ms | ||||||
| release/Speedometer/Iteration-1-Total | 📈 view plot | 808.45 ms | ||||||
| release/Speedometer/Iteration-2-Total | 📈 view plot | 794.91 ms | ||||||
| release/Speedometer/Iteration-3-Total | 📈 view plot | 960.66 ms | ||||||
| release/Speedometer/Iteration-4-Total | 📈 view plot | 1,069.14 ms | ||||||
| release/Speedometer/Iteration-5-Total | 📈 view plot | 1,064.04 ms | ||||||
| release/Speedometer/Iteration-6-Total | 📈 view plot | 806.81 ms | ||||||
| release/Speedometer/Iteration-7-Total | 📈 view plot | 813.10 ms | ||||||
| release/Speedometer/Iteration-8-Total | 📈 view plot | 977.00 ms | ||||||
| release/Speedometer/Iteration-9-Total | 📈 view plot | 1,077.52 ms | ||||||
| release/Speedometer/Score | 📈 view plot | 1.52 units | ||||||
| release/Speedometer/TodoMVC-Angular | 📈 view plot | 977.04 ms | ||||||
| release/Speedometer/TodoMVC-Angular/Adding100Items | 📈 view plot | 464.78 ms | ||||||
| release/Speedometer/TodoMVC-Angular/Adding100Items/Async | 📈 view plot | 31.51 ms | ||||||
| release/Speedometer/TodoMVC-Angular/Adding100Items/Sync | 📈 view plot | 433.27 ms | ||||||
| release/Speedometer/TodoMVC-Angular/CompletingAllItems | 📈 view plot | 316.68 ms | ||||||
| release/Speedometer/TodoMVC-Angular/CompletingAllItems/Async | 📈 view plot | 35.66 ms | ||||||
| release/Speedometer/TodoMVC-Angular/CompletingAllItems/Sync | 📈 view plot | 281.02 ms | ||||||
| release/Speedometer/TodoMVC-Angular/DeletingAllItems | 📈 view plot | 195.58 ms | ||||||
| release/Speedometer/TodoMVC-Angular/DeletingAllItems/Async | 📈 view plot | 6.93 ms | ||||||
| release/Speedometer/TodoMVC-Angular/DeletingAllItems/Sync | 📈 view plot | 188.65 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES5 | 📈 view plot | 1,390.83 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES5/Adding100Items | 📈 view plot | 1,126.09 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES5/Adding100Items/Async | 📈 view plot | 52.34 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES5/Adding100Items/Sync | 📈 view plot | 1,073.76 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES5/CompletingAllItems | 📈 view plot | 168.19 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES5/CompletingAllItems/Async | 📈 view plot | 38.95 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES5/CompletingAllItems/Sync | 📈 view plot | 129.24 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES5/DeletingAllItems | 📈 view plot | 96.55 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES5/DeletingAllItems/Async | 📈 view plot | 6.79 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES5/DeletingAllItems/Sync | 📈 view plot | 89.76 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES6-Webpack | 📈 view plot | 1,980.41 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES6-Webpack/Adding100Items | 📈 view plot | 1,588.98 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES6-Webpack/Adding100Items/Async | 📈 view plot | 30.15 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES6-Webpack/Adding100Items/Sync | 📈 view plot | 1,558.83 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES6-Webpack/CompletingAllItems | 📈 view plot | 243.27 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES6-Webpack/CompletingAllItems/Async | 📈 view plot | 38.82 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES6-Webpack/CompletingAllItems/Sync | 📈 view plot | 204.45 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES6-Webpack/DeletingAllItems | 📈 view plot | 148.17 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES6-Webpack/DeletingAllItems/Async | 📈 view plot | 7.15 ms | ||||||
| release/Speedometer/TodoMVC-JavaScript-ES6-Webpack/DeletingAllItems/Sync | 📈 view plot | 141.02 ms | ||||||
| release/Speedometer/TodoMVC-Preact | 📈 view plot | 155.08 ms | ||||||
| release/Speedometer/TodoMVC-Preact/Adding100Items | 📈 view plot | 76.87 ms | ||||||
| release/Speedometer/TodoMVC-Preact/Adding100Items/Async | 📈 view plot | 69.77 ms | ||||||
| release/Speedometer/TodoMVC-Preact/Adding100Items/Sync | 📈 view plot | 7.10 ms | ||||||
| release/Speedometer/TodoMVC-Preact/CompletingAllItems | 📈 view plot | 62.40 ms | ||||||
| release/Speedometer/TodoMVC-Preact/CompletingAllItems/Async | 📈 view plot | 50.40 ms | ||||||
| release/Speedometer/TodoMVC-Preact/CompletingAllItems/Sync | 📈 view plot | 12.00 ms | ||||||
| release/Speedometer/TodoMVC-Preact/DeletingAllItems | 📈 view plot | 15.81 ms | ||||||
| release/Speedometer/TodoMVC-Preact/DeletingAllItems/Async | 📈 view plot | 9.58 ms | ||||||
| release/Speedometer/TodoMVC-Preact/DeletingAllItems/Sync | 📈 view plot | 6.23 ms | ||||||
| release/Speedometer/TodoMVC-React | 📈 view plot | 871.91 ms | ||||||
| release/Speedometer/TodoMVC-React-Redux | 📈 view plot | 1,070.63 ms | ||||||
| release/Speedometer/TodoMVC-React-Redux/Adding100Items | 📈 view plot | 344.17 ms | ||||||
| release/Speedometer/TodoMVC-React-Redux/Adding100Items/Async | 📈 view plot | 34.57 ms | ||||||
| release/Speedometer/TodoMVC-React-Redux/Adding100Items/Sync | 📈 view plot | 309.60 ms | ||||||
| release/Speedometer/TodoMVC-React-Redux/CompletingAllItems | 📈 view plot | 475.99 ms | ||||||
| release/Speedometer/TodoMVC-React-Redux/CompletingAllItems/Async | 📈 view plot | 37.64 ms | ||||||
| release/Speedometer/TodoMVC-React-Redux/CompletingAllItems/Sync | 📈 view plot | 438.35 ms | ||||||
| release/Speedometer/TodoMVC-React-Redux/DeletingAllItems | 📈 view plot | 250.47 ms | ||||||
| release/Speedometer/TodoMVC-React-Redux/DeletingAllItems/Async | 📈 view plot | 7.62 ms | ||||||
| release/Speedometer/TodoMVC-React-Redux/DeletingAllItems/Sync | 📈 view plot | 242.85 ms | ||||||
| release/Speedometer/TodoMVC-React/Adding100Items | 📈 view plot | 300.91 ms | ||||||
| release/Speedometer/TodoMVC-React/Adding100Items/Async | 📈 view plot | 36.90 ms | ||||||
| release/Speedometer/TodoMVC-React/Adding100Items/Sync | 📈 view plot | 264.02 ms | ||||||
| release/Speedometer/TodoMVC-React/CompletingAllItems | 📈 view plot | 366.42 ms | ||||||
| release/Speedometer/TodoMVC-React/CompletingAllItems/Async | 📈 view plot | 39.75 ms | ||||||
| release/Speedometer/TodoMVC-React/CompletingAllItems/Sync | 📈 view plot | 326.66 ms | ||||||
| release/Speedometer/TodoMVC-React/DeletingAllItems | 📈 view plot | 204.58 ms | ||||||
| release/Speedometer/TodoMVC-React/DeletingAllItems/Async | 📈 view plot | 7.50 ms | ||||||
| release/Speedometer/TodoMVC-React/DeletingAllItems/Sync | 📈 view plot | 197.08 ms | ||||||
| release/Speedometer/TodoMVC-Svelte | 📈 view plot | 133.98 ms | ||||||
| release/Speedometer/TodoMVC-Svelte/Adding100Items | 📈 view plot | 71.72 ms | ||||||
| release/Speedometer/TodoMVC-Svelte/Adding100Items/Async | 📈 view plot | 55.14 ms | ||||||
| release/Speedometer/TodoMVC-Svelte/Adding100Items/Sync | 📈 view plot | 16.58 ms | ||||||
| release/Speedometer/TodoMVC-Svelte/CompletingAllItems | 📈 view plot | 47.16 ms | ||||||
| release/Speedometer/TodoMVC-Svelte/CompletingAllItems/Async | 📈 view plot | 38.43 ms | ||||||
| release/Speedometer/TodoMVC-Svelte/CompletingAllItems/Sync | 📈 view plot | 8.73 ms | ||||||
| release/Speedometer/TodoMVC-Svelte/DeletingAllItems | 📈 view plot | 15.09 ms | ||||||
| release/Speedometer/TodoMVC-Svelte/DeletingAllItems/Async | 📈 view plot | 10.51 ms | ||||||
| release/Speedometer/TodoMVC-Svelte/DeletingAllItems/Sync | 📈 view plot | 4.58 ms |
|
|
|
Lots of errors from the cli parser it seems. I don't see any obvious reasons for it to fail this way - it could be that |
|
Yeah there are a couple of things that have slightly different parameters than expected. Going to figure them out and post when I get a successful |
Narfinger
force-pushed
the
using-bpaf
branch
3 times, most recently
from
c038ade to
677ead6
Compare
|
Ok finally we should be there. here is also a completed try run, to prove that I didn't forget anything this time :D https://github.com/Narfinger/servo/actions/runs/17458323904/job/49579707316 |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
The PR that will never merge: |
Narfinger
force-pushed
the
using-bpaf
branch
from
677ead6 to
74040aa
Compare
|
Clearly I just wanted to test if the merge queue works -.- |
This includes some small refactoring and some small breaking changes as listed below. Other than these I tried to keep the functionality exactly the same but because in the old code the parsing and settings of preferences was intermingled it was difficult to figure out. Small Breaking: - Size and resources-path were unused but appeared in the help. - soft-fail and hard-fail: Soft-fail flag got removed because it is too difficult to keep both. The default is now soft-fail and hard-fail can be enabled. Thanks to @pacak for all the help! Signed-off-by: Narfinger <Narfinger@users.noreply.github.com>
mukilan
added a commit
to servo/internal-wpt-dashboard
that referenced
this pull request
Sep 8, 2025
Due to servo/servo#37194, the output format of `--version` has changed. Update the regex used accordingly. Signed-off-by: Mukilan Thiyagarajan <mukilan@igalia.com>
mukilan
added a commit
to servo/internal-wpt-dashboard
that referenced
this pull request
Sep 8, 2025
Due to servo/servo#37194, the output format of `--version` has changed. Update the regex used accordingly. Signed-off-by: Mukilan Thiyagarajan <mukilan@igalia.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This includes some small refactoring and some small breaking changes as
listed below. Other than these I tried to keep the functionality exactly
the same but because in the old code the parsing and settings of
preferences was intermingled it was difficult to figure out.
Small Breaking:
difficult to keep both. The default is now soft-fail and hard-fail can
be enabled.
Ideally, we want to have the ServoShellPreferences and Preferences be directly the Argument structure but that needs a bit more discussion because it would break backwards compatibility with the commandline.
This increases the binary size by ~280kb.
Testing: The testcases are still working but they do not cover much.
I added a unit test for the -p flag because it is the most difficult to parse in general.
Fixes: This will fix a small number of various parsing misshaps. It will also show if we are removing an option via unused lint.