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

Single resolve with coursier #5362

Merged
merged 12 commits into from Jan 24, 2018

Conversation

Projects
None yet
2 participants
@wisechengyi
Copy link
Contributor

wisechengyi commented Jan 19, 2018

Problem

Earlier jar_dependencys are divided into classifiers for coursier to resolve because coursier's --classifier option is applied globally.

Solution

Add the classifier specification on module level coursier/coursier#735. This is the pants side of the change which removes the classifier grouping.

Result

See added back test case.

Note: finding the correct strict subset jars for transitive dependencies is still an issue coursier/coursier#743. However, it is very, very rare.

@wisechengyi wisechengyi requested a review from stuhood Jan 20, 2018

@wisechengyi wisechengyi requested a review from baroquebobcat Jan 20, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks a lot Yi.

help='Location to download a bootstrap version of Coursier.')
# TODO(wisechengyi): currently using a custom url for fast iteration.
# Once the coursier builds are stable, move the logic to binary_util
# Ths sha in the version corresponds to the sha in the PR https://github.com/coursier/coursier/pull/692

This comment has been minimized.

@stuhood

stuhood Jan 22, 2018

Member

Please update this to indicate where this artifact is coming from. It's really disconcerting to be relying on anonymous code, and having a complete list of required PRs to get on master releases (here, or via context at the link) would be good.

This comment has been minimized.

@wisechengyi

wisechengyi Jan 23, 2018

Contributor

Added the reference back.

The goal is still use binary util down the road #5381 once the work in coursier has stabilized.

transitive_jar_path_for_coord = []
if my_simple_coord in flattened_resolution:
for c in [my_simple_coord] + flattened_resolution[my_simple_coord]:
# TODO(wisechengyi): this only grabs jar with matching classifier the current coordinate

This comment has been minimized.

@stuhood

stuhood Jan 22, 2018

Member

Is this the other PR you were referring to? Could you link directly to it here please?

This comment has been minimized.

@wisechengyi
transitive_jar_path_for_coord = []
if my_simple_coord in flattened_resolution:
for c in [my_simple_coord] + flattened_resolution[my_simple_coord]:
# TODO(wisechengyi): this only grabs jar with matching classifier the current coordinate
# and it still takes the jars wholesome for its transitive dependencies.

This comment has been minimized.

@stuhood

stuhood Jan 22, 2018

Member

s/wholesome/wholesale/ maybe?

This comment has been minimized.

@wisechengyi

wisechengyi Jan 23, 2018

Contributor

corrected.

@wisechengyi wisechengyi merged commit cd484d3 into pantsbuild:master Jan 24, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@wisechengyi wisechengyi deleted the wisechengyi:single_coursier branch Jan 24, 2018

@stuhood stuhood added this to the 1.4.x milestone Jan 24, 2018

stuhood added a commit that referenced this pull request Jan 24, 2018

Single resolve with coursier (#5362)
### Problem

Earlier `jar_dependency`s are divided into classifiers for coursier to resolve because coursier's `--classifier` option is applied globally.

### Solution
Add the classifier specification on module level coursier/coursier#735. This is the pants side of the change which removes the classifier grouping.

### Result

See added back test case. 

Note: finding the correct strict subset jars for transitive dependencies is still an issue coursier/coursier#743. However, it is very, very rare.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment