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.

Sign up for GitHub

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

Jump to bottom

Crater rework targets #1083

Merged
merged 2 commits into from
Oct 31, 2024
Merged

Crater rework targets #1083

merged 2 commits into from
Oct 31, 2024

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented Oct 31, 2024

Okay this... was annoying, but how we were handling targets had some dumb issues:

  • we weren't including the config file in serialization, which means that we didn't have enough info to generate the repro command for gftools
  • (and also it was possible to end up with distinct targets serializing identically, in the case where a repo contained multiple configs which pointed to overlapping sources; not sure if this existed but was possible)
  • but I also want to be efficient in what we serialize, and the config and source paths generally share a stem
  • because we serialize to json and use targets as the keys, we need to ensure they serialize as strings
  • so the logic got complicated fairly quickly.

I have the repro command fix working on top of this, and will PR that separately.

@cmyr
[crater] Get rid of TargetId
a4f10d8 
This was overly clever, because I wanted to avoid serializing the config
file paths everywhere; however we're going to need them in order to
generate the reproduction commands.
@cmyr
[crater] Tweak how we save target paths
83d30f4 
Instead of these being full paths, we save the path of the sources
directory and then the config and source files are paths relative to
that.

This mostly just lets us be represented more efficiently in json.

- This also makes config & source on target private, so the only way to
get these paths is to pass in the `cache_dir` and receive the fully
resolved path.
@cmyr cmyr mentioned this pull request Oct 31, 2024
Merged
@cmyr cmyr added this pull request to the merge queue Oct 31, 2024
Merged via the queue into main with commit 49f5107 Oct 31, 2024
10 checks passed
@cmyr cmyr deleted the crater-rework-targets branch October 31, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsheeter rsheeter rsheeter approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cmyr @rsheeter