One way sync for jpms args from maven compiler args to .classpath file #216

treilhes · 2021-06-05T08:19:58Z

This commit :

get the maven-compiler-plugin compiler arguments if any
use regex to parse them and extract jpms arguments (--add-exports,--add-opens,--add-reads,--patch-module)
dispatch the arguments in the right container (if the target module is part of JRE then in JreContainer else in M2eContainer)
transform them into classpath attribute (add-exports, add-opens, add-reads, patch-module)

1 junit test

fbricon

Nice contribution! I've only skimmed it from the browser, so mostly bikeshed complaints for now, I'll get into more details later.
There's probably a couple utility methods that already exist that could be used in your test. But I'll look into it.

fbricon · 2021-06-05T09:05:10Z

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/BuildPathManager.java

@@ -173,6 +173,21 @@ public static IClasspathEntry getMavenContainerEntry(IJavaProject javaProject) {
    return null;
  }

+  public static IClasspathEntry getJREContainerEntry(IJavaProject javaProject) {


move to MavenClasspathHelpers

ok. What about getMavenContainerEntry, isn't it a bit similar ?

org.eclipse.m2e.jdt.tests/src/org/eclipse/m2e/jdt/tests/JpmsConfigurationTest.java

fbricon · 2021-06-05T09:16:42Z

org.eclipse.m2e.jdt.tests/src/org/eclipse/m2e/jdt/tests/JpmsConfigurationTest.java

+    assertTrue(m2eAttributes.get(ADD_READS_ATTR).equals(M2E_ADD_READS_VALUE1));
+    assertTrue(m2eAttributes.get(PATCH_MODULE_ATTR).equals(M2E_PATCH_MODULE_VALUE1));
+
+    String argsSet2Content = contents.replace(REPLACED_POM_STRING, argsSet2);


would be better to cover compilerArgument works as intended on the second test

I do not handle compilerArgument anymore in the getCompilerAgument method. As the maven-compiler-plugin doc states, all the jpms arguments are in fact two arguments so no complete jpms argument can be provided using compilerArgument.

And i think it is not worth supporting the following cases:

<compilerArgument>--add-exports</compilerArgument> <compilerArgs> <arg>somemodule/somepackage=someothermodule</arg> </compilerArgs> <compilerArgs> <arg>--add-exports</arg> </compilerArgs> <compilerArgument>somemodule/somepackage=someothermodule</compilerArgument>

Do you agree ?

org.eclipse.m2e.jdt.tests/projects/compilerJpmsSettings/argsSet2.xml

org.eclipse.m2e.jdt.tests/src/org/eclipse/m2e/jdt/tests/JpmsConfigurationTest.java

fbricon

Once jpms flags are removed from pom.xml, they should be removed from the eclipse classpath as well

org.eclipse.m2e.jdt/src/org/eclipse/m2e/jdt/internal/ModuleSupport.java

org.eclipse.m2e.jdt.tests/src/org/eclipse/m2e/jdt/tests/JpmsConfigurationTest.java

Also: sources updated according review comments

treilhes · 2021-06-07T12:35:23Z

Hi @fbricon,

I've checked the continuous integration logs and i'm wondering how my code could be the cause of the failures.
Do you think my code can be responsible for this kind of failure?

testDownloadSources_006_nonMavenProject(org.eclipse.m2e.tests.BuildPathManagerTest)  Time elapsed: 2.649 s  <<< ERROR!
org.eclipse.core.runtime.CoreException: Reindexing error
	at org.eclipse.m2e.tests.BuildPathManagerTest.testDownloadSources_006_nonMavenProject(BuildPathManagerTest.java:656)
Caused by: java.nio.channels.OverlappingFileLockException
	at org.eclipse.m2e.tests.BuildPathManagerTest.testDownloadSources_006_nonMavenProject(BuildPathManagerTest.java:656)

The last build failure seems even more unrelated to my code :

Transfer failed for https://repo.eclipse.org/content/repositories/cbi-releases/org/apache/maven/doxia/doxia-module-apt/1.0/doxia-module-apt-1.0.pom 503 Service Unavailable -> [Help 2]

Rgds,
Live long and prosper

fbricon · 2021-06-07T13:07:14Z

testDownloadSources_006_nonMavenProject has been failing for quite some time, it's unrelated. That test (or the regression it seems to indicate) needs to be fixed, but that's another battle.

Eclipse's nexus instance might be unstable and cause the other issue.

treilhes · 2021-06-07T17:14:21Z

As a side note, it will solve:

#129 Bug 543631 - Eclipse - Maven - JPMS (m2e ignores compilerArgs in pom.xml)
#136 Bug 535650 - Update Project removes add-export declarations in Java 9 projects

eclipse-m2e/m2e-core#216

treilhes · 2021-06-15T09:22:56Z

Hi @fbricon,
What do you think about my comments on compilerArgument and testCompilerArguments support ?

fbricon · 2021-06-18T13:36:57Z

Hi @fbricon,
What do you think about my comments on compilerArgument and testCompilerArguments support ?

Sorry for the late reply. I need to take a closer look. I'll do a deeper review next week.

YaYsimulator · 2021-08-19T10:03:16Z

Hi @fbricon
What is the status of this pull request? The project I'm currently working on for my colleagues would benefit greatly from this fix. :)

leerho · 2021-08-19T17:55:50Z

I am also very interested in this fix. Is there any hope of getting this PR accepted?

gayanper · 2021-09-04T10:12:10Z

I did test this change by merging in to latest master branch and doing a local build. I have been running it for one week now with my daily driver eclipse setup. I have not found any issues related to this change.

So @treilhes can we resolve the conflicts and make it ready to merge and @fbricon can we merge this so that we can have this support in the next release for Eclipse 4.22 ?

treilhes · 2021-09-04T14:07:51Z

Hello @gayanper,
I did try to fix it but the issue is related to some maven index lock and is completely unrelated to this pull request.
All the pull requests seem to suffer from this test failure and solving it requires someone knowing the inner working of m2e not a casual user like me.

@fbricon, are you still around? I hope the covid did not get you and you are well .....

Can someone give us some feedback ?

mickaelistria · 2021-09-05T12:09:29Z

Rebased and merged as f23d3f8 ; thanks!
Would you please add a note about it in the RELEASE_NOTES document?

treilhes · 2021-09-05T19:54:14Z

Hello @mickaelistria,
Don't know if it was the right way to do it but you can find the updated release note for this pr in #322

leerho · 2022-01-08T02:06:50Z

I thought this was going to be fixed in 4.22, but the problem is still there. In fact, it not only impacts compiler JPMS args specified with the compiler plugin, it also impacts JPMS args specified in the surefire plugin.

This bug severely impacts smooth migration from Java 8 to Java 9+.

mickaelistria · 2022-01-09T13:13:42Z

@leerho: can you please report a new issue with details to easily reproduce against latest m2e release please?

leerho · 2022-01-13T05:31:19Z

Will do. I figured it was already reported. It will take me some time to prepare the "easily reproduce" part :)

treilhes added 2 commits June 5, 2021 09:40

One way sync for jpms args from maven compiler args to .classpath file

3b34c7c

Bump jdt version to 1.18.1

4cd1da0

fbricon requested changes Jun 5, 2021

View reviewed changes

Bump m2e.feature version to 1.18.1

270a196

Also: sources updated according review comments

treilhes requested a review from fbricon June 7, 2021 10:37

Removed testCompilerArguments cause double dashed args are unsupported

9cecd26

treilhes added a commit to treilhes/scenebuilder that referenced this pull request Jun 9, 2021

Ignore .classpath backup until m2e-core pull request merge

0ffced7

eclipse-m2e/m2e-core#216

treilhes added 2 commits July 5, 2021 16:13

Merge branch 'master' into master

ec00f3f

Adapt to new m2e-core head

5444ca5

Merge branch 'master' into master

cf4f62a

mickaelistria closed this Sep 5, 2021

mickaelistria added this to the 1.18.2 milestone Sep 5, 2021

treilhes added a commit to treilhes/m2e-core that referenced this pull request Sep 5, 2021

Updated release note for pull request eclipse-m2e#216

51cf7de

treilhes added a commit to treilhes/m2e-core that referenced this pull request Sep 5, 2021

Updated release note for pull request eclipse-m2e#216

26c691b

treilhes mentioned this pull request Sep 5, 2021

Updated release note for pull request #216 #322

Merged

mickaelistria pushed a commit that referenced this pull request Sep 5, 2021

Updated release note for pull request #216

531dac9

CsCherrYY mentioned this pull request Nov 3, 2022

Support sync compilerArgs with delimiter #1034

Merged

CsCherrYY mentioned this pull request Nov 30, 2022

compilerArgs from JavaCompile should be recognized in eclipse project gradle/gradle#22830

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

One way sync for jpms args from maven compiler args to .classpath file #216

One way sync for jpms args from maven compiler args to .classpath file #216

treilhes commented Jun 5, 2021

fbricon left a comment

fbricon Jun 5, 2021

treilhes Jun 5, 2021

fbricon Jun 5, 2021

treilhes Jun 7, 2021

fbricon left a comment

treilhes commented Jun 7, 2021

fbricon commented Jun 7, 2021

treilhes commented Jun 7, 2021 •

edited

Loading

treilhes commented Jun 15, 2021

fbricon commented Jun 18, 2021

YaYsimulator commented Aug 19, 2021 •

edited

Loading

leerho commented Aug 19, 2021

gayanper commented Sep 4, 2021

treilhes commented Sep 4, 2021

mickaelistria commented Sep 5, 2021

treilhes commented Sep 5, 2021 •

edited

Loading

leerho commented Jan 8, 2022 •

edited

Loading

mickaelistria commented Jan 9, 2022

leerho commented Jan 13, 2022

One way sync for jpms args from maven compiler args to .classpath file #216

One way sync for jpms args from maven compiler args to .classpath file #216

Conversation

treilhes commented Jun 5, 2021

fbricon left a comment

Choose a reason for hiding this comment

fbricon Jun 5, 2021

Choose a reason for hiding this comment

treilhes Jun 5, 2021

Choose a reason for hiding this comment

fbricon Jun 5, 2021

Choose a reason for hiding this comment

treilhes Jun 7, 2021

Choose a reason for hiding this comment

fbricon left a comment

Choose a reason for hiding this comment

treilhes commented Jun 7, 2021

fbricon commented Jun 7, 2021

treilhes commented Jun 7, 2021 • edited Loading

treilhes commented Jun 15, 2021

fbricon commented Jun 18, 2021

YaYsimulator commented Aug 19, 2021 • edited Loading

leerho commented Aug 19, 2021

gayanper commented Sep 4, 2021

treilhes commented Sep 4, 2021

mickaelistria commented Sep 5, 2021

treilhes commented Sep 5, 2021 • edited Loading

leerho commented Jan 8, 2022 • edited Loading

mickaelistria commented Jan 9, 2022

leerho commented Jan 13, 2022

treilhes commented Jun 7, 2021 •

edited

Loading

YaYsimulator commented Aug 19, 2021 •

edited

Loading

treilhes commented Sep 5, 2021 •

edited

Loading

leerho commented Jan 8, 2022 •

edited

Loading