Maven project reload implementation

[sdedic]

Followup to #7651, implementation for Maven.
The implementation adapts maven internal project reloading to project reload SPI. It reports broken projects, project with missing dependencies and performs priming build to resolve the dependencies.

[dbalek]

Looks sane to me.

[mbien]

adding my obligatory: "I don't like priming" comment ;) It is too much IDE magic, projects could be primed by running a goal (with log showing in the output window IMO, e.g dependency:go-offline).

Features like priming or the invasive compile-on-save are from times when build tools like maven were lacking features (and times without mvnd and before NVMe).

NB is also currently breaking mvn 4 split repos during project initialization #6190, I never had time to look into this unfortunately so far.

[sdedic]

adding my obligatory: "I don't like priming" comment ;) It is too much IDE magic, projects could be primed by running a goal (with log showing in the output window IMO, e.g dependency:go-offline).

Yes, this is in the queue. It's just I/we need 'wait-for-pending-load' earlier. Priming is not necessary soldified by this API semantics as the declared quality "RESOLVED" in this API only asks to make dependencies available "somehow".

[neilcsmith-net]

I share @mbien concerns with priming, and think your queue priority might need a rethought. But what's the intended use cases here? No loads / reloads without user interaction? eg. executing mvnw dependency:go-offline might also open up some issues.

[sdedic]

But what's the intended use cases here?

Basically "any operation that I need a certain project state for". You may be satisfied with project dependencies from the last-read state .... or you'd like to get current dependencies even though it means wait for reload.

At the moment, one cannot check & wait after new MavenProject is loaded (you may force a reload though). Your op may be OK with SIMPLE quality, when the project barely knows about its subprojects (modules) ... or you demand better (for whatever reason specific to your operation). You can be able to work with broken project ... or not.

After POM modification (i.e. adding dependency, adding a plugin/configuration), one better wait for the model to (re)load before doing further actions or inspections. Similar for build.gradle

At the moment, we do not have such indicators/functions - or partially and unique in each gradle/maven/ant, and there are already implementations that fork branches based on project type or ignore the condition in a hope it "will work somehow" ... and it usually does, but exhibits time-related bugs right after modification or checkout.

eg. executing mvnw dependency:go-offline might also open up some issues.

I'm lost, can you be more specific ?

[neilcsmith-net]

Thanks! I'm concerned about introducing anything that might auto-execute the build tool without warning, particularly but not exclusively if the projects aren't trusted. It should be clear when any operation results in tool execution. I can understand the need to reload with POM modification, but that shouldn't happen auto-magically. With that comment, I particularly meant making sure we don't get into a situation like with the need to add a warning in the Micronaut wizard that was downloading and executing any old mvnw without knowledge!

possibly. Management of spare-time is rather tough.

That depends on who the "we" is that needs this feature! 😉 Most of us are putting our spare time into this.

[sdedic]

Thanks! I'm concerned about introducing anything that might auto-execute the build tool without warning, particularly but not exclusively if the projects aren't trusted.

Note: currently one could execute ACTION_PRIME with the same effect... and Maven does not implement trust concept, at the moment. This API contains a flag to mark the project trusted ( processed by the implementation), so if Maven impl some day implements trust checks, it will be able to fail the request with a documented status.
BTW - with the API, trust concept can be added 'centrally' in front of passing the request to the impl (which can still do its own checks).

I can understand the need to reload with POM modification, but that shouldn't happen auto-magically.

The fact is, that POM reload already happens mostly automagically, for all opened projects. Maven module itself schedules a reload on the background as well as Gradle. But an observer/client lacks a handle to wait for that schedule to complete. This API does does not do that on itself though - it needs "someone" who requests a refresh.

With that comment, I particularly meant making sure we don't get into a situation like with the need to add a warning in the Micronaut wizard that was downloading and executing any old mvnw without knowledge!

Maven / Gradle display at least cancellable progress indicator. The implementation is free to prompt the user (i.e. Maven impl might be changed to prompt before launching the priming build, since at that time, it already knowns some artifacts are missing). BTW - neither gradle or maven does do that atm when doing "priming" or "full_online" load. If one requests NbGradle.Quality.FULL_ONLINE on a trusted project, it will happily download all the world.

That depends on who the "we" is that needs this feature! 😉 Most of us are putting our spare time into this.

Yeah, I realized that while you were replying and ashamed deleted the comment. sorry.

@neilcsmith-net ... as I am replying, I am getting a feeling that the API does not actually make things more "automagic", but the discussion abot it itself surfaces some areas that are automagic already for years. It seems to me that the API just formalizes actions .... and we can discuss (even separately) what policies should be set up (and then enforced by the API core).

[neilcsmith-net]

No worries! 😄

Yes, I know there's quite a bit of reloading going on already. I guess the thing that concerns me is that ideally we should be delegating to the build tool execution more as @mbien says, but then it opens up more concerns with the things we're doing auto-magically executing things we have less control over?! When I did the initial mvnw PR I gave a bit of thought to whether we needed project trust in there, and I'm not sure if I know the answer still! 😆

The implementation is free to prompt the user

That's just making me think that the base API implementation is possibly the place to have a (possibly pluggable) UI that all requests go through.

[sdedic]

Re.: trust - Maven is in a little better than gradle here. As maven is declarative (with exceptions of scripting plugins), the malicious artifact would have to be signed + come from a plugin repository .... so if

• the project ONLY uses trusted(*) plugin repositories
• it does not use user-code-executing(*) (scripting) plugins
• it does not use mvnw (as the script may be scewed up)
  ... then it's moderately safe to execute the build tool

Gradle is bad, since the build.gradle checked out from a random git is executable itself with no sanbox and fs access, so trust needs to be tight.

In fact I originally had a prototype of ProjectTrust ;)) similar to Gradle that would just mark a folder (or its parent) as 'trusted' + TrustProvider SPI that could (based on its own logic) make a project (folder) trusted. But I shelved into the work queue :)

IMHO these policymaking discussions should happen on mailing list perhaps to get broader attention. I would like to trim down the discussion in this PR just to things that the API (or its maven impl) makes "more open" than they are now (IMHO: none, as things like priming or force-maven-load can be done with Maven friend apis even now)

• -- we don't have a configuration/setting for that :)

[mbien]

IMHO these policymaking discussions should happen on mailing list perhaps to get broader attention

yep. I wanted to do that between releases since it is a broader topic where priming is only one aspect, but since this here is fresh i quickly wrote it down today: https://lists.apache.org/thread/lgfvt0649rlg83j46bcczxj30y87qh95

[sdedic]

OK, I would like to focus the PR discussion back to what this PR's code does.

The API itself does not say "how" (that's up to the implementation), just gives the client a way to request a certain project state - and blocks the operation (failed Future) if the implementation does not fullfill it. IMHO, overall it is better that clients DO NOT run "priming build" (which can fail, but the failure cannot be detected reliably btw) explicitly e.g. when classpath is incomplete, but instruct the project system to "make the project resolved" (somehow). The less the client knows what it means "resolve project" the better.

The Reload API implementation in this PR now does just what would be done if the code was maven module's friend and used the existing friend APIs. We should still (elsewhere) argue if there should be a defined policy, or what that policy should be and why. But it's out of scope of this PR I think, since it does not introduce anything not done already - it actually hides the details behind an abstract API.

[neilcsmith-net]

Fine from my perspective. I think the issue of making things explicit and controllable is better handled in (a possibly future iteration) of ProjectReload.withProjectState() and StateRequest.

[sdedic]

Last call ... will merge Maven impl as soon as I resolve the conflicts.

[sdedic]

Squashed + rebased on the API PR (merged to master).

[mbien]

saw this after merging something to master:

testReloadFailsWithPomModified: org.netbeans.modules.maven.queries.MavenReloadImplementationTest
java.lang.IllegalStateException
	at org.netbeans.modules.project.dependency.reload.ProjectReloadInternal.assertNoOperations(ProjectReloadInternal.java:757)
	at org.netbeans.modules.maven.queries.MavenReloadImplementationTest.tearDown(MavenReloadImplementationTest.java:110)
	at org.netbeans.junit.NbTestCase.runBare(NbTestCase.java:530)
	at org.netbeans.junit.NbTestCase.run(NbTestCase.java:298)

https://github.com/apache/netbeans/actions/runs/10592522502

[sdedic]

@mbien thanks; will try to stress-test and fix.