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

Forward release setting to AbstractImageBuilder #3606

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mickaelistria
Copy link
Contributor

What it does

Honor the OPTION_release setting in CompilerOptions so it can easily be used in downstream AbstractImageBuilder.

Note that there is currently no practical consumer for this option in ECJ-based builder; but it still seems interesting to allow proper access to this setting anyway.

How to test

Run tests, see no regression.

Author checklist

@mickaelistria mickaelistria force-pushed the compilerOptions-release branch from 99ecf3d to f13d5cc Compare January 24, 2025 08:57
@jukzi
Copy link
Contributor

jukzi commented Jan 24, 2025

mind to add any test for it so it is not accidentally removed?

@stephan-herrmann
Copy link
Contributor

what's wrong with asking this.javaBuilder.javaProject.getOption(JavaCore.COMPILER_RELEASE) or similar?

@mickaelistria
Copy link
Contributor Author

what's wrong with asking this.javaBuilder.javaProject.getOption(JavaCore.COMPILER_RELEASE) or similar?

Nothing too wrong, but since the non-deprecated constructors for Compiler and the AbstractImageBuilder.newCompiler() do use CompilerOptions as input without easy access to a particular project, it seems like honoring the release in CompilerOptions would make things much more consistent.

mind to add any test for it so it is not accidentally removed?

I'll try it soon.

@stephan-herrmann
Copy link
Contributor

... AbstractImageBuilder.newCompiler() do use CompilerOptions as input without easy access to a particular project

AbstractImageBuilder.newCompiler() certainly has no issues accessing the project:

protected Compiler newCompiler() {
	// disable entire javadoc support if not interested in diagnostics
	Map projectOptions = this.javaBuilder.javaProject.getOptions(true);
...

I simply don't get which problem you are solving... you may have a relevant usecase in mind, but I don't see it. --release is interesting for name environments not for the compiler.

@mickaelistria
Copy link
Contributor Author

But then Compiler newCompiler = compilerFactory.newCompiler(..) looses track of the project, but has access to the CompilerOptions.
Another argument is the one can expect that the value returned by new CompilerOptions(this.javaBuilder.javaProject.getOptions(true)).getMap() contains no different value (not contain all values, but all the contained ones are equivalent) than initial this.javaBuilder.javaProject.getOptions(true). Some data may be lost in translation, but none should be changed in translation, as it's currently the case for release.

And as you probably have guess, when working on an alternative compiler (eg Javac) we need to have access to information about whether --release is to be used; and CompilerOptions are used to access the relevant options.

@stephan-herrmann
Copy link
Contributor

... as you probably have guess ...

... I prefer explicit communication over guessing ... question answered.

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.

3 participants