-
Notifications
You must be signed in to change notification settings - Fork 136
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
Add the option to use an alternative compiler for compiling classes in JavaBuilder #2497
Add the option to use an alternative compiler for compiling classes in JavaBuilder #2497
Conversation
@@ -5,6 +5,7 @@ Bundle-SymbolicName: org.eclipse.jdt.core.tests.builder; singleton:=true | |||
Bundle-Version: 3.12.400.qualifier | |||
Bundle-Vendor: %providerName | |||
Bundle-Localization: plugin | |||
Fragment-Host: org.eclipse.jdt.core |
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 ensures the tests shares the same classloader as jdt.core plugin, and the AbstractImageBuilder in jdt.core can create an instance of the MockCompiler in the tests.
thanks to @mickaelistria for the original implementation of this new option. |
6860972
to
1f683a9
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 principle the change looks simple enough and purely technical. Please also add a Junit test for the case where there is a problem with a custom supplied builder. Currently that seems to be possibly inconsistent (fall back vs hardcoded change in annotation processing)
* @param project the project to participate in | ||
* @param isTest whether the annotation processor path is for test code | ||
* @return the annotation processor paths | ||
* @since 3.38 |
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.
Would be better if such API changes could be avoided, given that is more like an experiment
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.
Annotation processing configurations, such as the processor path and output directory, are part of the standard compiler arguments. It is better to explicitly provide these configurations within the host builder (for instance, AbstractImageBuilder) when invoking the custom compiler.
Currently, jdt.core utilizes the CompilationParticipant mechanism to interact with the APT plugin for annotation processing tasks. Extending the CompilationParticipant class appears to be a natural choice for obtaining APT configurations. Any other ideas we could consider for accessing APT information?
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.
Any other ideas we could consider for accessing APT information?
Does your client code need to call or override that API or can jdt just typecast to its internal implementation?
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.
Does your client code need to call or override that API
My client code does not use this API. It's AbstractImageBuilder in jdt.core that calls the API to get the related information and pass the result to the client code.
can jdt just typecast to its internal implementation?
jdt.core doesn't depend on the apt plugin directly. It uses extension point to discover AptCompilationParticipant instance. Not sure how to use typecast.
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.
Let a new intermediate jdt internal class
AbstractAptCompilationParticipant extends CompilationParticipant, let
AptCompilationParticipant extends AbstractAptCompilationParticipant (jdt apt should be a friend of jdt),
downcast CompilationParticipant to AbstractAptCompilationParticipant if the participiant is an instance of that.
That's a little detour but avoids otherwise unneeded public API.
-- Or mark the new API as Provisional so that it could be removed again without further warning.
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.
Good point. thanks for suggestion.
* Provisional API for experimental support for an alternative compiler | ||
* and can be removed without notice. |
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 looks like a case for @noreference
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.
nice. @noreference tag added. thanks.
/** | ||
* @since 3.38 | ||
*/ | ||
public class CompilerConfiguration { |
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.
What about this? Are we exposing this to the outside world too? If not, we need @noreference here as well. Otherwise, this could use some documentation.
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.
As this seems on;y used by AbstractImageBuilder, what about just moving it in the same (internal) package?
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 is not only used by internal AbstractImageBuilder. It will be also used by clients for retrieving the compiler options.
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.
But aren't all the expected clients basically implementing AbstractImageBuilder (so already consuming this internal package) ?
While I think all this will need to be API at some point, as it's early stage, I think we'd rather keep as much as possible in "internal" packages, until we feel comfortable enough with the design to turn it API.
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.
The clients don't implement AbstractImageBuilder, but the ECJ internal Compiler class. Yes, we can put the new CompilerConfiguration within internal package since it remains accessible from other plugin.
Just curious what's guideline to split the public package and internal package in the JDT codebase?
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.
Ah yes, my bad!
The public package become API that we can then hardly modify and that we have to maintain as such. internal packages are basically not supported externally and can be changed more freely. So in general, unless there is a good reason for something to become API, we try to keep things internal as it makes further maintenance significantly easier.
But here, it can be a case where a new API class makes sense. I think it's actually good to have an actual object formalizing portable standard compiler configuration that could be used in multiple clients, rather than passing some options and (potentially unknown) participants.
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.
Exactly. Currently, project compiler-related settings are scattered across multiple places. Introducing a centralized class to formalize them would make them easily portable for other clients to reuse.
@testforstephen can this one be rebased and updated and turned from draft to PR already? |
7f4d9e1
to
3159b36
Compare
Done. Here’s the CI failure message. This CI error also occurs for other pull requests such as #2519. It seems to be a general environment issue when we start a new version (e.g., 4.33).
|
114cf81
to
47685b2
Compare
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/AbstractImageBuilder.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/model/org/eclipse/jdt/core/compiler/CompilerConfiguration.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/AbstractImageBuilder.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/AbstractImageBuilder.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core.tests.builder/src/org/eclipse/jdt/core/tests/builder/BasicBuildTests.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core.tests.builder/src/org/eclipse/jdt/core/tests/builder/BasicBuildTests.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/model/org/eclipse/jdt/core/compiler/CompilerConfiguration.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/AbstractImageBuilder.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/AbstractImageBuilder.java
Outdated
Show resolved
Hide resolved
47685b2
to
5f60fc3
Compare
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/compiler/CompilerConfiguration.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/AbstractImageBuilder.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/AbstractImageBuilder.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/AbstractImageBuilder.java
Outdated
Show resolved
Hide resolved
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/AbstractImageBuilder.java
Show resolved
Hide resolved
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.
overall it looks like its going in the right direction. Please fix the faiiing tests
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/builder/AbstractImageBuilder.java
Outdated
Show resolved
Hide resolved
5f60fc3
to
3033c7a
Compare
org.eclipse.jdt.core/model/org/eclipse/jdt/internal/compiler/CompilerConfiguration.java
Outdated
Show resolved
Hide resolved
This is currently for experimental Javac support and not intended for a general audience. That's why we retain the internal class references and minimize intrusion into the existing mechanism.
Already replied in the related comment.
The warning for require-bundle is fixed.
No idea for this. I don't see the warnings for them in the Eclipse IDE.
Fixed. |
I could still see it, perhaps related to what target platform one had configured in Eclipse, but once I updated to 1627800 the warnings disappeared. Theory: with the redundant Require-Bundle perhaps we saw classes from o.e.jdt.core via the wrong dependency-path: when seen as a regular dependency, access is discouraged, when seen as owned by the fragment host, all is fine. |
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.
Once we have a green build this is good to go - as the experimental option it is, perhaps to be revisited before the next release.
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 agree the current proposal is a (very) good enough iteration.
I think everything @stephan-herrmann suggested or remarked is relevant though, so if there is a chance to cover some of them easily right now, then I think it's better to do it before merging, so that's fewer reasons to come back to this topic later.
But if implemented the suggested change seems too much effort at the moment, I'm all for merging as is now.
@jukzi @stephan-herrmann @jarthana @mickaelistria @fbricon thanks everyone for the review. I appreciate all your feedback. |
Hey @testforstephen [Jinbo Wang] - After pulling this change, Am getting compilation errors |
For the IDE, I guess you will have to import the new projects from the repo after pulling. |
Do you have org.eclipse.jdt.core.tests.builder.mockcompiler imported in your workspace? |
Hey thanks all - yes, got it resolved. Didn't realize that a new project was added - a rare occurrence in jdt.core world - [and how stupid of me for not taking that possibility into consideration :( ] - thanks! |
I am typical using OOmphs "Perform Setup Task" but in this case it does not work. Does the new project need to be registered somewhere for a OOmph setup? @merks |
That depends. The ones with * requirement will find all projects, the ones with requirements listed will transitively include all those requirements: So if there is something new somewhere in the clones that do not use * and are not required in any feature already required, then it may need to be added. Just to avoid reading the whole long thread, which specific project is of concern here? |
Doing a pull and resolving the modular targlet resulted in:
So that appears to be working okay. |
eclipse.jdt.core\org.eclipse.jdt.core.tests.builder.mockcompiler.project |
Back to original code now. While thinking about comments on #2606 I've noticed that we use a system property to select compiler. Sorry I haven't participated in the review here, is there any reason that the alternative compiler is supplied as a system property and not compiler option here? Line 582 in b9f1cdd
Wouldn't be natural to read which compiler is to be used few lines above from project options in Line 561 in b9f1cdd
If I read the code right, this would be possible as the compiler is created per project builder and that also would allow have two compilers running in same IDE? Also this would allow any set of tests running in parallel with both compilers etc, avoid mocking of system properties. Of course one could still have that property for projects without settings. |
Currently, the only reason is convenience: a system property allows to dynamically switch compiler in the IDE. They allow to repeat the same operation with a simple flag that can be easily controlled in a debugger to compare results, they also allow easy "external" configuration (eg JDT-LS can just add a
Yes, that would be possible with your proposal to have distinct projects using distinct compilers in parallel. Currently, the system property only allows to switch the compiler globally for all projects (but dynamically). |
@mickaelistria : so would you or @testforstephen follow up on that proposal? |
This particular proposal is not so interesting for our current goals regarding Javac integration. However, if you are certain that this can be soon profitable for the JDT Core project, and since it's not too much code to change, I think we can invest the small effort to unleash extra value to the ecosystem. So the (genuine) question is now for you: do you really think if we develop it now, it's going to be useful soon? |
I see the system property as an implementation of the statement that this is an experimental flag not to be exposed to any wider audience. Right now we wouldn't want the option to (automatically?) appear in Creating a compiler option can IMHO wait until we discuss making this an official feature. |
…n JavaBuilder (eclipse-jdt#2497) * Add the option to use an alternative compiler for compiling classes in JavaBuilder
…n JavaBuilder (eclipse-jdt#2497) * Add the option to use an alternative compiler for compiling classes in JavaBuilder
This is a follow up to #2490
What does in this PR:
-DAbstractImageBuilder.compilerFactory=x.y.z.MyCompilerFactory
) to specify the compiler factory that produces the compiler for compiling classes in AbstractImageBuilder. Refer to MockCompiler in the test case org.eclipse.jdt.core.tests.builder.BasicBuildTests.MockCompiler for an example of a custom compiler implementation, and see JavacCompiler for a real Javac implementation.