Skip to content
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

Actually fix lemminx compilation #729

Conversation

datho7561
Copy link

Need to actually generate the .class files when building, since the src/main/test compilation needs the .class from src/main/java being present.

@datho7561
Copy link
Author

@testforstephen if you have some time, I would appreciate if you could review this change. I don't fully understand why the .class files were being generated in a temporary folder instead of the expected folder. When I debugged the JavaBuilder running with ECJ, the .class files for the source files were generated and placed in target by the BatchImageBuilder.

@datho7561
Copy link
Author

datho7561 commented Aug 20, 2024

I ran the quick fix test suite from jdt.ui on this PR and this PR causes a bunch of errors. I'll look into fixing those. seems to be a fluke, the test suite is flaky

@datho7561 datho7561 force-pushed the dom-with-javac-actaully-fix-lemminx-compilation branch from 25bab9d to 8b60d5f Compare August 21, 2024 19:01
datho7561 added a commit to datho7561/eclipse.jdt.core that referenced this pull request Aug 21, 2024
- progress is currently hampered by
  eclipse-jdtls#729,
  I get a stacktrace that that PR should fix
  (classcast from within javac)

Signed-off-by: David Thompson <davthomp@redhat.com>
@testforstephen
Copy link

@testforstephen if you have some time, I would appreciate if you could review this change. I don't fully understand why the .class files were being generated in a temporary folder instead of the expected folder. When I debugged the JavaBuilder running with ECJ, the .class files for the source files were generated and placed in target by the BatchImageBuilder.

Javac generates .class files in a temporary folder because the JDT builder API expects the compiler to return class bytes rather than writing the files directly. To meet this requirement, we work around the javac implementation by generating .class files in a temporary directory first, then return the bytes of the temp class file to the ImageBuilder via the CompilationResult. Once the upstream ImageBuilder accepts the CompilationResult, we will delete the temp Javac class files. You can find the full discussion history in #535 (comment).

@testforstephen
Copy link

I tried lemminx project with Javac bits, it can generate classes and test-classes under target directory successfully. Could you clarify the specific issue you encountered that you intend to address with this PR?

@datho7561
Copy link
Author

If you clean the project, (eg. delete all the .class files under target) and rebuild, then many of the test files won't be able to compile, since compiling them relies on referencing the compiled source files, which aren't generated when the compilation of the test files starts.

Need to actually generate the `.class` files when building,
since the `src/main/test` compilation needs the `.class` from
`src/main/java` being present.

Signed-off-by: David Thompson <davthomp@redhat.com>
@datho7561 datho7561 force-pushed the dom-with-javac-actaully-fix-lemminx-compilation branch from 8b60d5f to 52feb1a Compare August 27, 2024 14:44
@testforstephen
Copy link

If you clean the project, (eg. delete all the .class files under target) and rebuild, then many of the test files won't be able to compile, since compiling them relies on referencing the compiled source files, which aren't generated when the compilation of the test files starts.

After deleting the target directory manually, I use "Force Java Compilation -> Full" command to perform a full build, it can regenerate all class files. In current implementation, we only persist class files for the requested ICompilationUnits in the compile operation. For those intermediate dependency sources, we don't persist class files for them. this is by design.

@datho7561
Copy link
Author

Okay, I checked again, and compiling lemminx now works properly.

Some of the changes in this PR were related to compiling modules properly; I'll check if they are still necessary and move them over to a new PR.

@datho7561 datho7561 closed this Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants