[BREAKING-CHANGE] [NCL-6230] store and share sizes of not built artifacts

[michalovjan]

This is a breaking change for 3 reasons:

1. checksums-* files now store filesize which is a format change therefore breaking
2. Infinispan cache has a changed format too because of additionally storing information about size (now it stores a Class instead of simple String) which is also a format change. This means that the already built caches need to be dropped.
3. DistributionAnalyzer API has changed

[codecov]

Codecov Report

Merging #297 (b8658fa) into master (cd966fd) will decrease coverage by 0.44%.
The diff coverage is 32.94%.

@@             Coverage Diff              @@
##             master     #297      +/-   ##
============================================
- Coverage     69.00%   68.55%   -0.45%     
- Complexity      622      629       +7     
============================================
  Files            38       39       +1     
  Lines          3049     3101      +52     
  Branches        378      379       +1     
============================================
+ Hits           2104     2126      +22     
- Misses          769      801      +32     
+ Partials        176      174       -2     

Impacted Files	Coverage Δ	Complexity Δ
...main/java/org/jboss/pnc/build/finder/cli/Main.java	23.99% <4.16%> (-0.41%)	15.00 <0.00> (ø)
...ss/pnc/build/finder/core/DistributionAnalyzer.java	64.12% <13.33%> (-1.37%)	65.00 <0.00> (ø)
...ava/org/jboss/pnc/build/finder/core/LocalFile.java	27.27% <27.27%> (ø)	3.00 <3.00> (?)
...java/org/jboss/pnc/build/finder/core/Checksum.java	74.41% <75.00%> (+0.50%)	28.00 <5.00> (+2.00)
...a/org/jboss/pnc/build/finder/core/BuildConfig.java	96.82% <100.00%> (ø)	71.00 <1.00> (ø)
.../jboss/pnc/build/finder/core/BuildFinderUtils.java	89.33% <100.00%> (+0.07%)	28.00 <0.00> (ø)
...rg/jboss/pnc/build/finder/core/ConfigDefaults.java	100.00% <100.00%> (ø)	1.00 <0.00> (ø)
...ava/org/jboss/pnc/build/finder/core/JSONUtils.java	100.00% <100.00%> (ø)	5.00 <1.00> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd966fd...eb1b549. Read the comment docs.

[dwalluck]

One final comment on the breaking cache change. Unfortunately, while we have a version for the externalizer, we do not store a version for the cache itself.

Can we at least catch the exception thrown when the old version of build-finder cannot load the new cache (given that this is a breaking change) and at least print an error to better inform the user what to do? Something like:

LOGGER.error("Error loading cache {}: {}. The cache format has changed and you will have to manually delete the existing cache", boldRed(pathToCache), boldRed(e.getMessage()), e);

[michalovjan]

One final comment on the breaking cache change. Unfortunately, while we have a version for the externalizer, we do not store a version for the cache itself.

Can we at least catch the exception thrown when the old version of build-finder cannot load the new cache (given that this is a breaking change) and at least print an error to better inform the user what to do? Something like:

LOGGER.error("Error loading cache {}: {}. The cache format has changed and you will have to manually delete the existing cache", boldRed(pathToCache), boldRed(e.getMessage()), e);

I'll see what I can do.

[dwalluck]

OK, I guess you got this from Main.initCaches which has String location = new File(ConfigDefaults.CONFIG_PATH, "cache").getAbsolutePath();.

But, here is not the right place for it. We need the actual value it has been set to.

Then we have Main call something to set it, and only return the default path if unset.

Also, we could define a global default in ConfigDefaults, but defaulting to CONFIG_PATH, "cache" is OK, too.

[michalovjan]

I've added an option to specify cache location so that we take the value from config instead of creating it on the spot. Is this a suitable solution? :)

[dwalluck]

The current value appears to be ConfigDefaults.CONFIG_PATH + "/cache".

Rather than specify the cache directory, we should just specify on the command line the default app home directory, which is normally ${user.home}/.build-finder and allow the user to change this location, but then the cache path would always be relative to that location. What do you think?

[michalovjan]

Yeah, I was initially against having the option to specify the cache-location because I didn't like the implementation but it is not that ugly I suppose. Keeping it relative to the app-directory is better, I agree.

[michalovjan]

Yeah, I like your proposal much more. :)

[dwalluck]

Is it possible to change the cache location now? Looking at the code, I wasn't sure, the ConfigDefaults file was changed, but these paths should be immutable.

[michalovjan]

No, it shouldn't be possible to change cache location. The cache location is bound to the root path (app home directory), which is unchangeable.