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

compiletest: Test execution time-stamping should take into account debugger pretty printers. #45022

Closed
michaelwoerister opened this issue Oct 4, 2017 · 6 comments
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.

Comments

@michaelwoerister
Copy link
Member

Modifying one of the pretty-printer python files in src/etc does "un-ignore" debuginfo tests. The relevant files should be added to the list of inputs in compiletest::up_to_date():

fn up_to_date(config: &Config, testpaths: &TestPaths, props: &EarlyProps) -> bool {
let stamp = mtime(&stamp(config, testpaths));
let mut inputs = vec![
mtime(&testpaths.file),
mtime(&config.rustc_path),
];
for aux in props.aux.iter() {
inputs.push(mtime(&testpaths.file.parent().unwrap()
.join("auxiliary")
.join(aux)));
}
for lib in config.run_lib_path.read_dir().unwrap() {
let lib = lib.unwrap();
inputs.push(mtime(&lib.path()));
}
inputs.iter().any(|input| *input > stamp)
}

Bonus points for exiting from that function immediately when finding something out-of-date.

@michaelwoerister michaelwoerister added A-testsuite Area: The testsuite used to check the correctness of rustc C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. WG-infra-rustbuild labels Oct 4, 2017
@k0pernicus
Copy link
Contributor

Maybe I can help here as a first contribution to the programming language.
I will try to propose a solution for tomorrow evening.

@k0pernicus
Copy link
Contributor

k0pernicus commented Oct 4, 2017

So, I have read the code and I tried to understand where is the problem, and what kind of solution I can propose.

This is my thought:

  1. The problem is pretty-printer Python files in src/etc are not considered in up_to_date; so, if you modified one of those files, you will not ignoring them (am I right?).
  2. You propose to add the relevant files (debugger_pretty_printers_common.py, gdb_load_rust_pretty_printers.py, and gdb_rust_pretty_printers.py I think) as inputs to up_to_date, to consider them as "important files to not ignore" (am I right?).
  3. In order to include the pretty printer files, I have to include them in TestPaths, right?

Thanks for those responses.

@michaelwoerister
Copy link
Member Author

@k0pernicus You're on the right track:

  1. Almost right. When the compiletest tool executes a test and that test passes, it will not re-execute it until something relevant has changed. "Not re-executing" in practice means that the test will show up as ignored in the commandline output. The problem here is that modifications to the pretty printer files are not picked up as "relevant changes". As a consequence the tests stay "ignored" even though they should actually be re-run because the changes to the pretty printer scripts can influence their result.
  2. Correct. inputs is a list of all things that must not have changed since the last test execution if that test is to stay ignored. If any file from inputs has a modification time that is newer than the last test execution, the test has to be re-executed.
  3. Not quite. TestPaths contains the location of the test that is being executed. What we want here is to just directly add the pretty-printer script paths to the inputs list. Something like
let rust_src_dir = ...; 
inputs.push(rust_src_dir.join("src/etc/debugger_pretty_printers_common.py"));
inputs.push(rust_src_dir.join("src/etc/gdb_load_rust_pretty_printers.py"));
...

Unfortunately the rust_src_dir is not readily available. But you could move the runtest::TestCx::find_rust_src_root() to common::Config and then use that for finding it.

@k0pernicus
Copy link
Contributor

Ok, I understand.
Thanks a lot for these explanations.

I will work on that issue after work, in order to submit a PR + code review for tomorrow morning.

@k0pernicus
Copy link
Contributor

k0pernicus commented Oct 5, 2017

@michaelwoerister I just created the PR to share the code.
I compiled the project and I ran the test (using ./x.py test).
Unfortunately, I don't know how to test my changes (which is why I put a "WiP")...

Is it possible to obtain a simple example of "procedure", in order to test my changes please?
Thanks a lot.

k0pernicus added a commit to k0pernicus/rust that referenced this issue Oct 9, 2017
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
kennytm added a commit to kennytm/rust that referenced this issue Oct 9, 2017
Debugger pretty printer files are take into account in test execution time-stamping

This PR is proposed to solve the issue rust-lang#45022.
@michaelwoerister
Copy link
Member Author

Implemented in #45051.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion.
Projects
None yet
Development

No branches or pull requests

2 participants