javac doesn't configure output folders of dependent projects correctly #1017
Conversation
snjeza
requested review from
mickaelistria,
rgrunber,
testforstephen and
datho7561
| @@ -385,7 +385,7 @@ private static Collection<File> classpathEntriesToFiles(JavaProject project, Pre | |||
| for (IClasspathEntry transitiveEntry : resolved ) { | |||
| if (transitiveEntry.getEntryKind() == IClasspathEntry.CPE_SOURCE) { | |||
| IPath outputLocation = transitiveEntry.getOutputLocation(); | |||
| if (outputLocation != null && select.test(transitiveEntry)) { | |||
| if (outputLocation != null && !transitiveEntry.isTest()) { | |||
There was a problem hiding this comment.
The select condition needs to be a parameter, it may take different values depending on the case. So instead of hardcoding the condition here, I think it should instead be tweaked in the parameter of the callers.
Concretely here, what is missing? Is it an entry in the sourcepath or in the classpath? If the former, tweaking line 268 seems better, if the former then line 303 would be a more accurate location to change,
mickaelistria
force-pushed
the
dom-with-javac
branch
2 times, most recently
from
7f5c20a to
87832d6
Compare
snjeza
force-pushed
the
issue-1016
branch
2 times, most recently
from
cd777ab to
c591429
Compare
|
Thanks. This patch indeed seems simple. |
The issue happens when a dependent project has a source folder with an output folder. You can try the project (multimodule.zip) from redhat-developer/vscode-java#3885 (comment)
How to create a test case for javac? |
|
You can try to add a test to org.eclipse.jdt.core.tests.javac/.../RegressionTests.java . |
|
Note that the test cannot depend on Gradle. Having a non-Gradle reproducer seems important. |
snjeza
changed the title
|
@mickaelistria I have updated the PR. |
snjeza
changed the title
mickaelistria
force-pushed
the
issue-1016
branch
from
8df3562 to
20b620e
Compare
Signed-off-by: Snjezana Peco <[email protected]>
mickaelistria
force-pushed
the
issue-1016
branch
from
20b620e to
35b7069
Compare
There was a problem hiding this comment.
This change indeed seems good. Thank you.
I suggest we wait for build results before merging. I won't be available until monday, so here are the things to check before a safe merge:
- the CI Jenkins build completes
- its number of failures is not superior to the one of the dom-with-javac branch for its parent commit
- The number of total tests increased to ensure the newly added tests has run
When all these criteria are verified (build should take ~40 minutes to complete, so not too far from now), then please merge this PR.
Fixes #1016