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

Support custom index files for incremental up-to-date checking #1055

Merged
merged 7 commits into from
Jan 6, 2022

Conversation

tisoft
Copy link
Contributor

@tisoft tisoft commented Dec 29, 2021

No description provided.

@tisoft
Copy link
Contributor Author

tisoft commented Dec 29, 2021

This augments #935 with the ability to set a custom index file location. This can be used to have the index file somewhere, it isn't deleted on each clean build.

@lutovich Please check, that I didn't break your code. 😄

@nedtwigg
Copy link
Member

Is it your intention to check the index file into source control? The index file contains filesystem timestamps, so I don't think that it will work well for that.

When a user does a clean, I think they usually want to redo the work from scratch for the next invocation, that's the point of performing a clean.

I'm okay with merging this, but I'd like to understand the usecase better.

@tisoft
Copy link
Contributor Author

tisoft commented Dec 30, 2021

Formatting for our codebase takes about 5 minutes. Our developers do clean builds fairly often, so the cache would be deleted very often and don't help us much. We do have a .cache directory in our projects, where e.g. the maven-frontend-plugin downloads node/npm. This would be the perfect location for us to put the index files. The directory is not checked into source control, it is not deleted on every clean, but we do have a custom mechanism, that cleans it from time to time, so "broken" index files can be cleaned up again.

@lutovich
Copy link
Contributor

lutovich commented Jan 4, 2022

Hi @tisoft, thank you for this PR!

Is your project a multi-module Maven project where each submodule has a .cache directory? If yes, then your configuration would probably look something like:

<upToDateChecking>
  <enabled>true</enabled>
  <indexFile>${project.basedir}/.cache/custom-index-file</indexFile>
</upToDateChecking>

and live in the project's root POM. Is my understanding correct?

I think in a multi-module project indexFile configuration could make it easy for users to misconfigure Spotless. Multiple submodules could point to the same file by mistake. The plugin does not do file locking or anything else to ensure exclusive read/write access to the index file. Example of an incorrect configuration:

<upToDateChecking>
  <enabled>true</enabled>
  <indexFile>/home/custom-index-file</indexFile>
</upToDateChecking>

That's why I'm thinking perhaps the new configuration could be indexFileDirectory. File name would be generated by the plugin as spotless-index-<groupId>-< artifactId>-<version> to make sure each submodule has its own file.

What do you think?

@tisoft
Copy link
Contributor Author

tisoft commented Jan 5, 2022

Hi,

in our use case the .cache directory is in the root directory of the multi-module project. Our configuration would look like this:

<upToDateChecking>
  <enabled>true</enabled>
  <indexFile>${maven.multiModuleProjectDirectory}/.cache/${project.groupId}_${project.artifactId}.index</indexFile>
</upToDateChecking>

I agree, that it's easily possible to wrongly configure this setting. Specifying an indexDirectory instead would work for us.
Personally I like setting the indexFile directly more, since that gives me more control, but that's maybe just me. 😁
What about you, @nedtwigg? Would you prefer this to be changed to a directory setting? I would change the PR then. 😄

(I wouldn't include the project version in the filename, there are maven plugins, that automatically set the version based on git commits and that would generate a new index file everytime a new commit is checked out, also cleaning the index file would only work for the exact same version)

@lutovich
Copy link
Contributor

lutovich commented Jan 5, 2022

Interesting, I did not know about the maven.multiModuleProjectDirectory property.

The indexFile setting is good, I was just exploring potential edge cases. Maven checkstyle plugin uses a cacheFile property. So it's not that unusual to configure an exact file.

@tisoft
Copy link
Contributor Author

tisoft commented Jan 5, 2022

Rebased against current main.

@nedtwigg nedtwigg requested a review from lutovich January 5, 2022 22:23
@nedtwigg
Copy link
Member

nedtwigg commented Jan 5, 2022

@lutovich this LGTM, but if you want more changes please request them. If it looks good to you too then push the merge button :)

nedtwigg and others added 2 commits January 5, 2022 21:29
 * use java.nio APIs for file manipulations in UpToDateCheckingTest
 * simplify index file creation in NoopCheckerTest
 * annotate getter's return type with `@Nullable`
Copy link
Contributor

@lutovich lutovich left a comment

Choose a reason for hiding this comment

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

Looks good. I pushed a few minor refactorings.
Thanks again @tisoft!

@lutovich lutovich merged commit e98684a into diffplug:main Jan 6, 2022
@tisoft tisoft deleted the custom_index_file branch January 6, 2022 11:54
@nedtwigg
Copy link
Member

nedtwigg commented Jan 6, 2022

Released in 2.19.0.

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.

None yet

3 participants