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

Honor configured profile when determining the execution environment #4549

Closed

Conversation

ptziegler
Copy link
Contributor

When calculating the best matching profile, any explicitly configured profile is effectively overwritten with the one matching the current runtime version.

This makes it impossible to select a profile older than the JRE Tycho is executed with. As a result, properties such as the "useJDK" are ignored if the BREE of the bundle is smaller than the current Java version.

When calculating the best matching profile, any explicitly configured
profile is effectively overwritten with the one matching the current
runtime version.

This makes it impossible to select a profile older than the JRE Tycho is
executed with. As a result, properties such as the "useJDK" are ignored
if the BREE of the bundle is smaller than the current Java version.
@ptziegler
Copy link
Contributor Author

For context: I'm having a test bundle with Java 17 as required EE, but have to run Tycho with Java 21 due to this whole Lucene 10 stuff. I've set useJDK to BREE but despite that, the tests continue to be executed with java 21.

Copy link

Test Results

  603 files  ±0    603 suites  ±0   4h 16m 53s ⏱️ -1s
  432 tests ±0    378 ✅  -  47   7 💤 ±0  2 ❌ +2   45 🔥 + 45 
1 296 runs  ±0  1 137 ✅  - 137  22 💤 ±0  2 ❌ +2  135 🔥 +135 

For more details on these failures and errors, see this check.

Results for commit 8d60784. ± Comparison against base commit cfd83b1.

@ptziegler
Copy link
Contributor Author

I suppose there's more to it than to simply return early... The configured profile is still ignored but my plate is already full enough to also look deeper into this :/

@ptziegler ptziegler closed this Dec 20, 2024
@laeubi
Copy link
Member

laeubi commented Dec 21, 2024

@ptziegler I'm not sure we are looking at the right place here?

useJDK=BREE is mainly used (and referenced) by compiling that allows to use a "real" JVM... due to today's good support for target in the compiler I would actually not recommend to use it anymore as it complicates the project setup and the benefit is quite low. Nevertheless it should not be ignored and if you don't have a matching JVM in your toolchains you will get an error. This is configured here.

But as you referring to test execution, maybe you mean this setting here so have you really set this one? Here it might be useful to test on a real VM but also the benefit is often quite low and if any of your dependencies require a higher java version it will simply not work at all.

Beside that I'm not 100% sure we test this particular case, but it should be possible to add a test-case as we can configure different JVMs in Tycho CI builds without much problem.

@ptziegler ptziegler deleted the honor-configured-profile branch December 21, 2024 16:30
@ptziegler
Copy link
Contributor Author

I'm not sure we are looking at the right place here?

Perhaps it isn't, but that's the first place I noticed while debugging that looked odd.

if (configuredProfile != null) {
// non standard profile, stick to it
sink.setProfileConfiguration(configuredProfileName, reason);
}

This is dead code, because the profile configuration is always overwritten a few lines further down. Assuming that the ExecutionEnvironmentConfigurationImpl is used, of course...

public void setProfileConfiguration(String profileName, String configurationOrigin) throws IllegalStateException {
checkConfigurationMutable();
if (profileName == null) {
throw new NullPointerException();
}
this.configurations[SECONDARY] = new ProfileConfiguration(profileName, configurationOrigin);
}

due to today's good support for target in the compiler I would actually not recommend to use it anymore as it complicates the project setup and the benefit is quite low.

From my experience, using target is always a gamble whether it will actually detect illegal API (e.g. the SequencedCollection) when targeting Java 17. So I believe that compiling with a matching version is much more reliable.

But as you referring to test execution, maybe you mean this setting here so have you really set this one? Here it might be useful to test on a real VM but also the benefit is often quite low and if any of your dependencies require a higher java version it will simply not work at all.

Yes, I've configured useJDK for both the tycho-compiler-plugin and the tycho-surefire-plugin. It works for the former, but not for the latter.

What made me gave up was even when the tests are started with Java 17, they'll fail because the Eclipse application can't be launched anymore.

I probably could've spend more time trying to launch the Eclipse IDE, rather than the SDK but ultimately, I accepted that this whole ordeal really isn't worth the effort.

@laeubi
Copy link
Member

laeubi commented Dec 21, 2024

Yes it looks a bit odd but it is not really "new" seems to be introduced by this commit maybe @mickaelistria can tell a bit more about the rationale, especially as the build.properties defined one is used as-as and the user has no choice at all I agree it looks wrong here.

I can only imagine that the intend is/was that if you have some bundles requiring it as a dependency this would help, but in this case I would have expected that the dependencies are actually examined. Also I think one might want to consider the target platform BREE as well... We can likely not change it for Tycho 4 easily but this should be reworked for Tycho 5!

@laeubi
Copy link
Member

laeubi commented Dec 22, 2024

I have now take a deeper look and as always it is a bit more complicated an mangled... Actually there is a setting that allows to disable the consideration of EEs (or more formally make all EEs available) or otherwise only use one EE selectively. But this is used for resolve and for compile and for target resolution. I think there are historically a lot of options also to support JustJ in the early days and before there where modular VMs.

Now that java evolves faster an JustJ is better integrated, one might drop some of those and better always resolve the target platform with all EEs (if not explicitly specified) or simply with the highest EE found.

After that one might decide that the Bundle-BREE is used to check the resolved dependencies an either warn or fail if the there is anything higher required in the chain. For compile it might default to warn, and for tests it should default to failure.

I have now created

@laeubi
Copy link
Member

laeubi commented Dec 22, 2024

@ptziegler by the way just for your case it should have worked to set <executionEnvironment>JavaSE-17</executionEnvironment> in the target platform to compile+run with java 17 always.

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