Bug fix: use target.id as bundle prefix to avoid conflict from basenames

Review Request #3272 - Created Dec. 19, 2015 and submitted

Information
Peiyu Wang
pants
2745
c76f888...
Reviewers
pants-reviews
patricklaw, stuhood, zundel

We use basename to prefix bundle folder, it has always been unsafe but since we
used to bundle from target_roots and people normally bundle one or just a few
targets, the issue isn't obvious.

Since https://rbcommons.com/s/twitter/r/3119/ switching from target_roots to
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.

This review

  • Switches to use target.id as the default bundle prefix, with target.id it's
    safe to bundle transitively from context.targets(). Long term this ensures
    correctness.
  • 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
    from 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.

NOTE:
- 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!
https://rbcommons.com/s/twitter/r/3250/

  • https://travis-ci.org/pantsbuild/pants/builds/97797692

  • Running ./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
...

Issues

  • 0
  • 0
  • 2
  • 2
Description From Last Updated
Patrick Lawson
Eric Ayers
Peiyu Wang
Eric Ayers
Eric Ayers
Eric Ayers
Stu Hood
Peiyu Wang
Review request changed

Status: Closed (submitted)

Change Summary:

Committed as 5b7fd99d9997020ec688bb1ad4b3bbbb34c51d76

Loading...