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

Synchronize jdt.compiler.targetPlatform in J2SE profiles #655

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

HannesWell
Copy link
Member

The org.eclipse.jdt.core.compiler.codegen.targetPlatform values in the J2SE profiles seem to not match what I would expect based on the Id of the profile and based on their description:

These values where introduced in 3165d6c. The corresponding compliance settings where changed for example in
Bug 268152.

I don't know if there is a specific reason to have different values or if it was a mistake.
Given that ECJ is about to drop support for all Java version below 1.8 these values are probably irrelevant most of the time any ways. I just noticed it during my work for eclipse-pde/eclipse.pde#1324, where this value influences the order of these old java versions (which is in practice also not very relevant anymore).

@HannesWell HannesWell requested a review from tjwatson July 7, 2024 05:56
Copy link

github-actions bot commented Jul 7, 2024

Test Results

  660 files  ±0    660 suites  ±0   1h 13m 27s ⏱️ - 1m 59s
2 195 tests ±0  2 148 ✅ ±0   47 💤 ±0  0 ❌ ±0 
6 729 runs  ±0  6 586 ✅ ±0  143 💤 ±0  0 ❌ ±0 

Results for commit 4f6acc1. ± Comparison against base commit f582f10.

@tjwatson
Copy link
Contributor

tjwatson commented Jul 8, 2024

I don't recall why these values are the way they are, other than we got them from the JDT team and they were the ones that cared about what they were set to. The framework never cared. I am therefore reluctant to go against what Markus stated in https://bugs.eclipse.org/bugs/show_bug.cgi?id=268152#c0

E.g. "J2SE-1.2.profile" contains ...

org.eclipse.jdt.core.compiler.compliance=1.2
org.eclipse.jdt.core.compiler.source=1.2

... but these must be at least 1.3 if you look at the definitions of these
options in the JavaCore class.

@HannesWell
Copy link
Member Author

I don't recall why these values are the way they are, other than we got them from the JDT team and they were the ones that cared about what they were set to. The framework never cared. I am therefore reluctant to go against what Markus stated in https://bugs.eclipse.org/bugs/show_bug.cgi?id=268152#c0

E.g. "J2SE-1.2.profile" contains ...
org.eclipse.jdt.core.compiler.compliance=1.2
org.eclipse.jdt.core.compiler.source=1.2
... but these must be at least 1.3 if you look at the definitions of these
options in the JavaCore class.

Yes I read that as well and since it only mentioned compliance and source I only adjusted compiler.codegen.targetPlatform.
The motivation for this is just to avoid special handling of the adjusted EEs in https://github.com/eclipse-pde/eclipse.pde/pull/1324/files#r1669333170. But in the end it would just be nice to have and it's nothing that can't be achieved in other ways. I just though this would be the most elegant way.
At the same time I assume JDT does not care anymore about these values since ECJ has dropped or is about to drop support for all values?
@iloveeclipse or @jarthana can you maybe shed some light on this?

@iloveeclipse
Copy link
Member

At the same time I assume JDT does not care anymore about these values since ECJ has dropped or is about to drop support for all values?

ECJ may be not interested, but debugger / JDT UI may do.
However, I'm not sure where the profile data you changed is surfacing in the Java code, does that affect org.eclipse.jdt.internal.launching.environments.ExecutionEnvironment ?
If yes, see

  • org.eclipse.jdt.internal.corext.util.JavaModelUtil.getExecutionEnvironmentCompliance(IExecutionEnvironment)
  • org.eclipse.jdt.launching.environments.IExecutionEnvironmentsManager.getSupportedExecutionEnvironments()

@HannesWell
Copy link
Member Author

At the same time I assume JDT does not care anymore about these values since ECJ has dropped or is about to drop support for all values?

ECJ may be not interested, but debugger / JDT UI may do. However, I'm not sure where the profile data you changed is surfacing in the Java code, does that affect org.eclipse.jdt.internal.launching.environments.ExecutionEnvironment ?

Yes, the values defined here are read in https://github.com/eclipse-jdt/eclipse.jdt.debug/blob/3a3eba11c7b0c6079ff44a99fb409f277ec5ec21/org.eclipse.jdt.launching/launching/org/eclipse/jdt/internal/launching/environments/ExecutionEnvironment.java#L425-L437

If yes, see

* `org.eclipse.jdt.internal.corext.util.JavaModelUtil.getExecutionEnvironmentCompliance(IExecutionEnvironment)`

* `org.eclipse.jdt.launching.environments.IExecutionEnvironmentsManager.getSupportedExecutionEnvironments()`

These places only use the COMPILER_COMPLIANCE not the COMPILER_CODEGEN_TARGET_PLATFORM. I have also checked the other callers of IExecutionEnvironment.getProfileProperties(), which is also called by IExecutionEnvironment.getComplianceOptions(), and didn't found a caller that relies on the value for the COMPILER_CODEGEN_TARGET_PLATFORM key.
With that it looks to me that this value is effectively unused at the moment.

@HannesWell
Copy link
Member Author

@tjwatson how should we proceed here? I think it is fine to submit this.

@tjwatson
Copy link
Contributor

@tjwatson how should we proceed here? I think it is fine to submit this.

I approved, but I still am unsure what externally observable problem it is fixing.

@HannesWell
Copy link
Member Author

@tjwatson how should we proceed here? I think it is fine to submit this.

I approved, but I still am unsure what externally observable problem it is fixing.

Thanks, then let's submit this now.

It is fixing the problem that IExecutionEnvironment..getProfileProperties().get(JavaCore.COMPILER_CODEGEN_TARGET_PLATFORM) returns unexpected values for the EEs whoose definitions are changed is this PR. The new values are the expected values.

At the moment this has to be handled specifically in PDE:
https://github.com/eclipse-pde/eclipse.pde/blob/029c7d798b1b546e0379898375b07b3b48109a5c/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/util/VMUtil.java#L116-L131

With this, that special handling can be removed.

@HannesWell HannesWell merged commit 95971b5 into master Jul 17, 2024
27 of 28 checks passed
@HannesWell HannesWell deleted the adapt-profile-targets branch July 17, 2024 21:03
HannesWell added a commit to HannesWell/eclipse.pde that referenced this pull request Jul 21, 2024
This also leverages the changs from
eclipse-equinox/equinox#655
HannesWell added a commit to HannesWell/eclipse.pde that referenced this pull request Jul 21, 2024
This also leverages the changs from
eclipse-equinox/equinox#655
HannesWell added a commit to eclipse-pde/eclipse.pde that referenced this pull request Jul 21, 2024
This also leverages the changs from
eclipse-equinox/equinox#655
fedejeanne pushed a commit to fedejeanne/eclipse.pde that referenced this pull request Jul 31, 2024
akurtakov added a commit to eclipse-tycho/tycho that referenced this pull request Sep 10, 2024
github-actions bot pushed a commit to eclipse-tycho/tycho that referenced this pull request Sep 10, 2024
akurtakov added a commit to eclipse-tycho/tycho that referenced this pull request Sep 11, 2024
github-actions bot pushed a commit to eclipse-tycho/tycho that referenced this pull request Sep 11, 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.

3 participants