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

Ensure JarLibrary classpath entries have directory digests #6544

Merged
merged 8 commits into from Sep 25, 2018

Conversation

Projects
None yet
4 participants
@dotordogh
Copy link
Contributor

dotordogh commented Sep 21, 2018

A simple fix for #6432. There was a common method between coursier and ivy, so I broke it out into a new parent class.

Test cred to @illicitonion

@dotordogh dotordogh changed the title Dotordogh/6432 impl Ensure JarLibrary classpath entries have directory digests Sep 21, 2018

Dorothy Ordogh

@dotordogh dotordogh force-pushed the dotordogh:dotordogh/6432-impl branch from 327aa42 to 9c25512 Sep 22, 2018

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great, thanks!

@stuhood
Copy link
Member

stuhood left a comment

Not blocking, but: this might be cleaner as a change to ResolvedJar.

"""Adds jar classpath elements to the products of the provided targets.
The resolved jars are added in a way that works with excludes.
:param targets: The targets to add the jars for.
:param conf: The configuration.
:param resolved_jars_and_directory_digests: A list of tuples of ResolvedJars

This comment has been minimized.

@stuhood

stuhood Sep 24, 2018

Member

Would it be easier to add a digest to ResolverJar instead?

This comment has been minimized.

@baroquebobcat

baroquebobcat Sep 24, 2018

Contributor

I think adding a digest to ResolvedJar makes sense too. ResolvedJar links coordinates to locations of resolved artifacts. A digest is a sort of location, so it makes sense for ResolvedJar to have one.

@stuhood
Copy link
Member

stuhood left a comment

Thanks.

"""Get DirectoryDigests for jars and return them zipped with the jars.
:param jars: List of pants.java.jar.jar_dependency_utils.ResolveJar
:return: List of tuples of ResolveJar and pants.engine.fs.DirectoryDigest

This comment has been minimized.

@stuhood

stuhood Sep 24, 2018

Member

No longer a tuple.

This comment has been minimized.

@dotordogh

dotordogh Sep 24, 2018

Contributor

Good catch.

@@ -277,7 +276,7 @@ def test_jar_missing_pants_path_fails_adding(self):
cache_path='somewhere',
pants_path=None)])
self.assertEqual(
'Jar: org:name: has no specified path.',
'Jar: org:name:::jar has no specified path.',

This comment has been minimized.

@stuhood

stuhood Sep 24, 2018

Member

This is a bit odd... I don't think the digest should be affecting the coordinate?

This comment has been minimized.

@dotordogh

dotordogh Sep 24, 2018

Contributor

This must have been a conflict when I tried resetting the file to its master version. Will fix

Dorothy Ordogh added some commits Sep 24, 2018

@illicitonion illicitonion merged commit 59a79f5 into pantsbuild:master Sep 25, 2018

1 check passed

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

illicitonion added a commit that referenced this pull request Oct 3, 2018

Improve Noop resolution performance (#6577)
### Problem

In preparation for remote execution, #6544 introduced many calls to `capture_snapshots`, which added a significant overhead to coursier fetching.

### Solution

A partial solution is to unify all the snapshot capturing into a single call to `capture_snapshots`.

### Result

- Both coursier and ivy resolvers now make a single call to `capture_snapshots`, which reduced the overhead somewhat, though not completely. The synchronous call to `capture_snapshots` adds a 4-5 seconds of overhead to fetching from coursier on a relatively big project. A solution is discussed in a separate comment, below.

**EDIT** This is not true anymore, it was a faulty measurement on my part:
> 
> These are the noop measured times in my laptop. They all clean all and reset the daemon after every run:
> ```
> Measured execution time: 73s for commit 46e9dd9 (this pr)
> Measured execution time: 96s for commit 6268a77 (master with the regression)
> Measured execution time: 38s for commit f60c40a (master straight before the regression) 
> ```


- Separate test cases to cover ivy and coursier resolution (coursier wasn't covered before).
- The test cases now include 2 3rdparty dependencies, to cover for a logic bug introduced while developing this.



### Acknowledgements
Credit to @dotordogh for pairing on the issue and coming up with the final fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment