Skip to content
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

Toward a machine-independent incremental cache file #188

Merged
merged 2 commits into from
Feb 18, 2021

Conversation

gontard
Copy link
Contributor

@gontard gontard commented Feb 16, 2021

The objective is to produce a machine-independent cache in order to include sbt-scalafix cache files in the remote cache artifact.
Two things are missing:

  1. Cache stamps must be persisted with relative paths instead of absolute paths (Machine-independent file stamping to embed caches in remote cache artifacts sbt/sbt#6298)
  2. sbt-scalafix have to use hash instead of lastModified style for cache stamping.

This PR solves the second point.

To make the scripted tests green, i had to permit to get back the previous behaviour (lastModified). Done with a system property.

@gontard gontard force-pushed the caching_style branch 2 times, most recently from 82d2605 to 438f418 Compare February 16, 2021 06:44
Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR 👍 That's a good first step towards caching sbt-scalafix executions across machines.

src/main/scala/scalafix/sbt/ScalafixPlugin.scala Outdated Show resolved Hide resolved
build.sbt Outdated
@@ -92,5 +92,6 @@ sbtPlugin := true
scriptedBufferLog := false
scriptedLaunchOpts ++= Seq(
"-Xmx2048M",
s"-Dplugin.version=${version.value}"
s"-Dplugin.version=${version.value}",
"-Dsbt-scalafix.uselastmodified=true"
Copy link
Collaborator

@bjaglin bjaglin Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"-Dsbt-scalafix.uselastmodified=true"
"-Dsbt-scalafix.uselastmodified=true" // the caching scripted relies on sbt-scalafix only checking file attributes, not content

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I don't have a better idea than changing the style for scripted (and I think a system property is better/more hidden than a setting key), as

$ exec chmod 000 src/main/scala/Valid.scala
would no longer work by default: with hashing, a file access won't necessarily mean that the file is being read by protoc (but just hashed beforehand).

@gontard gontard force-pushed the caching_style branch 4 times, most recently from e74ac0c to 800bd90 Compare February 17, 2021 15:22
@gontard gontard force-pushed the caching_style branch 2 times, most recently from df9afba to 2ae3d1b Compare February 18, 2021 16:27
* Switch from lastModified to hash caching style in order to produce
machine-independent file stamps once sbt/sbt#6298 will solved.
* Add a system property "sbt-scalafix.uselastmodified" to force the
use of the lastModified caching style. (Used in scripted tests)
Stamps scalafix arguments using a hash of the files instead of
relying on the lastMofified property in order to produce a
machine-independent "args" cache.
@bjaglin bjaglin marked this pull request as ready for review February 18, 2021 17:26
@bjaglin bjaglin changed the title Use FileInfo.hash as caching style Toward a machine-independent incremental cache file Feb 18, 2021
Copy link
Collaborator

@bjaglin bjaglin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bjaglin bjaglin merged commit b3db20a into scalacenter:master Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants