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

Relax serialized project cache checks #3907

Merged
merged 4 commits into from
Apr 5, 2022

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented Mar 31, 2022

The idea in #3375 to check the global Artifact store was good in general, BUT ...

it turned out that when the project is loading in FULL_ONLINE mode (implies no --offline switch), the NBTooling plugin will not report back any javadocs or sources even though they may exist in the local ~/.gradle jar cache. OK, after that the project is saved without any such artifacts, and the artifact store is updated with this imprecise info: no sources / javadocs would be registered (so eventually sources/javadocs maps can be completely empty). Consider, for example, if the var/cache in userdir is cleared, but the gradle (~/.gradle) caches are intact.

In some future run of the IDE, the project is fetched from the cache (without any source / javadoc), but GradleModuleFileCache21 can find them from their binary .jar - and the cached project is deemed invalid as info in IDE's artifact store differs from the gradle's caches. So, the project is not loaded from the cache, but again through Gradle daemon. This time, the FULL level is sufficient - and that one will load even source + javadoc info - that's stored in the project cache AND in the artifact store. But if eventually the project file contains some real problems and ONLINE mode activates, no source/javadoc update goes to the artifact store.

In addition, there are BOM-like projects, which do not have any binaries, just pom, for example. Such projects are always rejected, as binaries are always empty. It is sufficient for a project to have a dependency on just one POM-like module for the disk cache to be rejected.

This PR relaxes the checks, assuming that if the project does not contain any info for sources | javadocs, it's OK. The info will eventually accumulate. The jar/binary is still used to detect inconsistency between the cached state and gradle cache: if the cached project does not contain JAR artifacts, which are in gradle local cache, the project is most probably wrong and should be reloaded.

The PR improves diagnostics by logging problems encountered during project load; problems are printed with FINE level, so one could find out why a project is reloaded, not cached etc.

During debugging, I've found out that certain resolution errors are reported in offline mode as problems - but do not throw an exception that is caught in LegacyProjectLoader and cause re-load in online mode. The tooling now throws a specific exception in such case that is allowed to pass to the Loader.

@lkishalmi
Copy link
Contributor

Let me add some history on this.
So at the initial phase of the Gradle plugin, all JavaDoc and Source information was retrieved directly by the tooling plugin. These are treated as miscellaneous information. They are not serialized along the other project information,rather than kept a globally serialized artifact store updated.

With the introduction of the GradleModuleFileCache21 the globally serialized artifact store could be redundant, as the javadoc and source information could be retrieved directly from the cache.

So probably it would be wise to revisit the handling of this information and remove some code.

@sdedic
Copy link
Member Author

sdedic commented Mar 31, 2022

Yes, the description is misleading / wrong sorry; I am still working on some scenarios where serialized project is rejected despite the data in artifact store are still valid. For now just ignore the description - will update :) before going from draft stage.

Or do you mean the whole concept of the artifact store is superseded by the GradleModuleFileCache21 ?

@lkishalmi
Copy link
Contributor

Or do you mean the whole concept of the artifact store is superseded by the GradleModuleFileCache21 ?

Yes. I think that's possible. At the moment I can't name any use-case where GradleModuleFileCache21 with additional heuristics for Maven and File dependencies would not be enough.

@sdedic
Copy link
Member Author

sdedic commented Apr 1, 2022

Consolidated; description updated.

So probably it would be wise to revisit the handling of this information and remove some code.

It could be much better, but we are about to release vscode in a few weeks, and I'd rather not to disrupt things. So far the changes made here are pretty local, not affecting other things than the loading process - and greatly improve project loading time.

Let's do the cleanup in a separate PR.

@sdedic sdedic marked this pull request as ready for review April 1, 2022 14:06
@sdedic sdedic requested review from lkishalmi and ppisl April 1, 2022 14:06
@sdedic sdedic self-assigned this Apr 1, 2022
@sdedic sdedic added enhancement Gradle [ci] enable "build tools" tests performance labels Apr 1, 2022
@sdedic sdedic added this to the NB14 milestone Apr 1, 2022
Copy link
Member

@ppisl ppisl left a comment

Choose a reason for hiding this comment

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

I don't see anything problematic in the changes. I have also checkout the branch a little bit play/tested the funtionality and it seems to be ok.

@sdedic sdedic merged commit da88283 into apache:master Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Gradle [ci] enable "build tools" tests performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants