Relativize jar_dependency.base_path #4326

Merged
merged 4 commits into from Mar 14, 2017

Conversation

Projects
None yet
3 participants
@peiyuwang
Contributor

peiyuwang commented Mar 13, 2017

Problem

The issue with 1366ba4 is the cached ivy resolution.json still contains the absolute path. Before 1366ba4 it was absolute, but fingerprinting based on absolute path will always result cache miss, so the abs url is never used. Now with 1366ba4 using relative path for fingerprint, it's a cache hit and ivy resolver using absolute url from another host will eventually fail.

Solution

Make base_path relative to build_root and save both relative url and base_path to json.

Result

Before ivy/15518434f0a1b64cad9945f5b65c3107669a77e1-IvyResolveFingerprintStrategy_af8f5f9a707f/resolution.json contains:

{"default": {"target_to_coords": {"//:scrooge-linter": ["un:used:5b57267c90aa46dafe53651d927f9aae05aaf38b::jar"]}, "coord_to_attrs": {"un:used:5b57267c90aa46dafe53651d927f9aae05aaf38b::jar": {"url": "file:///a/long/sandbox/.../path/.pants.d/bootstrap/internal-tools/252d64521cf9/scrooge.scrooge-linter.bin/current/tool.jar"}}}}

Now:

{"default": {"target_to_coords": {"//:scrooge-linter": ["un:used:79b462b8e63783da0ca96fc828c3ab7ab5c2b63d::jar"]}, "coord_to_attrs": {"un:used:79b462b8e63783da0ca96fc828c3ab7ab5c2b63d::jar": {"url": "file:.pants.d/bootstrap/internal-tools/252d64521cf9/scrooge.scrooge-linter.bin/current/tool.jar", "base_path": "."}}}}

@peiyuwang peiyuwang requested review from stuhood and wisechengyi Mar 13, 2017

@stuhood

A new test for this would be great.

@@ -296,8 +296,9 @@ def add_resolved_jars(self, target, resolved_jars):
# Assuming target is a jar library.
for j in target.jar_dependencies:
- if j.get_url():
- self.coordinate_to_attributes[j.coordinate] = {'url': j.get_url()}
+ url = j.get_url(relative=True)

This comment has been minimized.

@stuhood

stuhood Mar 13, 2017

Member

Please bump the implementation_version of all tasks that use ivy_utils.

@stuhood

stuhood Mar 13, 2017

Member

Please bump the implementation_version of all tasks that use ivy_utils.

This comment has been minimized.

@peiyuwang

peiyuwang Mar 14, 2017

Contributor

Done.

Added tests.

Good call for that, I was able find and fix a few things

  • relative path should be computed using abspath against (build_root + basepath), build_root was missed.
  • normpath so we get a/b/c instead of /a/b/./c
@peiyuwang

peiyuwang Mar 14, 2017

Contributor

Done.

Added tests.

Good call for that, I was able find and fix a few things

  • relative path should be computed using abspath against (build_root + basepath), build_root was missed.
  • normpath so we get a/b/c instead of /a/b/./c
@stuhood

Thanks!

@peiyuwang peiyuwang merged commit 9e843b8 into pantsbuild:master Mar 14, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@peiyuwang peiyuwang deleted the peiyuwang:peiyu/relatize-jar-dependency-base-path branch Mar 14, 2017

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

Relativize jar_dependency.base_path (#4326)
### Problem

The issue with 1366ba4 is the cached ivy `resolution.json` still contains the absolute path. Before 1366ba4 it was absolute, but fingerprinting based on absolute path will always result cache miss, so the abs url is never used. Now with 1366ba4 using relative path for fingerprint, it's a cache hit and ivy resolver using absolute url from another host will eventually fail.

### Solution

Make `base_path` relative to build_root and save both relative `url` and `base_path` to json.

### Result

Before `ivy/15518434f0a1b64cad9945f5b65c3107669a77e1-IvyResolveFingerprintStrategy_af8f5f9a707f/resolution.json` contains:
```
{"default": {"target_to_coords": {"//:scrooge-linter": ["un:used:5b57267c90aa46dafe53651d927f9aae05aaf38b::jar"]}, "coord_to_attrs": {"un:used:5b57267c90aa46dafe53651d927f9aae05aaf38b::jar": {"url": "file:///a/long/sandbox/.../path/.pants.d/bootstrap/internal-tools/252d64521cf9/scrooge.scrooge-linter.bin/current/tool.jar"}}}}
```
Now:
```
{"default": {"target_to_coords": {"//:scrooge-linter": ["un:used:79b462b8e63783da0ca96fc828c3ab7ab5c2b63d::jar"]}, "coord_to_attrs": {"un:used:79b462b8e63783da0ca96fc828c3ab7ab5c2b63d::jar": {"url": "file:.pants.d/bootstrap/internal-tools/252d64521cf9/scrooge.scrooge-linter.bin/current/tool.jar", "base_path": "."}}}}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment