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

Make coursier resolve more friendly #5675

Merged
merged 14 commits into from Apr 9, 2018

Conversation

Projects
None yet
3 participants
@wisechengyi
Copy link
Contributor

wisechengyi commented Apr 8, 2018

Address points from #5382

  • Extract the following coursier options and assign sensible defaults
    • artifact types
    • cache dir
    • fetch options
    • repos
  • Add --report flag to show coursier output
  • Add documentation

wisechengyi added some commits Apr 7, 2018

fmt

@wisechengyi wisechengyi changed the title [WIP] make coursier resolve more friendly Make coursier resolve more friendly Apr 8, 2018

@wisechengyi wisechengyi requested review from stuhood , baroquebobcat and benjyw Apr 8, 2018

@stuhood

stuhood approved these changes Apr 9, 2018

Copy link
Member

stuhood left a comment

Thanks Yi: these look great. Would be nice to improve the UX on --report, as the status quo isn't great.

* To show coursier command line invocation, use `-ldebug`

:::bash
./pants --cache-ignore --resolver-resolver=coursier resolve.coursier --report examples/tests/scala/org/pantsbuild/example/hello/welcome -ldebug

This comment has been minimized.

@stuhood

stuhood Apr 9, 2018

Member

Why is the --cache-ignore necessary here? Assuming this isn't to avoid a bug (as is the case with ivy), making --report and --open fingerprint=True options would be preferable.

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Contributor

--cache-ignore means to ensure coursier is called, in case

./pants --resolver-resolver=coursier resolve.coursier --report examples/tests/scala/org/pantsbuild/example/hello/welcome

is called twice in a row, then the 2nd run will just be validated. I wanted to use invalidate but that doesn't work anymore due to #5674

fingerprint --report makes sense

This comment has been minimized.

@stuhood

stuhood Apr 9, 2018

Member

then the 2nd run will just be validated

If the --report option is specified, then we should consider that a bug, IMO. Basically, the implementation of --report should be invoked outside of the invalidated block, because it is a sideeffect ... so I actually take back what I said about fingerprinting it: if it's outside the block, the fingerprint isn't relevant to it.

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Contributor

Gotcha. will make --report essentially doing a forced run.

@benjyw

benjyw approved these changes Apr 9, 2018

Copy link
Contributor

benjyw left a comment

Thanks!

@stuhood

stuhood approved these changes Apr 9, 2018

Copy link
Member

stuhood left a comment

Thanks!

@@ -57,7 +57,7 @@ def register_options(cls, register):
super(CoursierMixin, cls).register_options(register)
register('--allow-global-excludes', type=bool, advanced=False, fingerprint=True, default=True,
help='Whether global excludes are allowed.')
register('--report', type=bool, advanced=False, fingerprint=True, default=False,
register('--report', type=bool, advanced=False, fingerprint=False, default=False,

This comment has been minimized.

@stuhood

stuhood Apr 9, 2018

Member

False is the default, so unless you're leaving this here as a "comment" indicating that this is intentionally not fingerprinted, would remove it. Although a comment wouldn't hurt either.

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Contributor

fingerprint=False and added the behavior to help msg

# Check each individual target without context first
if not invalidation_check.invalid_vts:

# If a report is requested, do not proceed with loading validated result.

This comment has been minimized.

@stuhood

stuhood Apr 9, 2018

Member

It would be good to add a test of the report case, as it broke accidentally in ivy, and seems liable to break here as well.

This comment has been minimized.

@wisechengyi

wisechengyi Apr 9, 2018

Contributor

Good idea. Done.

wisechengyi added some commits Apr 9, 2018

@wisechengyi wisechengyi merged commit 881eb3a into pantsbuild:master Apr 9, 2018

1 check passed

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

@wisechengyi wisechengyi deleted the wisechengyi:coursier_sub branch Apr 9, 2018

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