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
Priming build and reload improvements. #6514
Priming build and reload improvements. #6514
Conversation
b6fe560
to
1f27de7
Compare
@@ -374,7 +391,8 @@ public MavenProject loadParentOf(MavenEmbedder embedder, MavenProject project) t | |||
request.setRepositorySession(maven.newRepositorySession(req)); | |||
|
|||
if (project.getParentFile() != null) { | |||
parent = builder.build(project.getParentFile(), request).getProject(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Former call to DefaultProjectBuilder.build
does not contain error handling / processing as implemented in MavenProjectCache
. The other branch works with (nonlocal) artifacts, it is still TBD but serves mainly as a fallback.
@@ -544,12 +578,6 @@ void stopHardReferencingMavenPoject() { | |||
model = null; | |||
} | |||
newproject = MavenProjectCache.getMavenProject(this.getPOMFile(), reload); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getMavenProject never returns null
, but a fallback can be returned.
@@ -94,7 +91,7 @@ public class ReactorChecker implements PrerequisitesChecker { | |||
* @return its apparent reactor root; maybe just the same project | |||
*/ | |||
public static @NonNull NbMavenProject findReactor(@NonNull NbMavenProject module) { // #197232 | |||
MavenProject prj = module.getMavenProject(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is important, as a subproject cannot be typically read with empty Maven repository. But the partial project contains a correct parent reference.
50523e5
to
5b5e083
Compare
c24b79c
to
56ff0a3
Compare
just a general comment: I can't remember I ever primed anything using NetBeans (I am using the maven support since ea days). I press build and its ready (this works even if the build would fail, unless the pom is broken). I am not completely convinced that this is still a useful action, given that NB uses the local maven repo, project sources and target folder directly (I think some other IDEs have to prime since they have their own internal build representation, but this isn't the case here). |
You probably didn't -- since the process happens automatically, if you open a Maven project with The second thing is that headless LSP usage of NB has some initialization paths not well reachable through NetBeans UI. Our colleagues also use 'throwaway' VMs to run the NBLS servers, so some of the scenarios come from using a pristine machine without anyting in .m2 repo -- from the NB IDE point of view, they're testing for our first-time users/adopters. You would be surprised how many things go different if you just run BTW - in some of the later PRs I hope I will be able to avoid running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Sorry, where have we got automatic priming happening without user interaction? That's potentially an issue. As is treating Maven build scripts as trusted by default. |
@sdedic thanks for explaining, some comments:
I saw a few situations where NB asked me to prime something, I am pretty sure there is a notification bubble for that, and priming happens also at some point in the "resolve problems dialog", we had even an issue filed where a user used the "reload pom" action do download dependencies (!) (#6223), which I assume is running the priming code. Why does NB not simply ask the user to build the project using the regular maven toolchain outside of the embedder? This could cause additional problem in future, e.g: #6190 I think mvn could also run
thats good since installing deps should not happen without user interaction IMO (if the LSP server has special requirements - thats ok to me, esp headless mode is a completely different situation) |
Yes.
Now it does, it actually runs
We need (1) for parsing, code analysis, location services to work. And (2) prolongs the time, mixes project dev errors (i.e. broken code) with project setup errors (i.e. nonexistent artifact) or environment setup errors (bad network setup, ...) Majority of NB operations actually use information extracted from the Embedder, so we need the Embedder to work/be happy in the first place ... and #6190 shows our Embedder is just not configured well enough, at last for some operations. An interesting idea would be to inject a plugin that would report all project info into the buildchain; similar to Gradle support ... but that would prevent NB to resolve queries using Maven libraries at all. If we still want to use maven libs, we IMHO need to fix the Embedder operations.
I was experimenting with that, but I was interrupted by higher-priority tasks. Need to look this through again to rule out possible bad analysis. If I remember right,
That Anyway, we need to put the artifacts to a place where our internal Embedder can find them ... and ideally to a place where the external buildchain can find them to avoid duplicate downloads. |
But what's the default in the VSCode case? There shouldn't be automatic priming anywhere without the user opting in to that. |
In VSCode, whenever a new project is opened in workspace, user is asked to decide whether code in the project folder can be executed by VS Code and extensions without further explicit approval - see https://code.visualstudio.com/docs/editor/workspace-trust |
OK, thanks @dbalek - that's good to know they're covering this here for us. Just concerned about ensuring we don't get automatic code execution anywhere with eg. |
java/maven/src/org/netbeans/modules/maven/NbMavenProjectImpl.java
Outdated
Show resolved
Hide resolved
java/maven/src/org/netbeans/modules/maven/NbMavenProjectImpl.java
Outdated
Show resolved
Hide resolved
84f0b2e
to
241f303
Compare
Locally squashed. |
many tests are always on, esp those who don't use up many resources. E.g the PHP label toggles only the most expensive step in the php job. If you want me to toggle more steps/jobs with labels - let me know. (the enterprise job used to be small initially and grew over time) |
OK... some explanation before I clean up the code again: I would appreciate to run these tests (since MN4 has quite weird project structure with all the BOMs etc) - and in addition other Oracle work focuses on Micronaut ... and it's a reasonably complex application library to use as test data. So I'd like to run build tools on both JDK11 and JDK17 as the behaviour of Gradle/Maven may differ on different JDKs. |
7c54073
to
71b6dd4
Compare
this is fine but will increase the whack-a-mole factor since the job is one of the more unreliable ones. It already has retry wrappers for mx project etc. once gradle supports JDK 21 this should be moved from 17 to 21, since if it works on 21 it will on 17 too with high probability |
Especially review commit 71b6dd4; I've realized that althgough I've defined |
I don't know either, but if it works it is probably the right one :), I am wondering though why it worked for the other modules, since this isn't the first one which sets the flags. Should this be set too |
71b6dd4
to
7166cc6
Compare
added a default property value. |
7166cc6
to
9f51a38
Compare
... and finally rebased on |
The PR is still based on master though. Please update the default branch if you want this in delivery.
Well, they'll only be one more release before 17 becomes our baseline! |
@neilcsmith-net done. All tests seem to pass. Please include in NB20. |
Do you want to clean it up here, or should I include it into next commit to maven in |
Don't mind! Possibly better here to fix for the release. I did a quick search in the action output, but I guess it doesn't output that with the use of |
well, I am still worried to merge a non-trivial change like this so late (again) since it can have unexpected side effects. But since this has been tested #6514 (comment) it should hopefully cause no trouble. I wished this could have been implemented in a simpler way, without hooking into the task event system if possible, but I had unfortunately no time to provide a full alternative implementation (beside this quick experiment #6514 (comment)). We should try to clean up / refactor something every time we add complexity to the project otherwise this is going to get interesting. CI has already fairly low chances to complete without manual restarts, and this is after retry scripts etc. lets get this in |
OK, thanks @mbien Merging in time for 20-rc2. Let's ensure that is well tested in this area. It is always possible to revert for 20-rc3 in case of regressions. |
project properties do not open #6622 |
There was a race condition with opening projects and subprojects with an empty
.m2/repository
. The issue was that when the main project was being primed, a file-based watcher recognized appearance of a dependency .jar and scheduled a reload of the project before the priming was completed.Consider a multi-module Micronaut-based project:
Initially, when a subproject was opened, reading of the project failed as the immediate parent (root project) was locally present, but super-parent (micronaut-parent) was not resulting in properties not defined - the project read broke and was marked as a fallback, the same as the parent project. Good!
Because of the file-based trigger (micronaut-parent POM appears), the parent reloaded while other dependencies were still being downloaded. Some
jar
files already downloaded - and thelib
project was able to resolve its dependencies tojar
files. However Maven internally asks forpom
-classified artifacts for the dependencies to compute (and download) a recursive closure. POMs were not available locally and were faked byNbArtifactFixer
. However the information that Maven model resolver got incomplete dependency info was not recorded anywhere - thoseArtifact
objects are used internally by Maven and do not appear in theProjectBuildingResult
.So the basic fix has two parts:
incomplete
. After parent loads, allincomplete
(not just fallbacks) projects will be reloaded, just in case, as parent's build should build all included modules.References to (internal)
MavenProjectCache
replaced byNbMavenProject
when possible. Detailed logging added. UsedgetPartialProject
to potentially unwrap at least some project structure after a failed load.I've added basic unit tests to ensure that if a subproject primes, it becomes valid. And if parent primes, the subproject also becomes valid.