[rustdoc] Ensure that temporary doctest folder is correctly removed even if doctests failed #140706
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
…o allow a callback to be called before exiting
rustbot
added
S-waiting-on-review
notriddle
reviewed
May 6, 2025
GuillaumeGomez
force-pushed
the
fix-missing-temp-dir-cleanup
branch
from
9959875 to
173cdaf
Compare
|
Applied suggestion. |
|
Okay, so what would it take to build a run-make test of this? I understand if it's too much trouble, but I'd at least like to have some rationale, because without a regression test, this seems like it could easily regress. |
|
@lolbinarycat suggested a good way to test it so gonna write it. |
|
This PR modifies cc @jieyouxu |
|
Added the regression test. Since |
GuillaumeGomez
force-pushed
the
fix-missing-temp-dir-cleanup
branch
from
1ba215c to
442ae63
Compare
Just a caveat, on Windows the env vars are slightly different ( |
|
Hence why for now it's only run on linux. 😉 I can write a follow-up though to extend the tests to other platforms, but I'd like to get this merged as soon as possible for this bug to disappear. |
|
Yeah, not blocking for sure 👍 |
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 7, 2025
…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#140234 (Separate dataflow analysis and results) - rust-lang#140614 (Correct warning message in restricted visibility) - rust-lang#140671 (Parser: Recover error from named params while parse_path) - rust-lang#140700 (Don't crash on error codes passed to `--explain` which exceed our internal limit of 9999 ) - rust-lang#140706 ([rustdoc] Ensure that temporary doctest folder is correctly removed even if doctests failed) - rust-lang#140734 (Fix regression from rust-lang#140393 for espidf / horizon / nuttx / vita) - rust-lang#140741 (add armv5te-unknown-linux-gnueabi target maintainer) - rust-lang#140745 (run-make-support: set rustc dylib path for cargo wrapper) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 7, 2025
…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#140234 (Separate dataflow analysis and results) - rust-lang#140614 (Correct warning message in restricted visibility) - rust-lang#140671 (Parser: Recover error from named params while parse_path) - rust-lang#140700 (Don't crash on error codes passed to `--explain` which exceed our internal limit of 9999 ) - rust-lang#140706 ([rustdoc] Ensure that temporary doctest folder is correctly removed even if doctests failed) - rust-lang#140734 (Fix regression from rust-lang#140393 for espidf / horizon / nuttx / vita) - rust-lang#140741 (add armv5te-unknown-linux-gnueabi target maintainer) - rust-lang#140745 (run-make-support: set rustc dylib path for cargo wrapper) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 8, 2025
Rollup merge of rust-lang#140706 - GuillaumeGomez:fix-missing-temp-dir-cleanup, r=notriddle [rustdoc] Ensure that temporary doctest folder is correctly removed even if doctests failed Fixes rust-lang#139899. The bug was due to the fact that if any doctest fails for any reason, we call `exit` (or it's called inside `libtest` if not edition 2024), meaning that `TempDir`'s destructor isn't called, and therefore the temporary folder isn't cleaned up. Took me a while to figure out how to reproduce but finally I was able to reproduce the bug with: `````rust #![doc(test(attr(deny(warnings))))] //! ``` //! let a = 12; //! ``` ````` And then I ensured that panicking doctests were cleaned up as well: `````rust //! ``` //! panic!(); //! ``` ````` And finally I checked if it was fixed for merged doctests too (`--edition 2024`). To make this work, I needed to add a new public function in `libtest` too which would call a function once all tests have been run. So only issue is: I have absolutely no idea how we can add a regression test for this fix. If anyone has an idea... r? `@notriddle`
GuillaumeGomez
added
the
beta-nominated
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 8, 2025
…oval, r=jieyouxu Make `rustdoc-tempdir-removal` run-make tests work on other platforms than linux Follow-up of rust-lang#140706. r? `@jieyouxu`
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
May 8, 2025
…oval, r=jieyouxu Make `rustdoc-tempdir-removal` run-make tests work on other platforms than linux Follow-up of rust-lang#140706. r? ``@jieyouxu``
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 9, 2025
Rollup merge of rust-lang#140800 - GuillaumeGomez:rustdoc-tempdir-removal, r=jieyouxu Make `rustdoc-tempdir-removal` run-make tests work on other platforms than linux Follow-up of rust-lang#140706. r? `@jieyouxu`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #139899.
The bug was due to the fact that if any doctest fails for any reason, we call
exit(or it's called insidelibtestif not edition 2024), meaning thatTempDir's destructor isn't called, and therefore the temporary folder isn't cleaned up.Took me a while to figure out how to reproduce but finally I was able to reproduce the bug with:
And then I ensured that panicking doctests were cleaned up as well:
And finally I checked if it was fixed for merged doctests too (
--edition 2024).To make this work, I needed to add a new public function in
libtesttoo which would call a function once all tests have been run.So only issue is: I have absolutely no idea how we can add a regression test for this fix. If anyone has an idea...
r? @notriddle