Consistency of `--jvm` CLI options: allowing optional JVM path #13225

JaroslavTulach · 2025-06-05T07:35:18Z

Pull Request Description

Fixing broken sbt command by adding support for optional parameter to --jvm argument:

sbt:enso> runProjectManagerDistribution --debug

was broken by incorrect assumption that --jvm flags may take arguments. The enso one did accept argument, the project-manager one didn't. This PR fixes that by giving the project-manager --jvm flag an optional path attribute.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
All code follows the
Scala,
Java,
Unit tests have been written where possible.

github-actions · 2025-06-05T07:36:27Z

✨ GUI Checks Results

Summary

🧹 Prettier: ✅ Passed
🧰 GUI Checks: ❌ Failed
📚 Storybook: ❌ Failed

⚠️ Action Required: Please review the failed checks and fix them before merging.

See individual check results for more details.

This change makes Native Image build opt-out rather than opt-in. To use JVM mode, IDE has to start with `--jvm` or `ENSO_LAUNCHER=shell`. An alternative approach to #12501. Closes #12483. Likely also addresses #12303.

...ct-manager/src/main/scala/org/enso/projectmanager/boot/configuration/MainProcessConfig.scala

JaroslavTulach · 2025-06-05T07:44:14Z

...la/runtime-version-manager/src/main/scala/org/enso/runtimeversionmanager/runner/Runner.scala

-            val requiresJVMRunner =
-              ensoLauncher.exists(_.equals("shell")) || runSettings.jvmMode
-            if (requiresJVMRunner) {
+            if (runSettings.jvm.isDefined) {


Is there a recommended way to (unit) test this logic?

I don't think we are testing this at all. Make sure you try manually launching project-manager.

JaroslavTulach · 2025-06-05T07:47:17Z

...la/runtime-version-manager/src/main/scala/org/enso/runtimeversionmanager/runner/Runner.scala

-            if (requiresJVMRunner) {
+            if (runSettings.jvm.isDefined) {
+              val javaHome = runSettings.jvm.get
+              val javaExec = if (javaHome.isDefined) {


I wrote this code in a way I can read it later.

I assume there is some fancy way of using Option.map.orElse instead

reviewers feel free to make such a local stylistic changes directly

you have write access to the code in this PR

JaroslavTulach · 2025-06-05T07:47:56Z

...la/runtime-version-manager/src/main/scala/org/enso/runtimeversionmanager/runner/Runner.scala

-
-object Runner {
-
-  private val LAUNCHER_ENV_NAME = "ENSO_LAUNCHER"


I don't think there is any need to support ENSO_LAUNCHER=shell when there is --jvm option in project-manager. Removing the ENSO_LAUNCHER check.

...ion-manager/src/main/scala/org/enso/runtimeversionmanager/runner/LanguageServerOptions.scala

Akirathan

Unification of --jvm argument is good.

Make sure you manually test it, I am afraid there are no unit tests for this.
Please choose any other type than Option[Option[Path]].

...n/scala/org/enso/projectmanager/infrastructure/languageserver/LanguageServerController.scala

...la/runtime-version-manager/src/main/scala/org/enso/runtimeversionmanager/runner/Runner.scala

...ct-manager/src/main/scala/org/enso/projectmanager/boot/configuration/MainProcessConfig.scala

Co-authored-by: Hubert Plociniczak <[email protected]>

JaroslavTulach · 2025-06-06T04:05:02Z

There are compilation failures in Launcher
trying to fix them I realized there is completely different CLI parser for ensoup than for enso, project-manager...
result of Launcher CLI Arguments Parsing #995, I guess
@radeusgd can you shed some light on the reasons why ensoup has different CLI parser than other components?
I got the launcher to compile, but there are doubts...

JaroslavTulach · 2025-06-06T04:32:45Z

engine/launcher/src/main/scala/org/enso/launcher/cli/LauncherApplication.scala

-  private def jvmMode =
-    Opts.flag(
+  private def jvm =
+    Opts.optionalParameter[Path](


I have to admit I am not sure at all how to represent this optional option with optional parameter in this system?

in 8728e1a I at least fixed the compilation

but using Option(jvm) at various places is unlikely correct, right @radeusgd?

JaroslavTulach · 2025-06-06T04:33:48Z

engine/launcher/src/main/scala/org/enso/launcher/cli/LauncherApplication.scala

          additionalArgs
        ) => (config: Config) =>
          Launcher(config).runRun(
            path                = path,
            versionOverride     = versionOverride,
            useSystemJVM        = systemJVMOverride,
            jvmOpts             = jvmOpts,
-            jvmMode             = jvmMode,
+            jvm                 = Option(jvm),


Assuming jvm is never null, this will always produce Some and that's not what we want.

It should be None if the --jvm option wasn't specified at all

How does one represent that with Opts factory methods, @radeusgd?

I'm not sure if I understand what you want to achieve.

Do I understand correctly that you want to support 2 scenarios?

--jvm with nothing after it - to enable JVM mode but no path to the JVM

--jvm <path> with a path to the JVM that should be used

If the requirements above are correct, then I'm afraid that the framework does not support such a duality at all.

The problem is the framework was created with some other assumptions in mind that contradict the approach above. One of the features we wanted in the framework was that the flags and options can appear in mostly any order relative to positional arguments.

Thus we either have flags that do not expect any 'values' after them, or parameters that expect 1 'value' for it. Thus, if I encounter --foo bar - depending on if --foo is a flag or a parameter, I can determine if bar should be treated as the 'value' for parameter foo or if bar is just a positional argument following --foo.

Of course I can imagine we could take some other approach to this, but these are assumptions of this framework and I think it may be unlikely that it may be easy to change them.

So IF my assumptions about what you want from this parameter/flag are correct, I would suggest to try some other approach for selecting the JVM to use. As this flag is doing 2 things at once (one could argue violating SRP).

I'd suggest to keep a boolean flag --jvm that either is there or isn't present.

Note that we already have a flag --use-system-jvm in the same launcher (in fact it's next to your --jvm option). Perhaps that + setting of JAVA_HOME and PATH could be sufficient.

If not, I'd suggest to add --use-custom-jvm=<path> parameter that would override the default selection.

Unless I misunderstood your requirements, and you are okay for <path> argument of --jvm to be required - i.e. you either:

do not specify --jvm at all

specify --jvm some-path always with some path.

In that case I think you should have it almost ready - the optionalParameter yields results in form of Option[A] so isn't your jvm already an Option[Path]? Then the Option(jvm) should not be needed. Or is the type something else?

suggest to try some other approach

With ...

The primary goal of this PR is to fix sbt "runProjectManagerDistribution --debug"

... being achieved by giving the --jvm flag in project-manager and enso the same meaning and behavior. It is not really possible to try some other approach in ensoup. That wouldn't bring the needed consistency.

But let's ask a question:

why do we have the --jvm flag at all?

it has been added by Make Native Image opt-out #12515, but

why do we need the flag at all?

unless I am mistaken ensoup can pass any options (specified after --) to enso, right Radek?

then we could just delete the special support and use ensoup run -- --jvm....

what do you think @hubertp, @radeusgd?

vote +1 for me to open a new PR that removes the ensoup --jvm option/flag/parameter altogether

Yes, passing some custom options can be done by passing them after --. I think due to the non-standard (from perspective of ensoup's CLI) behaviour of --jvm, passing it after -- may be the preferred solution.

On the other hand let's not that part of ensoup's job is also selecting which JVM to use to run the engine. SO I feel like we are kind of having 2 places that now do a similar thing here. This duplication bothers me a bit, but it's a completely separate issue.

So as for argument passing - yeah I'd vote for passing --jvm after -- and thus we avoid the issues with CLI parsing.

But still slightly worried that we have 2 components selecting JVMs - right now ensoup will select some JVM for the engine and then the engine may actually select another one. Messy.

passing it after -- may be the preferred solution.

I can try to open a PR removing this ensoup --jvm altogether. Is that OKeyish, @hubertp?

part of ensoup's job is also selecting which JVM to use to run the engine.

can you point me to the specification?

it may need updating - it was written prior enso being a native executable

Some docs:
https://github.com/enso-org/enso/blob/develop/docs/distribution/launcher.md#enso-and-graal-version-management
https://github.com/enso-org/enso/blob/develop/docs/distribution/launcher-cli.md#--use-system-jvm

Code that manages GraalVM versions and finds/installs the JVM for a given engine version:
https://github.com/enso-org/enso/blob/develop/lib/scala/runtime-version-manager/src/main/java/org/enso/runtimeversionmanager/components/GraalVersionManager.java
https://github.com/enso-org/enso/blob/develop/lib/scala/runtime-version-manager/src/main/scala/org/enso/runtimeversionmanager/components/RuntimeVersionManager.scala#L145-L168

Building engine manifest that specifies the JVM used with it:

enso/project/DistributionPackage.scala

Lines 191 to 196 in a1aa84c

buildEngineManifest(

template = file("distribution/manifest.template.yaml"),

destination = distributionRoot / "manifest.yaml",

graalVersion = graalVersion,

javaVersion = javaVersion

)

The representation of the Manifest file in code (and its parsing via YamlDecoder):
https://github.com/enso-org/enso/blob/develop/lib/scala/runtime-version-manager/src/main/scala/org/enso/runtimeversionmanager/components/Manifest.scala#L39

For each engine release we create a manifest.yaml artifact that stores the JVM version needed to run that engine.
ensoup, when installing a given release, downloads this manifest first and it will then install the required GraalVM version if it is not yet installed. All JVMs used for Enso are kept in $ENSO_DATA_DIRECTORY/runtime (see https://github.com/enso-org/enso/blob/a1aa84c0285de4b86168ace233d6d5b50693311c/docs/distribution/distribution.md#installed-enso-distribution-layout).

Perhaps this logic should be updated if the requirements are changed.

Perhaps if no JVM is needed as its running native (but --jvm flag still needs a JVM, right?) the JVM version in manifest.yaml could be set to null. Proper launcher upgrade (breaking change) would be needed for that though as currently null is not a valid value and the logic would break. Ideally it should be done in a backwards compatible way (e.g. by creating a new launcher version and bumping minimum-launcher-version so that a newer launcher is used for engines that rely on the new logic).

GraalVM will still be needed even with enso native executable

first of all --jvm needs the GraalVM

the GraalVM will be even more needed with Using Channel to load Java classes #13238

unless we create an native only mode

that would prevent automatic delegation to HotSpot JVM

Some docs:
https://github.com/enso-org/enso/blob/develop/docs/distribution/launcher.md#enso-and-graal-version-management
https://github.com/enso-org/enso/blob/develop/docs/distribution/launcher-cli.md#--use-system-jvm

OK. I believe the following flag can be removed...

--use-system-jvm
Tells the launcher to use the default JVM (based on JAVA_HOME) instead of the managed one. Will not work if the set-up JVM version is not GraalVM.
GraalVM Override
While the launcher manages its own installation of GraalVM to ensure that the right JVM version is used to launch each version of Enso, the user can override this mechanism to use the installed system JVM instead. This is an advanced feature and should rarely be used.

...such an advanced feature deserves to be complicated - e.g. ensoup run -- --jvm path_to_JDK would be good enough replacement...

Consistency of --jvm arguments: allowing optional JVM path

b8608cb

JaroslavTulach requested review from radeusgd and GregoryTravis June 5, 2025 07:35

JaroslavTulach self-assigned this Jun 5, 2025

JaroslavTulach requested review from 4e6, hubertp and Akirathan as code owners June 5, 2025 07:35

JaroslavTulach changed the title ~~Consistency of --jvm arguments: allowing optional JVM path~~ Consistency of --jvm CLI options: allowing optional JVM path Jun 5, 2025

JaroslavTulach mentioned this pull request Jun 5, 2025

Execute yjs from insight script #11965

Draft

5 tasks

Note in changelog

d5bfefb

JaroslavTulach mentioned this pull request Jun 5, 2025

Make Native Image opt-out #12515

Merged

2 tasks

JaroslavTulach commented Jun 5, 2025

View reviewed changes

Akirathan reviewed Jun 5, 2025

View reviewed changes

...ion-manager/src/main/scala/org/enso/runtimeversionmanager/runner/LanguageServerOptions.scala Show resolved Hide resolved

Akirathan approved these changes Jun 5, 2025

View reviewed changes

hubertp approved these changes Jun 5, 2025

View reviewed changes

JaroslavTulach and others added 2 commits June 5, 2025 19:00

jvmMode=false becomes jvm=None

b214e3a

Use .map and .getOrElse

b491672

Co-authored-by: Hubert Plociniczak <[email protected]>

JaroslavTulach force-pushed the wip/jtulach/DebugProjectManagerDistribution branch from b3251dc to b491672 Compare June 5, 2025 17:17

JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Jun 5, 2025

GregoryTravis approved these changes Jun 5, 2025

View reviewed changes

enso-bot bot mentioned this pull request Jun 6, 2025

Dual NI + JVM Mode for Loading Libraries #13172

Closed

3 tasks

Using Option[Option[Path]] in launcher

8728e1a

JaroslavTulach force-pushed the wip/jtulach/DebugProjectManagerDistribution branch from 2ea881b to 8728e1a Compare June 6, 2025 04:29

JaroslavTulach commented Jun 6, 2025

View reviewed changes

JaroslavTulach removed the CI: Ready to merge This PR is eligible for automatic merge label Jun 6, 2025

JaroslavTulach merged commit 2f27473 into develop Jun 6, 2025
81 of 83 checks passed

JaroslavTulach deleted the wip/jtulach/DebugProjectManagerDistribution branch June 6, 2025 17:24


		object Runner {

		private val LAUNCHER_ENV_NAME = "ENSO_LAUNCHER"

	buildEngineManifest(
	template = file("distribution/manifest.template.yaml"),
	destination = distributionRoot / "manifest.yaml",
	graalVersion = graalVersion,
	javaVersion = javaVersion
	)

Consistency of --jvm CLI options: allowing optional JVM path #13225

Consistency of --jvm CLI options: allowing optional JVM path #13225

Uh oh!

Conversation

JaroslavTulach commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Description

Important Notes

Checklist

Uh oh!

github-actions bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✨ GUI Checks Results

Summary

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Akirathan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JaroslavTulach commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaroslavTulach Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaroslavTulach Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaroslavTulach Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Consistency of `--jvm` CLI options: allowing optional JVM path #13225

Consistency of `--jvm` CLI options: allowing optional JVM path #13225

JaroslavTulach commented Jun 5, 2025 •

edited

Loading

github-actions bot commented Jun 5, 2025 •

edited

Loading

JaroslavTulach commented Jun 6, 2025 •

edited

Loading

JaroslavTulach Jun 6, 2025 •

edited

Loading

JaroslavTulach Jun 6, 2025 •

edited

Loading

JaroslavTulach Jun 8, 2025 •

edited

Loading