-
Notifications
You must be signed in to change notification settings - Fork 194
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
Reproducer for 'tycho-compiler-jdt unable to compile test code when module-info.java is present' / Issue 230 #809
base: main
Are you sure you want to change the base?
Conversation
@jason-faust this change seems to include a lot of unrelated changes, can you rebase it and only submit relevant changes? |
@laeubi Which branch do you want it to target? 2.7 or master? |
master please, if suitable we can backport it later on. |
75b5cdb
to
e112aa2
Compare
Hopefully this works. |
Is this the error you are expecting? |
That is the error, but a little lower down in the output. There may also be some follow on issues with the compiler plugin passing along the source and target options when it should only be passing the release option, but that is unrelated to the current issue.
|
Can you try to force target/source to 11 just to see if this changes anything? |
Testing locally has the same error when using source, test, and release at 11. |
@mickaelistria @akurtakov any one has an idea whats wrong here? Is this an EJC issue? |
Indeed, it looks like the tycho-compiler-jdt doesn't support the |
So any jdt / maven compiler expert who can help here? |
For the record, here is the full commandline passed to jdt and the error message:
|
@iloveeclipse can you tell if tycho is doing anything wrong here or if this is a user error? |
I'm not a module expert. Spec is here: https://openjdk.org/jeps/261 If you would attach source code in a project with configuration as it is expected to work plus the actual command line that triggers ECJ, it would simify debugging. |
The code is here: https://github.com/eclipse-tycho/tycho/tree/8d696d614a95000438b379bdeb41f2e0d9f322e7/tycho-its/projects/compiler.mavenCompilerPlugin.moduleWithTests If I understand @jason-faust correctly |
I really meant exact that above. Not some random files somewhere, but as a proect with compiler settings. |
I'm not sure if one can transform this into an eclipse project, this uses maven to replace the I think one can emulate this by
|
By the way javac says:
If I remove those,
So I maybe messed up the call a bit but maybe it still helps as |
@iloveeclipse is the provided example suitable? Is this a bug I should report to JDT? |
Would you please fix the conflict with master? |
By the way, the documentation of maven-compiler-plugin and other discussions recommends <plugin>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<compilerId>eclipse</compilerId>
</configuration>
<dependencies>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-compiler-eclipse</artifactId>
<version>...</version>
</dependency>
<dependency>
<groupId>org.eclipse.jdt</groupId>
<artifactId>ecj</artifactId>
<version>...</version>
</dependency>
</dependencies>
</plugin> So if this works, then I don't think Tycho should support the use-case you describe here and the |
Probably its the other way round, if |
See https://bugs.eclipse.org/bugs/show_bug.cgi?id=526110#c3 The compiler has support for the option, just the batch compiler doesn't parse it (and the above got forgotten due to little demand ...) |
@stephan-herrmann thanks for the insight, so should I open a bug at JDT or is there already an issue that tracks it? |
No issue in github yet, so please create one, and link also to the old bugzilla, thanks, which can then be closed as MOVED (with another link). |
Depends on
|
@jason-faust can you try out if using javac instead of ecj fixes the problem for you: |
@laeubi That won't fix my problem as I want to be able to use the Eclipse compiler. I ultimately want to be able to generate the same compiler warnings and errors that the Eclipse IDE will generate, so to use the same compiler, not to use a different compiler. |
You can at least validate if it actually can work :-) |
@laeubi The issue is about the JDT plugin to the maven compiler plugin, not the tycho compiler. |
So does it compile if you use javac maven compiler? |
2ba1368
to
8a28264
Compare
Updated to fix the merge conflict. And if you remove the call out to the |
It would be interesting to know the exact options which maven passes into javac. |
From the test output (excerpt)
|
Thanks. Just to be sure: these are the arguments that work with In those args source, target and release don't seem to match (1.7 != 17), but other than that I don't see anything magic, so fixing |
@srikanth-sankaran can you maybe help her as you recently worked on the compiler a lately, this seems to be the relevant JDT issue if I correctly understand: |
As it is now fixed (thanks to @stephan-herrmann ) hopefully this update will make it finally work: |
8a28264
to
af12e0d
Compare
For some reasons it assumes a wrong target level now
Strange enough the pom says
|
af12e0d
to
bdfc70e
Compare
@jason-faust if I specify
compile proceeds (this seems to be due to the default values passed by maven compiler plugin, not sure if one should ignore But then the compilation fails with
so it seems the inital problem is solved, do you want to adjust the testcase and fix the compile error(s) so we can merge it? |
@laeubi The test case is still correct as far as I can tell. If you comment out the Tycho is not passing down a Stock maven compiler plugin command line options sent (paths shortened for readability):
Tycho compiler plugin command line options sent (paths shortened for readability):
The Tycho plugin not handling |
bdfc70e
to
cde1d67
Compare
I have never considered tycho-compiler-jdt as a compiler backend for maven-compiler-plugin nor ever tried it. Have you tried https://mvnrepository.com/artifact/org.codehaus.plexus/plexus-compiler-eclipse if you rely on maven-compiler-plugin? |
cde1d67
to
00ed182
Compare
Have used the plexus compiler before but that one makes use of the |
I'm unsubscribing for now |
Demonstration of the problem reported in issue #230