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

Set in MOE 2.0 GraalVM as jdk instead of moe #6

Closed
wants to merge 1 commit into from

Conversation

Berstanio
Copy link
Contributor

This is currently only enabled for moe 2.0. I tested it a bit and it seems to work fine. Theoretical if I didn't missed anything we could also remove the whole sdk creation etc. also for MOE 1.9 and set directly the jdk which would be otherwise just wrapped.
(I also fixed some NPE bugs.)
Relatded prs: multi-os-engine/moe-plugin-gradle#10 multi-os-engine/moe-tools-common#2

@Noisyfox Noisyfox added this to To do in MOE 2.x Nov 9, 2021
@Noisyfox Noisyfox added this to the 2.0 milestone Nov 9, 2021
if (moeRootPath == null) {
return null;
String sdkPath = properties.getProperty(ProjectUtil.MOE_GRAALVM_HOME);
//New behavior only for MOE 2.0 currently. Adjusting it to MOE 1.x can still easily be done.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could add a "version" property to the gradle model, so we can handle SDK differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this still be done?

@Noisyfox Noisyfox moved this from To do to In progress in MOE 2.x Mar 19, 2022
@Berstanio
Copy link
Contributor Author

Berstanio commented Mar 29, 2022

I would also rework this so that in any case the project jdk is set, except for MOE 2.0 where GraalVM is set as jdk.
Using the moe sdk as jdk just leads to trouble and doesn't have any benefits as far as I'm aware.
Any objections?

@Noisyfox
Copy link
Member

I'd like to keep the MOE SDK type for moe 1.x project, because if the moe 1.x module is set to use a normal JDK, then in the IDE this module will contain classes from both MOE 1.x sdk (from Gradle dependencies) and the JDK you selected in IntelliJ module settings, so the auto completion will show you classes that's available from JDK but not available in MOE. The purpose of having a MOE sdk type is to get rid of classes from a normal JDK.

For 2.x I do agree that having a MOE sdk type is useless and we should set the GraalVM as the module JDK.

@Noisyfox
Copy link
Member

There is one thing I did wrong and need to fix first is the way of determine the JDK to run the build. I used the module JDK which causes a lot of problems. What I should be using is the JDK set in IDE Gradle settings instead.

@Berstanio
Copy link
Contributor Author

I'd like to keep the MOE SDK type for moe 1.x project, because if the moe 1.x module is set to use a normal JDK, then in the IDE this module will contain classes from both MOE 1.x sdk (from Gradle dependencies) and the JDK you selected in IntelliJ module settings, so the auto completion will show you classes that's available from JDK but not available in MOE. The purpose of having a MOE sdk type is to get rid of classes from a normal JDK.

Ahh, okay, I see! The reason why I wanted to remove it on 1.x too was, that I got the following error after a gradle sync, when running a build with the IDEA plugin: "Multi-OS Engine module build failedError while building Module SDK not configured or it's not a valid JDK ."

There is one thing I did wrong and need to fix first is the way of determine the JDK to run the build. I used the module JDK which causes a lot of problems. What I should be using is the JDK set in IDE Gradle settings instead.

Is the issue above also fixed by that?

@Noisyfox
Copy link
Member

"Multi-OS Engine module build failedError while building Module SDK not configured or it's not a valid JDK ."

Yep that's the exact issue I'm talking about.

@Noisyfox
Copy link
Member

I'll include this in #8 with a few changes

@Noisyfox Noisyfox closed this Jul 23, 2022
@Noisyfox Noisyfox moved this from In progress to Done in MOE 2.x Jul 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
2 participants