Generate metadata in distributed rule - #79
Conversation
ddf9607 to
4a7a2be
Compare
Szelethus
left a comment
Szelethus
left a comment
There was a problem hiding this comment.
We need some tests to show the observable behaviour change this patch introduces, or patches this is a part of.
Szelethus
left a comment
Szelethus
left a comment
There was a problem hiding this comment.
I have some thoughts. First, we add two new features:
- Enable metadata generation
- Add metadata merging
Can we somehow split this PR along that line? Also, with regards to testing, it would be great to have a test where we have two pre-existing metadata files which we merge using the new utility, and thats it.
89acada to
8ffb40b
Compare
Szelethus
left a comment
Szelethus
left a comment
There was a problem hiding this comment.
The test looks great, thanks! Sorry for the comments in this round, could've made it earlier.
| if os.path.isfile(os.path.join(DATA_DIR, "metadata.json")): | ||
| shutil.move(os.path.join(DATA_DIR, "metadata.json"), METADATA_FILE) | ||
| elif plist_exists: | ||
| # We do not create the "empty" metadata file, to make bazel fail |
There was a problem hiding this comment.
to make it fail? What does that mean?
There was a problem hiding this comment.
Also, if we are "making it fail", the warning below is actually an error.
There was a problem hiding this comment.
I rephrased the comment
Szelethus
left a comment
Szelethus
left a comment
There was a problem hiding this comment.
LGTM! Sorry for being so picky with this one, but I think the PR looks amazing now.
| elif plist_exists: | ||
| # We do not create the "empty" metadata file | ||
| # this will cause bazel to crash due to a missing output file. | ||
| print( | ||
| "[WARNING] metadata.json doesn't exists " | ||
| "despite successful analysis..." | ||
| ) |
There was a problem hiding this comment.
Sorry, final comment. Isn't this a should-never-happen case? If so, isn't this severe enough to warrant throwing an exception?
2 participants
Why:
We currently do not produce a
metadata.jsonwith our distributed rule.What:
Added metadata output for all steps.
Addresses:
#45