Use actual PR summaries instead of bors placeholders when listing them #379
Conversation
Also expand rollup description to show individual rolled up prs
|
This is an interesting addition. Just like you I also always inspect the rollup content manually. Just one thought. The new output is now (understandably) verbose. Do you think it can be compressed with some clever markdown tricks? Here an example, it's half-baked and does not render great (the Markdown nested syntax is very sensitive to spaces): PRs in range:
ERROR: no CI builds available between 8ace7ea1f7cbba7b4f031e66c54ca237a0d65de6 and 98aa3624be70462d6a25ed5544333e3df62f4c66 within last 167 days |
|
No, I don't think so. Three reasons:
If PR list looks to long, then it's better to hide the entirety of it (and but just parts) under collapsible, IMO. I wouldn't mind adding those collapsible decorations for the entire list in the output by default, if we think it's a good idea. |
|
I don't feel too strongly about my suggestion but I think some form of collapsible could help reducing the clutter for visitors not interested in the bisection. I also totally agree this bisection blurb should avoid tagging PRs in general, that's needless background noise across the repository. I use a github trick to avoid backlinks: Note the "togithub.com" domain. Could it be a solution to post backlinks quietly? In any case thanks for submitting your idea (I like it!) |
src/main.rs
Outdated
| let bors_re = regex::Regex::new( | ||
| r"Auto merge of #(?<pr_num>\d+) - (?<author>[^:]+):.*\n\s*\n(?<pr_summary>.*)", | ||
| ) |
There was a problem hiding this comment.
since this function is called in a loop, regexp instantiation can stay out of the loop for perf. reasons (also the other one down in this function)
There was a problem hiding this comment.
oh, yeah, thanks for cathing this one!
For some reason I was absolutely convinced that regex instantiation is internally cached, and re-creating one in a loop is effectively free. But now that I check that, the docs explicitly say that this is not the case.
There was a problem hiding this comment.
I gave it another thought and decided against it for now.
This is only called like 10 times tops in a normal run (once per PR in a nightly). Removing the regex re-parsing makes the code less pretty, and if perf is a concern, then there are much lower hanging fruit. Namely we create an entire reqwest::blocking::Client (and associated tokio runtime) to fetch the commits descriptions, and I'm confident that it takes way more time than a regex.
Still, I'm address the extra regex copies if desired, but I wanted to push back a little bit for this round.
|
Sorry for having it sitting for a while, this is a bit stale but absolutely not abandoned! |
|
no worries @moxian take your time :) |
|
Added the collapsible + code block. Looks like this now: Regression in nightly-2024-11-20. PRs in range:I struggle to show how it would look in console because codeblock-inside-codeblock no worky, but I'll say that I'm happy with the presentation 🙃 There is maybe an opportunity to add extra syntax highlight, but i'm undecided on whether this is an improvement and what language to use, if any. examplesrust: - #133179 (Rollup of 5 pull requests) by GuillaumeGomez
- #133156 (typo in config.example.toml)
- #133157 (stability: remove skip_stability_check_due_to_privacy)
- #133163 (remove pointless cold_path impl in interpreter)
- #133169 (Update autolabels for T-compiler and T-bootstrap)
- #133171 (Add the missing quotation mark in comment)
- #132460 (Use `TypingMode` throughout the compiler instead of `ParamEnv`) by lcnr
- #124780 (Improve VecCache under parallel frontend) by Mark-Simulacrum
- #133193 (Rollup of 9 pull requests) by fmease
- #132758 (Improve `{BTreeMap,HashMap}::get_key_value` docs.)
- #133180 ([rustdoc] Fix items with generics not having their jump to def link generated)
- #133181 (Update books)
- #133182 (const_panic: inline in bootstrap builds to avoid f16/f128 crashes)
- #133185 (rustdoc-search: use smart binary search in bitmaps)
- #133186 (Document s390x-unknown-linux targets)
- #133187 (Add reference annotations for diagnostic attributes)
- #133191 (rustdoc book: Move `--test-builder(--wrapper)?` docs to unstable section.)
- #133192 (RELEASES.md: Don't document unstable `--test-build-wrapper`)
- #132623 (`rustc_borrowck` cleanups, part 2) by nnethercote
- #133164 (interpret: do not ICE when a promoted fails with OOM) by RalfJung
- #133205 (Rollup of 4 pull requests) by matthiaskrgr
- #131081 (Use `ConstArgKind::Path` for all single-segment paths, not just params under `min_generic_const_args`)
- #132577 (Report the `unexpected_cfgs` lint in external macros)
- #133023 (Merge `-Zhir-stats` into `-Zinput-stats`)
- #133200 (ignore an occasionally-failing test in Miri)
- #132761 (Resolve tweaks) by nnethercotemarkdown - #133179 (Rollup of 5 pull requests) by GuillaumeGomez
- #133156 (typo in config.example.toml)
- #133157 (stability: remove skip_stability_check_due_to_privacy)
- #133163 (remove pointless cold_path impl in interpreter)
- #133169 (Update autolabels for T-compiler and T-bootstrap)
- #133171 (Add the missing quotation mark in comment)
- #132460 (Use `TypingMode` throughout the compiler instead of `ParamEnv`) by lcnr
- #124780 (Improve VecCache under parallel frontend) by Mark-Simulacrum
- #133193 (Rollup of 9 pull requests) by fmease
- #132758 (Improve `{BTreeMap,HashMap}::get_key_value` docs.)
- #133180 ([rustdoc] Fix items with generics not having their jump to def link generated)
- #133181 (Update books)
- #133182 (const_panic: inline in bootstrap builds to avoid f16/f128 crashes)
- #133185 (rustdoc-search: use smart binary search in bitmaps)
- #133186 (Document s390x-unknown-linux targets)
- #133187 (Add reference annotations for diagnostic attributes)
- #133191 (rustdoc book: Move `--test-builder(--wrapper)?` docs to unstable section.)
- #133192 (RELEASES.md: Don't document unstable `--test-build-wrapper`)
- #132623 (`rustc_borrowck` cleanups, part 2) by nnethercote
- #133164 (interpret: do not ICE when a promoted fails with OOM) by RalfJung
- #133205 (Rollup of 4 pull requests) by matthiaskrgr
- #131081 (Use `ConstArgKind::Path` for all single-segment paths, not just params under `min_generic_const_args`)
- #132577 (Report the `unexpected_cfgs` lint in external macros)
- #133023 (Merge `-Zhir-stats` into `-Zinput-stats`)
- #133200 (ignore an occasionally-failing test in Miri)
- #132761 (Resolve tweaks) by nnethercote |
When bisecting issues it often happens that a regression was introduced long enough ago that CI artifacts are unavailable, and we only have a list of PRs between two nightlies to go off of.
The current output only presents a numeric(!) list of PRs, which makes it very cumbersome to inspect those and narrow down. Best way I found was to handle this was to start a new comment on the issue, paste cargo-bisect-rustc output, then go to preview tab, and open all of the hotlinked issue numbers in new tabs.
This can be improved.
We can parse the bors autogenerated commit description and extract the original PR summary, and use that instead of just a number.
We can go even further and explicitly list all of the individual rolled-up PRs in case of rollups.
Output on current master (for mcve in rust-lang/rust#139089 as a random example):
Output with the PR:
Even with the change in place, one would still probably want to paste the output into the github commend field and then look at the Preview to click through the issues, but at least this allows to filter out the obviously irrelevant ones easier.