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

Fix JarCreate invalidation in the presence of changing resources. #5030

Merged
merged 2 commits into from Oct 30, 2017

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented Oct 28, 2017

Problem

As described in #4595, JarCreate caches jar contents too aggressively. The root cause is that the JarTask superclass looks at "some of the dependencies of a target" (in the case of that particular ticket: resources), but the target invalidation in JarCreate is configured to only invalidate for changes directly in the target.

This is another case of the "whitelist based cache key" issue, and is one of the primary motivators for moving to isolated-process-executions as cache entries.

Solution

As described in the change: rather than attempting to whitelist exactly what JarTask touches, turning on invalidate_dependents is more durable in the face of further code changes. We do that here.

Result

Fixes #4595.

@ity

ity approved these changes Oct 30, 2017

Copy link
Member

ity left a comment

thanks!

@stuhood stuhood merged commit b4e1e1c into pantsbuild:master Oct 30, 2017

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/fix-jar-invalidation branch Oct 30, 2017

stuhood added a commit that referenced this pull request Nov 3, 2017

Fix JarCreate invalidation in the presence of changing resources. (#5030
)

### Problem

As described in #4595, `JarCreate` caches jar contents too aggressively. The root cause is that the `JarTask` superclass looks at "some of the dependencies of a target" (in the case of that particular ticket: resources), but the target invalidation in `JarCreate` is configured to only invalidate for changes directly in the target.

This is another case of the "whitelist based cache key" issue, and is one of the primary motivators for moving to isolated-process-executions as cache entries.

### Solution

As described in the change: rather than attempting to whitelist exactly what `JarTask` touches, turning on `invalidate_dependents` is more durable in the face of further code changes. We do that here.

### Result

Fixes #4595.

lenucksi added a commit to lenucksi/pants that referenced this pull request Nov 8, 2017

Merge branch 'master' into ticket3630
* master: (305 commits)
  Add configurable message when missing-deps-suggest doesn't have suggestions (pantsbuild#5036)
  Use split_whitespace for parsing of cflags. (pantsbuild#5038)
  Add documentation about strict deps (pantsbuild#5025)
  Fix JarCreate invalidation in the presence of changing resources. (pantsbuild#5030)
  Prepare the 1.4.0dev18 release. (pantsbuild#5034)
  Use the script verified identity when signing. (pantsbuild#5032)
  Dedup dependencies output (pantsbuild#5029)
  Have twine use the previously established pgp key during release. (pantsbuild#5031)
  [simple-code-gen] extension point for injecting extra exports (pantsbuild#4976)
  Prepare the 1.4.0dev17 release. (pantsbuild#5028)
  Content-addressable {file,directory} store and utility (pantsbuild#5012)
  [pantsd] Launch the daemon via a subprocess call. (pantsbuild#5021)
  Use the service deps if the target declares an exception. (pantsbuild#5017)
  Fix support for custom javac definitions (pantsbuild#5024)
  Pass references to Paths (pantsbuild#5022)
  Replace Blake2 with Sha256 (pantsbuild#5014)
  Revert pytest successful test caching in CI. (pantsbuild#5016)
  Fingerprint has from_hex_string, as_bytes, Display, and Debug (pantsbuild#5013)
  Fix memory leak in `./pants changed` (pantsbuild#5011)
  Move confluence related things to contrib (pantsbuild#4986)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment