-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Detect cyclic auxiliary #134180
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
Closed
Shunpoco
wants to merge
1
commit into
rust-lang:master
from
Shunpoco:issue-133580-detect-cyclic-auxiliary
Closed
Detect cyclic auxiliary #134180
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem: so I thought about this a bit.
build_graph
during test collection that does directory traversal and file reading and prop parsing is quite expensive (especially on slower filesystems like native Windows), and I don't think this is necessarily the right place to do the check.I would've expected that compiletest does something like:
I would've expected that aux cycle detection happens after early prop collection after we establish the relationship between test files and auxiliaries, and not during test discovery and collection.
Alternatively, we could also just declare that we're not going to handle cyclic auxiliaries and "just don't do that", and doing cyclic auxiliaries is considered PEBKAC as "compiletest did exactly what the user instructed it to do".
cc @oli-obk: how does
ui_test
handle infinitely-recursive auxiliaries?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't: oli-obk/ui_test#123
But it should be fixable now that I have a build manager that handles all of these in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jieyouxu
Thanks for your review.
Now I understood that cycle detection should be run around here, is that right?
FYI: EarlyProps gathers auxiliaries only which are referred from the test file. For example from my manual test files above,
cycle1.rs
is included in the early_props foraux-cyclic.rs
butcycle2.rs
is not because only cycle1.rs has a dependency to cycle2.rs. So reading files to make a dependency graph is inevitable, but it should be lighter than reading files by entire directory traversal since we can check the cycle only against test files which contains auxs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I'll have to revisit this.