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

Improve priming from LSP client #7063

Merged
merged 5 commits into from
Mar 25, 2024
Merged

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented Feb 13, 2024

In Oracle Luna Labs environment, where Apache NBLS runs alongside Microsoft Java support, we've encountered an interesting situation:

  • the client starts from a pristine VM. No Gradle or Maven artifact caches, or locally downloaded artifacts.
  • when LSP client asks NBLS to open a project, NBLS checks if a priming build should run
  • Maven enables priming build if and only if project loading fails, or a (1) direct dependency is missing
  • at the time, NBLS Maven attempts to load the root project, the direct dependencies are already fetched by Microsoft Java initializing Maven in parallel to NBLS
  • NBLS is now able to load the project; however certain dependencies from the deep of the dependency tree are still being loaded to the computer. (2) But the priming build on the root project is now disabled - no need to prime.
  • NBLS skips prime, searches for subprojects and opens them (3) without priming
  • Subprojects contain direct dependencies on artifacts that the root project did not reference (althoug they may have been in the closure)
  • As subprojects open, they are analyzed. POM load fails and (4) a notification dialog will pop up (missing dependencies, missing properties)

So this fix contains two changes. Changes project analysis (1) in that not only direct dependencies, but indirect ones, too, will cause the project to be marked as "broken", suggesting to run a priming build (to download dependencies). This might be useful, since in NB21, when the user adds a dependency to pom.xml, it is not really visible in completion unless the dependency happens to be in the local repository - until the next build. This fixes (2).

And the second fix modifies (3) in that each newly discovered subproject is checked for priming action and potentially primed. The subproject check is then done again ... and again until the closure is searched fully.
Note that the parent primes first (if possible), so chances are all children will be primed during this process. And children are always processed only after the parent primes.

The 2nd+ child level processing will happen if the local repository is partially populated in a way that satisfies the parent (parent load does not notice any issues), but NOT the subprojects - this may be the case even after (1) is improved by this PR. This fixes (4).

@sdedic sdedic added LSP [ci] enable Language Server Protocol tests Maven [ci] enable "build tools" tests labels Feb 13, 2024
@sdedic sdedic added this to the NB22 milestone Feb 13, 2024
@sdedic sdedic self-assigned this Feb 13, 2024
@sdedic
Copy link
Member Author

sdedic commented Feb 16, 2024

Fixing the test was more tough than I expected. Why it failed: the Maven Problems inspector traverses artifacts (project.getArtifacts()), direct dependencies. Notes those that are not locally available. Earlier commits in this fix added "faked" artifacts created during project read. But I didn't pay attention to that these fakes are only created for POMs - not jars, but metadata of those jars. The resolver wrappers introduced by aaeabe1 allows to trace POM request back to the original artifact (which may have pom packaging !) and report that one. Final normalization of the missing artiact is done by ignoring artifact scope, as the scope is somewhat lost during the artifact resolution process - but it does not matter, the artifact was touched and needed during the project read. Without these steps, the Problems view either does not catch all problems, or displays duplicities (the same artifact in default and compile scope, for example).

Maybe the problem reporter could be rewritten to use recursive dependencies, or rely on Maven reading infrastructure with catching errors in resolver wrappers, so the info comes from a single source.

@sdedic sdedic requested a review from dbalek February 16, 2024 08:15
@sdedic
Copy link
Member Author

sdedic commented Mar 25, 2024

Rebased on latest master - Server.java had conflicts with fe5d373

@sdedic sdedic merged commit 13e62d7 into apache:master Mar 25, 2024
36 checks passed
@mbien
Copy link
Member

mbien commented Mar 28, 2024

@sdedic I haven't checked since when it happened and it might not be related to the priming changes here, but the micronaut tests throw NPEs during priming while green:
https://github.com/apache/netbeans/actions/runs/8444656505/job/23130853755#step:6:1262

@sdedic
Copy link
Member Author

sdedic commented Mar 28, 2024

@sdedic I haven't checked since when it happened and it might not be related to the priming changes here, but the micronaut tests throw NPEs during priming while green: https://github.com/apache/netbeans/actions/runs/8444656505/job/23130853755#step:6:1262

Will check, thanks for the alert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LSP [ci] enable Language Server Protocol tests Maven [ci] enable "build tools" tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants