-
Notifications
You must be signed in to change notification settings - Fork 117
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
Configure Maven JRE separately #1137
Configure Maven JRE separately #1137
Conversation
5e37fdf
to
23eee3e
Compare
@mickaelistria @laeubi @HannesWell Can you quickly comment, whether that extension makes sense from your PoV. Then I would adjust the test cases accordingly, although this probably requires some more refactoring as the tested Eclipse instance only has one installed JRE.... |
@kwin can you enhance the PR description a bit more with:
|
Can you explain what a "Maven JRE" is? The one to run maven commandline build in contrast to the "JRE System Library"? If yes, maybe this should have two levels:
And we should find a better name, maybe "Maven Runtime JRE" or "Maven Build JRE" .... but in general I think this would be valid, e.g. Tycho requires Java 17 to run, but can compile code for Java 8 ... |
Yes, exactly. Maybe we should relabel as "Maven Execution JRE"
There is already two levels: I don't think we need a third level here TBH. Also not sure about the minimum enforced by Maven itself, this is currently not enforced at all, but we could improve later on to also consider this. |
Do you think it would be possible to show the one currently active on the left like it is done for Java Projects?
Alright then start with this one. |
Where? Please share some screenshot because I don't know what you are referring to. |
I don't think the Maven Execution JRE should be listed in the project explorer as it has zero impact on the (compile or test) classpath! |
If it has no impact why do we configure it fro the project? I don't think this is reserved to "classpath" items and as it is a persistent property of the project it seems important enough. |
I didn't say that!
For me it is, it makes no sense to list anything else being configured on the project properties there. |
I take the silence here as consent, i.e. will work on adjusting test cases, labels and an entry for the release notes |
Yes please go on... |
3214f9f
to
25f6d57
Compare
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.
In general this change looks good and I agree that it is the right direction to let the enforcer plugin only influence the JRE to run a Maven-build process and the adjust the Maven launch config to use that EE by default, but not to consider the enforcer-p when setting the JRE_CONTAINER to build.
But I'm not so happy to handle all of that in m2e.core and therefore introducing a dependency to jdt for m2e.core.
Wouldn't it be possible to handle the selection of the JRE to launch completely in the m2e.launching plugin? Letting m2e.core.ui depend on m2e.launching is probably less problematic.
To test it, wouldn't it be sufficient to check the selected EE and to test the VMInstall algorithm in a unit test.
RELEASE_NOTES.md
Outdated
|
||
![Maven Project Properties](https://user-images.githubusercontent.com/185025/208966384-816570f8-8c02-4578-bd67-588f7d8e2c27.png) | ||
|
||
The default value is automatically set according to the configuration of the `maven-enforcer-plugin` rule [`requireJavaVersion`](https://maven.apache.org/enforcer/enforcer-rules/requireJavaVersion.html) when creating or updating the Maven configuration. This value is no longer considered for configuring the project's build JRE. |
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.
It should also be noted that the maven-compiler source/target-release settings are considered as well, but may be narrowed-down/pulled up by the enforcer-plugin.
Furthermore I would state first that the projects (build/classpath) JRE_CONTAINER no longer considers the maven-enforcer-plugin but that it is considered as default for the new option of the Maven-Runtime JRE.
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.
Technically the Maven Execution JRE falls back to project build JRE or workspace JRE when no enforcer rule is found!
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.
I rephrased this now, please check.
final IJavaProject javaProject; | ||
javaProject = JavaCore.create(project); |
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.
final IJavaProject javaProject; | |
javaProject = JavaCore.create(project); | |
IJavaProject javaProject = JavaCore.create(project); |
@@ -54,10 +72,28 @@ public class MavenProjectPreferencePage extends PropertyPage { | |||
|
|||
// private Button includeModulesButton; | |||
|
|||
//https://git.eclipse.org/c/jdt/eclipse.jdt.debug.git/tree/org.eclipse.jdt.debug.ui/plugin.xml#n2467 |
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.
You should include a specific commitId in the link. Otherwise the the line-number might become wrong one day.
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.
code removed
private static final Map<String, IVMInstall> INSTALLED_JRES_PER_ENVIRONMENT_ID = Map.of("JavaSE-1.8", | ||
new TestJREManager.JRE("1.8"), "JavaSE-11", new TestJREManager.JRE("11.0.4"), "JavaSE-13", | ||
new TestJREManager.JRE("13.0.3")); |
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.
private static final Map<String, IVMInstall> INSTALLED_JRES_PER_ENVIRONMENT_ID = Map.of("JavaSE-1.8", | |
new TestJREManager.JRE("1.8"), "JavaSE-11", new TestJREManager.JRE("11.0.4"), "JavaSE-13", | |
new TestJREManager.JRE("13.0.3")); | |
private static final Map<String, IVMInstall> INSTALLED_JRES_PER_ENVIRONMENT_ID = Map.of( // | |
"JavaSE-1.8", new JRE("1.8"), // | |
"JavaSE-11", new JRE("11.0.4"), // | |
"JavaSE-13", new JRE("13.0.3")); |
IClasspathEntry entry = null; | ||
for (int i = 0; i < classpath.length; i++) { | ||
entry = classpath[i]; | ||
switch (entry.getEntryKind()) { | ||
case IClasspathEntry.CPE_VARIABLE: | ||
throw new IllegalStateException("Classpath entries of type CPE_VARIABLE not supported"); | ||
case IClasspathEntry.CPE_CONTAINER: | ||
return resolveJRE(entry.getPath()); | ||
} | ||
} |
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.
IClasspathEntry entry = null; | |
for (int i = 0; i < classpath.length; i++) { | |
entry = classpath[i]; | |
switch (entry.getEntryKind()) { | |
case IClasspathEntry.CPE_VARIABLE: | |
throw new IllegalStateException("Classpath entries of type CPE_VARIABLE not supported"); | |
case IClasspathEntry.CPE_CONTAINER: | |
return resolveJRE(entry.getPath()); | |
} | |
} | |
for (IClasspathEntry entry : classpath) { | |
switch (entry.getEntryKind()) { | |
case IClasspathEntry.CPE_VARIABLE: | |
throw new IllegalStateException("Classpath entries of type CPE_VARIABLE not supported"); | |
case IClasspathEntry.CPE_CONTAINER: | |
return resolveJRE(entry.getPath()); | |
} | |
} |
defaultJRERadioButton = new Button(comp, SWT.RADIO); | ||
defaultJRERadioButton.setText(getDefaultJreName()); | ||
defaultJRERadioButton.setLayoutData(new GridData(SWT.FILL, SWT.BEGINNING, false, false, 3, 1)); | ||
defaultJRERadioButton.addSelectionListener(new SelectionAdapter() { |
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.
You can use SelectionListener.widgetSelectedAdapter()
here and below to use a lambda instead of anonymous inner class.
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.
code removed
|
||
Button installedJREsButton = new Button(comp, SWT.PUSH); | ||
installedJREsButton.setText(Messages.MavenProjectPreferencePage_btnInstalledJREs); | ||
installedJREsButton.addListener(SWT.Selection, new Listener() { |
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.
Couldn't you use addSelectionListener()
here?
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.
code removed
// sort by name | ||
Collections.sort(jres, new Comparator<Object>() { | ||
@Override | ||
public int compare(Object o1, Object o2) { | ||
IVMInstall left = (IVMInstall) o1; | ||
IVMInstall right = (IVMInstall) o2; | ||
return left.getName().compareToIgnoreCase(right.getName()); | ||
} | ||
|
||
@Override | ||
public boolean equals(Object obj) { | ||
return obj == this; | ||
} | ||
}); |
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.
// sort by name | |
Collections.sort(jres, new Comparator<Object>() { | |
@Override | |
public int compare(Object o1, Object o2) { | |
IVMInstall left = (IVMInstall) o1; | |
IVMInstall right = (IVMInstall) o2; | |
return left.getName().compareToIgnoreCase(right.getName()); | |
} | |
@Override | |
public boolean equals(Object obj) { | |
return obj == this; | |
} | |
}); | |
jres.sort((j1, j2) -> j1.getName().compareToIgnoreCase(j2.getName())); |
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.
Comparator.comparing(IVMInstall::getName, String.CASE_INSENSITIVE_ORDER)
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.
code removed
public void setUp() throws Exception { | ||
super.setUp(); |
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.
public void setUp() throws Exception { | |
super.setUp(); | |
public void setUpJREManager() throws Exception { |
If you don't override the super-class method you don't have to call super.
Having multiple before-methods is not a problem.
JDT Launching is necessary when updating projects to calculate the default Maven Execution JRE (and that happens in core)
The Maven Execution JRE is not based on EEs but on concrete JREs. |
We already have had the demand to inspect JRE here where it also was quite complex to not do it in core directly: I therefore thing it would be good to have some kind of |
Then we can not use Interfaces like IVMInstall in that service interface but must define our own one… |
because this is "owned" by JDT and has much more requirements than we need here. So we should decouple m2e.core from jdt. |
Beside that I think we can implement most of required stuff in the interface itself, e.g. we can have a method:
|
But why does this have to happen in core? The Maven Execution JRE is eventually just used to select a JRE to run a Maven build?
Why is it based on JREs/VMInstalls and not EEs anymore? Previously in Furthermore I want to challenge the need for a separate preference. All the preference is doing is saving the default Maven Execution JRE of a project, isn't it? Therefore my suggestion is to drop the extra preference and just use compiler+enforcer as default. If it later turns out that there is a great demand for such preference we can still add it. :) |
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.
I could not yet look into the details but from a quick overview my main points I'd like to discuss are the new API and where the enforcer-plugin config is read. Please see the comments below.
* | ||
* @return a version spec compliant with {@link VersionRange} spec | ||
*/ | ||
String getRequiredJavaVersion(); |
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.
Do you have a need for that as part of M2E's public API?
Actually I would consider this as internal.
In case there is a real need for that, I would rename it to getExecutionJavaVersion()
or similar.
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.
Please don't use the IProjectConfig
for this! This class is used to group project that have the same resolver context (e.g. selected properties and profiles) but not a general purpose place to store project specific settings, beside that these are workspace specific and cannot be shared.
This should be stored in the preferences (e.g. org.eclipse.m2e.core.prefs
or even org.eclipse.m2e.launching.prefs
).
/** | ||
* Configure Java version required for Maven execution based on maven-enforcer-plugin configuration | ||
*/ | ||
private void configurationRequiredJavaVersion(IMavenProjectFacade facade, IProgressMonitor monitor) { |
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.
I still would prefer to move that to the MavenLaunchDelegate
.
This would save the logic to persist that value and unless there is a strong need to make that available in the API (see my other comment), I don't think this is necessary here. Reading the value is not very costly.
To quote from @laeubi s comment from above
You guys really giving me a hard time. I did exactly that in my last update. |
* | ||
* @return a version spec compliant with {@link VersionRange} spec | ||
*/ | ||
String getRequiredJavaVersion(); |
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.
Please don't use the IProjectConfig
for this! This class is used to group project that have the same resolver context (e.g. selected properties and profiles) but not a general purpose place to store project specific settings, beside that these are workspace specific and cannot be shared.
This should be stored in the preferences (e.g. org.eclipse.m2e.core.prefs
or even org.eclipse.m2e.launching.prefs
).
I think we probably should split this into three parts (and PRs):
That way it would be possible to be more focused and we can probbaly already merge some parts of this than trying to get this done in one big chunck. |
Sorry, not willing to invest that much time here also for me these all belong to the same feature.
As long as you guys have not aligned on this, that won't make any sense. @HannesWell is against persisting it. |
It seems that the feature is too big to be solved in one change, so a good opportunity is often to split things into smaller pieces, for example all these discussion about the
As said, I think the issue is just to broad and thus things are getting mixed up, the concern was just to compute the enforced java version as part of the (default) project configuration. That is different from the topic of supporting different runtime JVM and that must be persisted somewhere and enforcer might not be the only source (e.g. I really liked the idea of having a user configuration UI for that). |
Sorry for that, but I can tell you that it is also not fun, if you know that you do that to somebody. To make your time not even harder, I added the changes how I would do it to another branch:
Actually with the launch configs we already have a place where to persist a specific runtime JVM if the default is not sufficient. With this change (in the way it should IMHO be done) 'just' the default becomes smarter. |
2421cd0
to
e92b0d3
Compare
3f9fd7e
to
257a53e
Compare
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.
@kwin and @laeubi
I have now worked out a solution based on what I have described in previous comments, which I believe satisfies all needs. It only considers the enforcer-plugin to select the JRE to run a launched maven build and it does that, without storing anything the projects resolver config, without any new API.
Additionally this approach can read the enforcer config, if if the base-dir/pom-dir points to a project, which is not in the workspace. That is IMHO a quite common scenario, e.g. if one doesn't import the root of a git repo, which often also is the root for a maven build.
Please have a look at it and let me know what you think.
If you are fine with it, it should be submitted ASAP, so that I can release M2E this evening, just in time for 2023-03 RC1.
257a53e
to
8e65063
Compare
I must confess I don't understand this part, a configuration should be inherited by its child so it doesn't matter if the parent is in the workspace or not? |
IMavenToolbox toolbox = IMavenToolbox.of(ctx); | ||
return toolbox.calculateExecutionPlan(tasks, setup); | ||
}, monitor); | ||
} catch(CoreException e) { | ||
manager.getMarkerManager().addErrorMarkers(pom, IMavenConstants.MARKER_POM_LOADING_ID, e); | ||
exceptionHandler.accept(e); |
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 part looks suspicious (also in the original) why not just throw the exception and let it handle at the caller site?
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 method is called by IMavenProjectFacade.calculateExecutionPlan/setupExecutionPlan()
, which don't permit CoreExceptions in the method signature.
So without breaking the API we cannot propagate the original exception. It would only possible when wrapping it in a RuntimeException.
But I think that would not help much here. It should have been thrown before, but changing it now is not ideal.
But we should note that for a M2E 3.0 :)
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.
then wrapping it in a runtime exception (or "handle") it there, this is simply bad style and we should avoid that so pushing the handling as far as possible to the top.
@@ -628,7 +654,7 @@ private MavenProject tryGetMavenProject() { | |||
return manager.getMavenProject(this, null); | |||
} catch(CoreException ex) { | |||
//TODO can we handle this more graceful? E.g. one error condition is that project dependencies can not be resolved, maybe we can try to load the project without dependencies then? | |||
manager.getMarkerManager().addErrorMarkers(pom, IMavenConstants.MARKER_POM_LOADING_ID, ex); | |||
logException(ex); |
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.
same here, better throw the exception to the caller instead and handel it there
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.
Same as above. Called by IMavenProjectFacade.createExecutionContext()
.
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.
One way would be to return an "ExceptionalExecutionContext" that throws the original exception on any call of its methods (that allow CoreException already), this is also done with the Plexuscontainer as well.
org.eclipse.m2e.core/src/org/eclipse/m2e/core/internal/project/registry/MavenProjectFacade.java
Outdated
Show resolved
Hide resolved
That's right. But which project in the workspace should I check if I just have to path the root of a git repo? Iterate over all projects in the WS and check if its within the given path? But then the config could be overridden in a child-project. Maybe different in different childs. Then it's the question which should I take? Considering all those corner cases, it is simpler to just load the pom from the given path and read its enforcer config. It is not the nicest approach, but I think it works in the most robust way. |
8e65063
to
30c5949
Compare
What do you mean by "root of the git repo"? Actually can only execute a |
30c5949
to
b48f19f
Compare
As we have discussed in private chat, I have now removed the part to load the enforcer-config from a maven-project that is not in the workspace and use only the existing methods on the |
b48f19f
to
452f993
Compare
The value is automatically derived from maven-enforcer rule requireJava Closes eclipse-m2e#1134 Closes eclipse-m2e#1099 Closes eclipse-m2e#1120 Co-authored-by: Hannes Wellmann <wellmann.hannes1@gmx.net>
452f993
to
1240321
Compare
The Jenkins build is green and the GH-workflows fail due to Tycho's current incompatibility with Maven 3.9 (looks like not all runners use Maven 3.9, some use 3.8.7). So this is ready for submission. Thank you @kwin for starting this PR. Btw. since this was also an issue discussed in this PR: |
Finally 👍 |
The value is automatically derived from maven-enforcer rule requireJava
This closes #1134
This closes #1120