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

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

Peiyu Wang
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
  • 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.

- 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://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

With the fix

tw-mbp-peiyu:pants peiyu$ find dist/testprojects.src.java.org.pantsbuild.testproject.bundle.bundle-bundle


  • 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