Fix broken export-classpath #4603

Merged
merged 1 commit into from May 17, 2017

Conversation

Projects
None yet
3 participants
@baroquebobcat
Contributor

baroquebobcat commented May 17, 2017

Problem

I'd assumed that ClassPathUtils.classpath would take just the root targets, and that it didn't apply excludes correctly when the closure was passed. That was incorrect.

Doing that would cause export-classpath to return an invalid classpath

Solution

Revert that change, and update the regression test to be more comprehensive.

Result

export-classpath works correctly again, and the regression test covers the excludes case more effectively.

Fix broken export-classpath
### Problem

I'd assumed that ClassPathUtils.classpath would take just the root targets, and that it didn't apply excludes correctly when the closure was passed. That was incorrect.

Doing that would cause export-classpath to return an invalid classpath

### Solution

Revert that change, and update the regression test to be more comprehensive.

### Result

export-classpath works correctly again, and the regression test covers the excludes case more effectively

@baroquebobcat baroquebobcat requested review from stuhood and JieGhost May 17, 2017

@stuhood

Thanks.

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

@baroquebobcat baroquebobcat merged commit c0a8b21 into pantsbuild:master May 17, 2017

1 check passed

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

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

Fix broken export-classpath (#4603)
### Problem

I'd assumed that ClassPathUtils.classpath would take just the root targets, and that it didn't apply excludes correctly when the closure was passed. That was incorrect.

Doing that would cause export-classpath to return an invalid classpath

### Solution

Revert that change, and update the regression test to be more comprehensive.

### Result

export-classpath works correctly again, and the regression test covers the excludes case more effectively.
@baroquebobcat

This comment has been minimized.

Show comment
Hide comment
@baroquebobcat

baroquebobcat Aug 18, 2017

Contributor

This fixed a bug in #4592, which wasn't tested properly. Commenting so that the link between them is clearer.

Contributor

baroquebobcat commented Aug 18, 2017

This fixed a bug in #4592, which wasn't tested properly. Commenting so that the link between them is clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment