compiletest: track LLDB inputs in up-to-date checks - #160137
Conversation
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
rustbot
added
A-compiletest
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
There was a problem hiding this comment.
This seems potentially OK but I'd like someone more familiar with the debuginfo story we're constructing right now to take a look.
r? @jieyouxu
| stamp.add_path(&lldb_batchmode); | ||
| stamp.add_dir(&lldb_batchmode); |
There was a problem hiding this comment.
Hm, this makes me wonder if we should make add_path detect whether you're adding a directory and recursively add in that case. It's not obvious to me why you would only want to stamp the directory itself, and it seems easy to not notice that you're doing a partial stamp.
There was a problem hiding this comment.
Yeah, that feels broadly more reasonable
| let lldb_input = test_dir.join("lldb_input"); | ||
| if lldb_input.is_dir() { | ||
| inputs_stamp.add_path(&lldb_input); | ||
| inputs_stamp.add_dir(&lldb_input); |
There was a problem hiding this comment.
These files look potentially generated? E.g., looking at tests/debuginfo/basic-types/lldb_input/non_windows.json I see bless_metadata which doesn't look written by humans... maybe we should include a __comment field in those files with some annotation of what they are?
I'm not clear from the PR description that added these files (#158298) on whether they are always 'outputs' from some process or true inputs.
There was a problem hiding this comment.
They're intended to be generated (as in the JSON files). The bless metadata was intended to be a record of how the metadata was produced, and without reblessing, bless metadata shouldn't participate in the "diff" of the JSONs. I think __comment could make sense, or maybe we could try to have a bit more elaboration in r-d-g about what the general sections are for these.
cc @Kobzol too
There was a problem hiding this comment.
Maybe just renaming lldb_input to something like lldb_output, lldb_generated, or something like that, would make it more obvious?
|
|
|
Please also don't use LLM to write the PR description, see our LLM usage policy. |
rustbot
removed
the
S-waiting-on-review
|
Reminder, once the PR becomes ready for a review, use |
rustbot
added
the
S-waiting-on-author
|
please feel free to take this further, my apologies I'm busy with something else |
|
Yeah, I think we'll need to see how better to handle this, so closing this for now. Thanks for the PR. |
rustbot
removed
the
S-waiting-on-author
Rerun `tests/debuginfo` tests if repr data has changed Resolves rust-lang#161138 This also includes the necessary checking for GDB's data even though none exists atm (rust-lang#160377 will contain the first set). The extra handling doesn't hurt anything since we have to account for all the other tests that don't have repr data anyway. In a followup, I can rename the `lldb_input` directory to something else (see: rust-lang#160137 (comment)). Doing so requires me to touch a bunch of other places where the name is used, so it should probably be it's own PR. Making sure the tests rerun when the data changes is higher priority though atm. r? @jieyouxu , @Kobzol cc @Mark-Simulacrum
Rerun `tests/debuginfo` tests if repr data has changed Resolves rust-lang#161138 This also includes the necessary checking for GDB's data even though none exists atm (rust-lang#160377 will contain the first set). The extra handling doesn't hurt anything since we have to account for all the other tests that don't have repr data anyway. In a followup, I can rename the `lldb_input` directory to something else (see: rust-lang#160137 (comment)). Doing so requires me to touch a bunch of other places where the name is used, so it should probably be it's own PR. Making sure the tests rerun when the data changes is higher priority though atm. r? @jieyouxu , @Kobzol cc @Mark-Simulacrum
Rollup merge of #161967 - Walnut356:stamp_repr_data, r=jieyouxu Rerun `tests/debuginfo` tests if repr data has changed Resolves #161138 This also includes the necessary checking for GDB's data even though none exists atm (#160377 will contain the first set). The extra handling doesn't hurt anything since we have to account for all the other tests that don't have repr data anyway. In a followup, I can rename the `lldb_input` directory to something else (see: #160137 (comment)). Doing so requires me to touch a bunch of other places where the name is used, so it should probably be it's own PR. Making sure the tests rerun when the data changes is higher priority though atm. r? @jieyouxu , @Kobzol cc @Mark-Simulacrum
5 participants
the compiletest was not tracking all the files used by LLDB tests. so because of that, when those files would be changed the compiletest can skip the test and reuse an outdated successful result. the PR makes those changes trigger a proper test run.