Bug fix: use target.id as bundle prefix to avoid conflict from basenames
Review Request #3272 - Created Dec. 19, 2015 and submitted
|patricklaw, stuhood, zundel|
We use basename to prefix bundle folder, it has always been unsafe but since we
used to bundle from
target_rootsand people normally bundle one or just a few
targets, the issue isn't obvious.
Since https://rbcommons.com/s/twitter/r/3119/ switching from
targets(), this becomes more problematic, because now jvm_binary that jvm_app
depends on will also be run, if jvm_binary is run after jvm_app, it will destroy
the additional files jvm_app includes. And turns out sharing the same basenames
is pretty common, greping twitter repo 15% basenames have more than 1 occurrence.
See "Testing Done" section for an example from pants own testprojects.
- Switches to use
target.idas the default bundle prefix, with target.id it's
safe to bundle transitively from
context.targets(). Long term this ensures
- basename is still supported to for backward compatibility, but in order to
reduce the chance people running into this problem, it can only bundle targets
context.target_roots, essentially goes back to before rb 3119.
- In basename mode, add a detection for duplicate basenames in case that does
happen so user at least can correct.
- binary has the same problem, I left as a TODO, but let me know if you think we
should fix it as well, say if junit_tests target wants to depend on binary, this
will be an issue.
- This review is from Karin's original reviewboard + fixing tests, thanks Karin!
./pants bundle testprojects/src/java/org/pantsbuild/testproject/bundle,
On master, note `data/exampledata.txt` is missing tw-mbp-peiyu:pants peiyu$ find dist/bundle-example-bundle dist/bundle-example-bundle/bundle-example.jar dist/bundle-example-bundle/libs/com.google.guava-guava-18.0.jar ... With the fix tw-mbp-peiyu:pants peiyu$ find dist/testprojects.src.java.org.pantsbuild.testproject.bundle.bundle-bundle dist/testprojects.src.java.org.pantsbuild.testproject.bundle.bundle-bundle/bundle-example.jar dist/testprojects.src.java.org.pantsbuild.testproject.bundle.bundle-bundle/data/exampledata.txt dist/testprojects.src.java.org.pantsbuild.testproject.bundle.bundle-bundle/libs/com.google.guava-guava-18.0.jar ...
I'm not sure this is the right way to solve the problem, because 'basename' is an explicit setting that users are asking for, and it is usually set to work around the issue that the default value of 'name' is a conflict under dist.
Did you consider setting default for
target.basenamein jvm_binary.py to be target.id if basename isn't specified instead of
- Since in basename mode duplicate basename can still happen, added a detection so at least people can be aware and take action.
- minor edits
Revision 2 (+200 -105)
nit: all of this setup in init() is unecessary. This is a holdover when options were a bit more difficult to calculate. You could just update the rest of the code to reference the options directly. Or you could move these into @property accessors. Either way, it makes the evaluation of options lazy and saves on startup time.
I think this test would be better as a dynamically created target in a unit test than something we statically declare under testprojects. We do have negative tests under testprojects, but usually they are more complex than this.
Again, I think this could easily be a unit test in test_bundle_create.py. It would run more quickly and not add another negative test case under testprojects/
Address Eric's comments
- clean up redundant variables for options in
- unit test conflict basenames instead of integration test
This special casing is confusing. IMO it would be better to explode for a duplicate (within the same execution) than to change what is bundled this way. For one thing, it makes the option much more confusing to explain (this type of thing would need to go in the help).
Given this check, the above smaller set from
When a jar goes inside a
target.id-named directory, it's a bit redundant to also use the target's name in the jar name. The target name is already a part of of its
Address Stu's comments
- Explain the code weirdness a bit more
Revision 4 (+208 -117)