Stabilize #[coverage] attribute #130766
Stabilize #[coverage] attribute #130766
Conversation
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
rustbot
added
S-waiting-on-review
|
Some changes occurred in coverage tests. cc @Zalathar rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer |
clarfonthey
force-pushed
the
stable-coverage-attribute
branch
from
dc49513 to
4b64d0d
Compare
This comment has been minimized.
This comment has been minimized.
clarfonthey
force-pushed
the
stable-coverage-attribute
branch
from
4b64d0d to
2a11c1d
Compare
This comment has been minimized.
This comment has been minimized.
clarfonthey
force-pushed
the
stable-coverage-attribute
branch
from
2a11c1d to
8037f0c
Compare
This comment has been minimized.
This comment has been minimized.
|
I will forward this to a more appropriate reviewer: r? @Zalathar |
|
FWIW, I'm currently working on fixing the test errors-- my computer kept crashing which made it difficult to run all the tests. Unfortunately a lot of coverage tests are now off-by-one due to the removal of the line enabling the feature, and they're all going to have to be re-blessed. |
clarfonthey
force-pushed
the
stable-coverage-attribute
branch
from
8037f0c to
53c467b
Compare
This comment has been minimized.
This comment has been minimized.
|
Appear to be getting very weird crashes with the debuginfo tests. From the backtrace: BacktraceI'm just going to run Yeah, okay. It might also be a difference in Python 3.12 that broke something too, but either way, the debuginfo tests show up as failing on my machine and the coverage tests show up as passing also, so, something weird is afoot. |
|
cc @rust-lang/lang |
I'm not exactly sure what debuginfo tests want, but bootstrap and other parts of compiletest AFAIK assumes Python 3.10 exactly, not 3.9 or 3.12. |
That feels worth documenting somewhere, considering how on my end that just shows up as LLVM hard-crashing and not as a wrong Python version. Like, maybe even worth having a virtualenv setup for the repo just to guarantee the right Python version is being used. |
|
I'm not sure if that's the root cause of your crashes, but if it is, then yes |
|
Well, I'm currently installing 3.10, so, we'll hopefully find out! |
|
Actually sorry minor correction: it's not bootstrap that expects Python 3.10 (as I see there is a toolstate check that goes through 3.10+ as well), but something in compiletest (maybe debuginfo tests?) expect Python 3.10 dll to be available. EDIT: I remembered what it is, it's lldb debuginfo tests, that requires Python 3.10 dll to be in the |
|
|
You linked #128392 twice, which I assume is a mistake. |
|
Yeah, I'm giving up on getting the debuginfo tests working. But I'll poke around to see if I can get the other tests to replicate their issues. I would have to downgrade my system Python version, since the tests explicitly link |
|
It seems that even running the test suite via docker, I can't replicate the latest errors. Running Looks like I've stumbled my way into the realm of cursed compiler testing issues that I don't know how to fix. |
|
@clarfonthey: 🔑 Insufficient privileges: Not in reviewers |
|
@bors delegate+ |
|
✌️ @clarfonthey, you can now approve this pull request! If @jhpratt told you to " |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (1d35638): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 770.185s -> 770.505s (0.04%) |
…esleywiser Revert stabilization of the `#[coverage(..)]` attribute Due to a process mixup, the PR to stabilize the `#[coverage(..)]` attribute (rust-lang#130766) was merged while there are still outstanding concerns. The default action in that situation is to revert, and the feature is not sufficiently urgent or uncontroversial to justify special treatment, so this PR reverts that stabilization. --- - A key point that came up in offline discussions is that unlike most user-facing features, this one never had a proper RFC, so parts of the normal stabilization process that implicitly rely on an RFC break down in this case. - As the implementor and de-facto owner of the feature in its current form, I would like to think that I made good choices in designing and implementing it, but I don't feel comfortable proceeding to stabilization without further scrutiny. - There hasn't been a clear opportunity for T-compiler to weigh in or express concerns prior to stabilization. - The stabilization PR cites a T-lang FCP that occurred in the tracking issue, but due to the messy design and implementation history (and lack of a clear RFC), it's unclear what that FCP approval actually represents in this case. - At the very least, we should not proceed without a clear statement from T-lang or the relevant members about the team's stance on this feature, especially in light of the other concerns listed here. - The existing user-facing documentation doesn't clearly reflect which parts of the feature are stable commitments, and which parts are subject to change. And there doesn't appear to be a clear consensus anywhere about where that line is actually drawn, or whether the chosen boundary is acceptable to the relevant teams and individuals. - For example, the [stabilization report comment](rust-lang#84605 (comment)) mentions that some aspects are subject to change, but that text isn't consistent with my earlier comments, and there doesn't appear to have been any explicit discussion or approval process. - [The current reference text](https://github.com/rust-lang/reference/blob/4dfaa4f/src/attributes/coverage-instrumentation.md) doesn't mention this distinction at all, and instead simply describes the current implementation behaviour. - When the implementation was changed to its current form, the associated user-facing error messages were not updated, so they still refer to the attribute only being allowed on functions and closures. - On its own, this might have been reasonable to fix-forward in the absence of other concerns, but the fact that it never came up earlier highlights the breakdown in process that has occurred here. --- Apologies to everyone who was excited for this stabilization to land, but unfortunately it simply isn't ready yet.
Rollup merge of rust-lang#134672 - Zalathar:revert-coverage-attr, r=wesleywiser Revert stabilization of the `#[coverage(..)]` attribute Due to a process mixup, the PR to stabilize the `#[coverage(..)]` attribute (rust-lang#130766) was merged while there are still outstanding concerns. The default action in that situation is to revert, and the feature is not sufficiently urgent or uncontroversial to justify special treatment, so this PR reverts that stabilization. --- - A key point that came up in offline discussions is that unlike most user-facing features, this one never had a proper RFC, so parts of the normal stabilization process that implicitly rely on an RFC break down in this case. - As the implementor and de-facto owner of the feature in its current form, I would like to think that I made good choices in designing and implementing it, but I don't feel comfortable proceeding to stabilization without further scrutiny. - There hasn't been a clear opportunity for T-compiler to weigh in or express concerns prior to stabilization. - The stabilization PR cites a T-lang FCP that occurred in the tracking issue, but due to the messy design and implementation history (and lack of a clear RFC), it's unclear what that FCP approval actually represents in this case. - At the very least, we should not proceed without a clear statement from T-lang or the relevant members about the team's stance on this feature, especially in light of the other concerns listed here. - The existing user-facing documentation doesn't clearly reflect which parts of the feature are stable commitments, and which parts are subject to change. And there doesn't appear to be a clear consensus anywhere about where that line is actually drawn, or whether the chosen boundary is acceptable to the relevant teams and individuals. - For example, the [stabilization report comment](rust-lang#84605 (comment)) mentions that some aspects are subject to change, but that text isn't consistent with my earlier comments, and there doesn't appear to have been any explicit discussion or approval process. - [The current reference text](https://github.com/rust-lang/reference/blob/4dfaa4f/src/attributes/coverage-instrumentation.md) doesn't mention this distinction at all, and instead simply describes the current implementation behaviour. - When the implementation was changed to its current form, the associated user-facing error messages were not updated, so they still refer to the attribute only being allowed on functions and closures. - On its own, this might have been reasonable to fix-forward in the absence of other concerns, but the fact that it never came up earlier highlights the breakdown in process that has occurred here. --- Apologies to everyone who was excited for this stabilization to land, but unfortunately it simply isn't ready yet.
traviscross
removed
the
relnotes
…-attribute, r=wesleywiser" This reverts commit 87c2f9a.
Closes #84605, which passed FCP.
Stabilisation report here: #84605 (comment)
Also added to reference here: rust-lang/reference#1628
try-job: aarch64-apple
try-job: x86_64-gnu
try-job: x86_64-msvc