-
Notifications
You must be signed in to change notification settings - Fork 13.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Debugger pretty printer files are take into account in test execution time-stamping #45051
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
)); | ||
} | ||
// Relevant pretty printer files | ||
let pretty_printer_files = [ |
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.
For this part, I don't know if the more relevant way to do is to "crawl" the rust_src_path
, and to check if file which contains in the name "pretty_print". Using this solution, if a member creates a new pretty printer file, this file will be automatically checked in up_to_date
...
The cons of this automated solution is that we must respect title rules for the pretty printing files.
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.
The explicit list is fine for now.
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.
But could you please add lldb_batchmode.py
and lldb_rust_formatters.py
as well?
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.
No problem!
Looks good to me. You can test it as follows:
|
@michaelwoerister Thanks - I will make those tests after work |
I just runned the tests, and 98/108 tests failed... As an example, I have:
This is an example of the stacktrace:
Finally, this is the end of the tests:
Maybe something wrong with my configuration, no? |
@michaelwoerister The tests to verify the PR are ok. This is a simple trace:
|
@k0pernicus Looks great now, thank you! Would you mind squashing the commits into one? Then this should be good to go. |
@michaelwoerister I made a rebase of my seven commits here: f331433 |
@k0pernicus GitHub still shows the other commits too, not just the squashed one. Following the workflow described at https://github.com/servo/servo/wiki/Beginner%27s-guide-to-rebasing-and-squashing should give you the desired results (which is just a single commit on top of the current |
Add pretty printer files into test execution time-stamping Move find_rust_src_path() as a method for Config Move find_rust_src_path() as a method for Config Call find_rust_src_path() from Config Move find_rust_src_path() from common.rs to header.rs Add pretty printer files as relevant files to get up_to_date information Remove dead code Add two pretty printer files to keep a close watch on Move find_rust_src_path() as a method for Config Move find_rust_src_path() as a method for Config Call find_rust_src_path() from Config Move find_rust_src_path() from common.rs to header.rs Remove dead code Add two pretty printer files to keep a close watch on
@michaelwoerister Sorry for that - I forgot the |
Yes, that looks better! Thank you |
@bors r+ rollup |
📌 Commit 53a6485 has been approved by |
Debugger pretty printer files are take into account in test execution time-stamping This PR is proposed to solve the issue rust-lang#45022.
This PR is proposed to solve the issue #45022.