Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add check to ensure error code explanations are not removed anymore even if not emitted #86623

Merged
merged 2 commits into from
Jul 5, 2021

Conversation

GuillaumeGomez
Copy link
Member

The error explanations are useful in case you use older version of the compiler. Even more if they had an explanation. If they are not emitted, their explanations should be updated but not removed (as we did for a few of them, like E0001).

r? @Mark-Simulacrum

@GuillaumeGomez GuillaumeGomez added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Jun 25, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2021
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez GuillaumeGomez force-pushed the prevent-explanation-removal branch 2 times, most recently from f83a7f3 to bc0aa3f Compare June 26, 2021 08:57
@GuillaumeGomez
Copy link
Member Author

Moved the check into validate-toolstate.sh in mingw-check.

@Mark-Simulacrum
Copy link
Member

This is unrelated to validating toolstate, so it should go in a separate file.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2021
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Created a new bash script I'm now calling in the Dockerfile.

@GuillaumeGomez GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 28, 2021
@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

Please push a temp commit to make sure this works (mingw-check should fail on the PR) and also we may need to somehow skip this script in local docker invocations (where FETCH_HEAD is meaningless). I'm not sure what the best way of doing that would be, but it may also be OK to not worry too much about it -- if you could check that src/ci/docker/run.sh mingw-check works locally that would be sufficient for me (it may not actually detect deleted error codes but that's OK, just shouldn't fail).

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Jun 28, 2021

$ src/ci/docker/run.sh mingw-check
...
Build completed successfully in 0:00:33
+ /scripts/validate-toolstate.sh
Cloning into 'rust-toolstate'...
<Nothing changed>
+ /scripts/validate-error-codes.sh
/scripts/validate-error-codes.sh: line 6: BASE_COMMIT: unbound variable
+ es-check es5 ../src/librustdoc/html/static/main.js ../src/librustdoc/html/static/search.js ../src/librustdoc/html/static/settings.js ../src/librustdoc/html/static/source-script.js ../src/librustdoc/html/static/storage.js
ES-Check: there were no ES version matching errors!  🎉
+ eslint ../src/librustdoc/html/static/main.js ../src/librustdoc/html/static/search.js ../src/librustdoc/html/static/settings.js ../src/librustdoc/html/static/source-script.js ../src/librustdoc/html/static/storage.js
Compile requests               237
Compile requests executed      215
Cache hits                      13
Cache misses                   202
Cache timeouts                   0
Cache read errors                0
Forced recaches                  0
Cache write errors               0
Compilation failures             0
Cache errors                     0
Non-cacheable compilations       0
Non-cacheable calls             18
Non-compilation calls            4
Unsupported compiler calls       0
Average cache write          0.000 s
Average cache read miss      0.105 s
Average cache read hit       0.000 s
Cache location             Local disk: "/sccache"
Cache size                     491 KiB
Max cache size                  10 GiB
== clock drift check ==
  local time: Mon Jun 28 19:08:09 UTC 2021
  network time: Mon, 28 Jun 2021 01:34:02 GMT
== end clock drift check ==

So this part is working fine apparently. Even with:

/scripts/validate-error-codes.sh: line 6: BASE_COMMIT: unbound variable

@Mark-Simulacrum
Copy link
Member

It looks like CI here is passing, but the E0001 code is deleted so presumably CI should fail. My guess is that the BASE_COMMIT variable is not set in the script -- you'll need to find a way to pipe it through. I'm not sure where it gets set, though, so there's likely some investigation that needs to be done.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2021
@GuillaumeGomez GuillaumeGomez force-pushed the prevent-explanation-removal branch 2 times, most recently from c069f7e to 8819021 Compare June 29, 2021 13:23
@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

IT WORKS! 🎉

Removing unwanted commit and it's good to go. :)

@GuillaumeGomez GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 2, 2021
@GuillaumeGomez
Copy link
Member Author

@Mark-Simulacrum This is now ready.

src/ci/scripts/should-skip-this.sh Outdated Show resolved Hide resolved
src/ci/docker/run.sh Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Updated! Only setting BASE_COMMIT if on CI. Then the error code removal checker only runs if the variable is not empty (so if it doesn't exist, it doesn't run either).

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

@Mark-Simulacrum Should be ready now.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 4, 2021

📌 Commit d6962ff has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 4, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 4, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#86477 (E0716: clarify that equivalent code example is erroneous)
 - rust-lang#86623 (Add check to ensure error code explanations are not removed anymore even if not emitted)
 - rust-lang#86856 (Make x.py less verbose on failures)
 - rust-lang#86858 (Stabilize `string_drain_as_str`)
 - rust-lang#86859 (Add a regression test for issue-69323)
 - rust-lang#86862 (re-export SwitchIntEdgeEffects)
 - rust-lang#86864 (Add missing code example for Write::write_vectored)
 - rust-lang#86874 (Bump deps)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit ac880e5 into rust-lang:master Jul 5, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 5, 2021
@GuillaumeGomez GuillaumeGomez deleted the prevent-explanation-removal branch July 5, 2021 08:10
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 28, 2021
…crum

Prepare beta 1.55.0

Included backports:

* rust-lang#86696
* rust-lang#87390 (squashed)

Reverted:

* rust-lang#86623

cc `@rust-lang/release` `@Mark-Simulacrum`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 15, 2021
…bini

Remove git fetch from CI

rust-lang#86623 added a call to `git fetch`, which is problematic for releases.

r? `@pietroalbini`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 15, 2021
…bini

Remove git fetch from CI

rust-lang#86623 added a call to `git fetch`, which is problematic for releases.

r? ``@pietroalbini``
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2021
Remove git fetch from CI

rust-lang#86623 added a call to `git fetch`, which is problematic for releases.

r? `@pietroalbini`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants