Avoid storing timestamp and absolute paths in cache

[gsmet]

Hi,

This is using an approach similar to what was done for the formatter plugin (reusing the exact same code for writing the properties to the cache).

To give a bit a bit more context: we are using this plugin to manage imports in the Quarkus project and we are leveraging the Develocity build cache and I would like to store the cache file in the Develocity build cache which is shared among hosts.
We are also using the formatter plugin and in the formatter plugin, things were already handled this way so I mimicked exactly the behavior by adjusting the cache key to remove the base dir and removing the timestamp from the properties file.

Formatter code is here if you want to check: https://github.com/revelc/formatter-maven-plugin/blob/main/src/main/java/net/revelc/code/formatter/FormatterMojo.java#L606

Let me know if you see anything incorrect.

/cc @jprinet

Thanks!

[ctubbsii]

This change really seems like a one-time upgrade. It doesn't seem worth it to read-sort-strip-write the file every time. Could probably add a version number to the file, either in the filename or as the first line, to determine if it needs to be rewritten with a new format.

[gsmet]

@ctubbsii not really. The timestamp is added every time by Properties#store().

Now, we could write the file ourselves instead of relying on Properties#store(). That would be more optimized.
I decided against it because I wanted both plugins to be consistent and preferred reusing the code in the formatter plugin.

[ctubbsii]

@gsmet I'm not too familiar with the caching implementation. It was added by others to my plugin, and I've allowed that, even though I don't use it. I'm okay with these changes, but I think if they were modeled after formatter-maven-plugin's implementation, there may be a problem. @hazendaz had identified some problem with the caching behavior, that I'm not too familiar with. It may be necessary to fix that behavior in the formatter-maven-plugin before the behavior is mimicked here.

[hazendaz]

@ctubbsii The issue on formatter plugin you mention has to do with configuration that was added to it in recent versions which broke its usage causing the cache to be rewritten constantly as the key changes every time now (revelc/formatter-maven-plugin@d1d5852). So we use an older version still until that isn't broken that way.

The point here is to make sure the cache is not being written constantly as well as fixing paths. I have the same notes on this plugin needing that fixed so we never used the cache here either as its useless if it keeps getting updated (as it pertains to storage). However this plugin was always pretty fast so it was not a huge concern. There are other plugins that are substantially worse with no cache (ie license-maven-plugin for example).

The broader point here is storing the cache to ensure builds don't do wasteful activities. If you recall, originally I had set the default .cache to the root of the project on formatter-maven-plugin. I'll call that the poor mans solution as we didn't have develocity (gradle enterprise) at that time and maven didn't yet have a cache (still seemingly work in progress).

This improvement would allow us to do the same here. But again this plugin is super fast in general.

[gsmet]

But again this plugin is super fast in general.

The in general is important :). For a project the size of Quarkus, when building without tests, it starts to show :).

[gsmet]

I might be biased but really this PR is about making the cache file only containing information that is portable and reproducible. It only makes it better IMO and I don't think we are introducing any new issues.

[ctubbsii]

I think there are problems with this implementation.
First, it writes a file using Properties.store, which appends to any existing file. That could be a problem. I think that problem exists in the current implementation as well.

It then reads the entire contents of the file into memory, which it already had in memory before. I don't think it needs to do that.

It then sorts before it removes the timestamp. But, when it removes the timestamp, it makes a big assumption that there's only one comment line and it's at the top after sorting. It should have removed the comment line before sorting.

I have proposed changes that are much smaller, more efficient, and I think probably better, in #90. However, I would like others to review it.

[gsmet]

Couldn’t agree more that the impl is not ideal.

As I said, I copied the code of the other plugin in the hopes it would avoid too many discussions but we can close this if you prepared something better.