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.

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

Include transitive Resources targets in PrepareResources #4569

Merged
merged 1 commit into from May 9, 2017

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

stuhood commented May 9, 2017

Problem

The resources arg on JVM and Python targets is now deprecated, but the previous behaviour of only collecting directly declared resources onto the runtime classpath remains. The result for the JVM is that resources are only collected if they are directly depended on by a JvmTarget, rather than if they are located anywhere in the transitive dependencies of that target.

While this behaviour has existed for a while, it was difficult to run into it when resources were directly declared as "owned" by a particular target.

Solution

Transitively collect the relevant Resources targets below JvmTarget roots in PrepareResources.

Also, I've verified on a very large codebase that this did not result in any adverse effects on resource availability.

Result

Resources act (more: see #4568) like other dependencies generally do. Fixes #4567

@stuhood stuhood added this to the 1.3.0 milestone May 9, 2017

@stuhood stuhood requested review from baroquebobcat , jsirois and benjyw May 9, 2017

@benjyw

benjyw approved these changes May 9, 2017

# TODO(John Sirois): Consider removing this convenience:
# https://github.com/pantsbuild/pants/issues/346
# TODO(John Sirois): Introduce a label and replace the type test?
# TODO: We should deprecate this method, but doing so will require changes to JVM publishing.

This comment has been minimized.

@benjyw

benjyw May 9, 2017

Contributor

s/deprecate/kill/? It's not public API.

This comment has been minimized.

@stuhood

stuhood May 9, 2017

Member

True!

This comment has been minimized.

@stuhood

stuhood May 9, 2017

Member

Will update the ticket though, rather than the comment.

@stuhood stuhood merged commit 27038b9 into pantsbuild:master May 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/transitive-jvm-resources branch May 9, 2017

stuhood added a commit that referenced this pull request May 12, 2017

Include transitive Resources targets in PrepareResources. (#4569)
### Problem

The `resources` arg on JVM and Python targets is now deprecated, but the previous behaviour of only collecting directly declared resources onto the runtime classpath remains. The result for the JVM is that resources are only collected if they are directly depended on by a `JvmTarget`, rather than if they are located anywhere in the transitive dependencies of that target.

While this behaviour has existed for a while, it was difficult to run into it when resources were directly declared as "owned" by a particular target.

### Solution

Transitively collect the relevant `Resources` targets below `JvmTarget` roots in `PrepareResources`.

Also, I've verified on a very large codebase that this did not result in any adverse effects on resource availability.

### Result

`Resources` act (more: see #4568) like other dependencies generally do. Fixes #4567
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment