Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jun 6, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b8608cb
Consistency of --jvm arguments: allowing optional JVM path
JaroslavTulach Jun 5, 2025
d5bfefb
Note in changelog
JaroslavTulach Jun 5, 2025
b214e3a
jvmMode=false becomes jvm=None
JaroslavTulach Jun 5, 2025
b491672
Use .map and .getOrElse
JaroslavTulach Jun 5, 2025
8728e1a
Using Option[Option[Path]] in launcher
JaroslavTulach Jun 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@

#### Enso Language & Runtime

- [Allow optional path for `--jvm` option of `project-manager`][13225]
- [Prevent `Meta` access to private constructors and private fields][12905]
- [Encapsulating Private_Access constructor][#12976]
- [Upgrading Truffle][12500] (including its
Expand All @@ -59,6 +60,7 @@
[12976]: https://github.com/enso-org/enso/pull/12976
[12855]: https://github.com/enso-org/enso/pull/12855
[12905]: https://github.com/enso-org/enso/pull/12905
[13225]: https://github.com/enso-org/enso/pull/13225

# Enso 2025.1

Expand Down
12 changes: 6 additions & 6 deletions engine/launcher/src/main/scala/org/enso/launcher/Launcher.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ case class Launcher(cliOptions: GlobalCLIOptions) {
versionOverride: Option[SemVer],
useSystemJVM: Boolean,
jvmOpts: Seq[(String, String)],
jvmMode: Boolean,
jvm: Option[Option[Path]],
additionalArguments: Seq[String]
): Int = {
val actualPath = path.getOrElse(Launcher.workingDirectory.resolve(name))
Expand All @@ -89,7 +89,7 @@ case class Launcher(cliOptions: GlobalCLIOptions) {
path = actualPath,
name = name,
engineVersion = version,
jvmMode = jvmMode,
jvm = jvm,
normalizedName = normalizedName,
projectTemplate = projectTemplate,
authorName = globalConfig.authorName,
Expand Down Expand Up @@ -212,7 +212,7 @@ case class Launcher(cliOptions: GlobalCLIOptions) {
logLevel: Level,
useSystemJVM: Boolean,
jvmOpts: Seq[(String, String)],
jvmMode: Boolean,
jvm: Option[Option[Path]],
additionalArguments: Seq[String]
): Int = {
runner
Expand All @@ -223,7 +223,7 @@ case class Launcher(cliOptions: GlobalCLIOptions) {
versionOverride,
logLevel,
cliOptions.internalOptions.logMasking,
jvmMode,
jvm,
additionalArguments
)
.get,
Expand Down Expand Up @@ -257,7 +257,7 @@ case class Launcher(cliOptions: GlobalCLIOptions) {
logLevel: Level,
useSystemJVM: Boolean,
jvmOpts: Seq[(String, String)],
jvmMode: Boolean,
jvm: Option[Option[Path]],
additionalArguments: Seq[String]
): Int = {
val exitCode = runner
Expand All @@ -268,7 +268,7 @@ case class Launcher(cliOptions: GlobalCLIOptions) {
versionOverride,
logLevel,
cliOptions.internalOptions.logMasking,
jvmMode,
jvm,
additionalArguments
)
.get,
Expand Down
33 changes: 17 additions & 16 deletions engine/launcher/src/main/scala/org/enso/launcher/cli/LauncherApplication.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ object LauncherApplication {
versionOverride,
systemJVMOverride,
jvmOpts,
jvmMode,
jvm,
additionalArgs
) mapN {
(
Expand All @@ -90,7 +90,7 @@ object LauncherApplication {
versionOverride,
systemJVMOverride,
jvmOpts,
jvmMode,
jvm,
additionalArgs
) => (config: Config) =>
Launcher(config).newProject(
Expand All @@ -101,7 +101,7 @@ object LauncherApplication {
versionOverride = versionOverride,
useSystemJVM = systemJVMOverride,
jvmOpts = jvmOpts,
jvmMode = jvmMode,
jvm = Option(jvm),
additionalArguments = additionalArgs
)
}
Expand All @@ -112,11 +112,12 @@ object LauncherApplication {
"jvm",
"These parameters will be passed to the launched JVM as -DKEY=VALUE."
)
private def jvmMode =
Opts.flag(
private def jvm =
Opts.optionalParameter[Path](
Copy link
Member Author

@JaroslavTulach JaroslavTulach Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  • 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?

"jvm",
"Setting this flag runs Enso in JVM mode rather than the default native one.",
showInUsage = true
"path",
"Runs Enso in JVM mode rather than the default native one.",
true
)
private def systemJVMOverride =
Opts.flag(
Expand Down Expand Up @@ -166,7 +167,7 @@ object LauncherApplication {
engineLogLevel,
systemJVMOverride,
jvmOpts,
jvmMode,
jvm,
additionalArgs
) mapN {
(
Expand All @@ -175,15 +176,15 @@ object LauncherApplication {
engineLogLevel,
systemJVMOverride,
jvmOpts,
jvmMode,
jvm,
additionalArgs
) => (config: Config) =>
Launcher(config).runRun(
path = path,
versionOverride = versionOverride,
useSystemJVM = systemJVMOverride,
jvmOpts = jvmOpts,
jvmMode = jvmMode,
jvm = Option(jvm),
Copy link
Member Author

@JaroslavTulach JaroslavTulach Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  • 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

  1. --jvm with nothing after it - to enable JVM mode but no path to the JVM
  2. --jvm <path> with a path to the JVM that should be used

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

  1. do not specify --jvm at all
  2. 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?

Copy link
Member Author

@JaroslavTulach JaroslavTulach Jun 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

  • 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...

additionalArguments = additionalArgs,
logLevel = engineLogLevel
)
Expand Down Expand Up @@ -255,7 +256,7 @@ object LauncherApplication {
engineLogLevel,
systemJVMOverride,
jvmOpts,
jvmMode,
jvm,
additionalArgs
) mapN {
(
Expand All @@ -271,7 +272,7 @@ object LauncherApplication {
engineLogLevel,
systemJVMOverride,
jvmOpts,
jvmMode,
jvm,
additionalArgs
) => (config: Config) =>
Launcher(config).runLanguageServer(
Expand All @@ -283,7 +284,7 @@ object LauncherApplication {
secureRpcPort = secureRpcPort,
dataPort = dataPort,
secureDataPort = secureDataPort,
jvmModeEnabled = jvmMode
jvm = Option(jvm)
),
contentRoot = path,
versionOverride = versionOverride,
Expand Down Expand Up @@ -316,7 +317,7 @@ object LauncherApplication {
engineLogLevel,
systemJVMOverride,
jvmOpts,
jvmMode,
jvm,
additionalArgs
) mapN {
(
Expand All @@ -325,15 +326,15 @@ object LauncherApplication {
engineLogLevel,
systemJVMOverride,
jvmOpts,
jvmMode,
jvm,
additionalArgs
) => (config: Config) =>
Launcher(config).runRepl(
projectPath = path,
versionOverride = versionOverride,
useSystemJVM = systemJVMOverride,
jvmOpts = jvmOpts,
jvmMode = jvmMode,
jvm = Option(jvm),
additionalArguments = additionalArgs,
logLevel = engineLogLevel
)
Expand Down
14 changes: 7 additions & 7 deletions engine/launcher/src/main/scala/org/enso/launcher/components/LauncherRunner.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class LauncherRunner(
versionOverride: Option[SemVer],
logLevel: Level,
logMasking: Boolean,
jvmMode: Boolean,
jvm: Option[Option[Path]],
additionalArguments: Seq[String]
): Try[RunSettings] =
Try {
Expand All @@ -68,7 +68,7 @@ class LauncherRunner(
}
RunSettings(
version,
jvmMode,
jvm,
arguments ++ setLogLevelArgs(logLevel, logMasking)
++ additionalArguments,
workingDirectory = workingDirectory,
Expand All @@ -85,7 +85,7 @@ class LauncherRunner(
versionOverride: Option[SemVer],
logLevel: Level,
logMasking: Boolean,
jvmMode: Boolean,
jvm: Option[Option[Path]],
additionalArguments: Seq[String]
): Try[RunSettings] =
Try {
Expand Down Expand Up @@ -134,7 +134,7 @@ class LauncherRunner(
}
RunSettings(
version,
jvmMode,
jvm,
arguments ++ setLogLevelArgs(logLevel, logMasking)
++ additionalArguments,
workingDirectory = workingDirectory,
Expand Down Expand Up @@ -216,7 +216,7 @@ class LauncherRunner(
(
RunSettings(
version,
jvmMode = false,
jvm = None,
arguments,
workingDirectory = None,
connectLoggerIfAvailable = false
Expand Down Expand Up @@ -267,7 +267,7 @@ class LauncherRunner(
tokenOpts ++ hideProgressOpts
RunSettings(
version,
jvmMode = false,
jvm = None,
arguments ++ setLogLevelArgs(logLevel, logMasking)
++ additionalArguments,
workingDirectory = None,
Expand Down Expand Up @@ -316,7 +316,7 @@ class LauncherRunner(
hideProgressOpts
RunSettings(
version,
jvmMode = false,
jvm = None,
arguments ++ setLogLevelArgs(logLevel, logMasking)
++ additionalArguments,
workingDirectory = None,
Expand Down
30 changes: 15 additions & 15 deletions engine/launcher/src/test/scala/org/enso/launcher/components/LauncherRunnerSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {

val runSettings = RunSettings(
SemVer.of(0, 0, 0),
jvmMode = true,
jvm = Some(None),
Seq("arg1", "--flag2"),
workingDirectory = None,
connectLoggerIfAvailable = true
Expand Down Expand Up @@ -127,7 +127,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
path = projectPath,
name = "ProjectName",
engineVersion = defaultEngineVersion,
jvmMode = false,
jvm = None,
normalizedName = None,
projectTemplate = None,
authorName = Some(authorName),
Expand Down Expand Up @@ -156,7 +156,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
path = projectPath,
name = "ProjectName",
engineVersion = defaultEngineVersion,
jvmMode = false,
jvm = None,
normalizedName = Some(normalizedName),
projectTemplate = None,
authorName = None,
Expand Down Expand Up @@ -187,7 +187,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
path = projectPath,
name = "ProjectName2",
engineVersion = nightlyVersion,
jvmMode = false,
jvm = None,
normalizedName = None,
projectTemplate = None,
authorName = None,
Expand Down Expand Up @@ -221,7 +221,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
additionalArguments = Seq("arg", "--flag"),
logLevel = Level.INFO,
logMasking = true,
jvmMode = false
jvm = None
)
.get

Expand All @@ -248,7 +248,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
additionalArguments = Seq(),
logLevel = Level.INFO,
logMasking = true,
jvmMode = false
jvm = None
)
.get

Expand All @@ -265,7 +265,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
additionalArguments = Seq(),
logLevel = Level.INFO,
logMasking = true,
jvmMode = false
jvm = None
)
.get

Expand All @@ -282,7 +282,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
additionalArguments = Seq(),
logLevel = Level.INFO,
logMasking = true,
jvmMode = false
jvm = None
)
.get

Expand All @@ -306,7 +306,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
secureRpcPort = None,
dataPort = 4321,
secureDataPort = None,
jvmModeEnabled = false
jvm = None
)
val runSettings = runner
.languageServer(
Expand Down Expand Up @@ -359,7 +359,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
additionalArguments = Seq(),
logLevel = Level.INFO,
logMasking = true,
jvmMode = false
jvm = None
)
.get

Expand All @@ -376,7 +376,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
additionalArguments = Seq(),
logLevel = Level.INFO,
logMasking = true,
jvmMode = false
jvm = None
)
.get

Expand All @@ -392,7 +392,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
additionalArguments = Seq(),
logLevel = Level.INFO,
logMasking = true,
jvmMode = false
jvm = None
)
.get

Expand All @@ -408,7 +408,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
additionalArguments = Seq(),
logLevel = Level.INFO,
logMasking = true,
jvmMode = false
jvm = None
)
.isFailure,
"Running outside project without providing any paths should be an error"
Expand Down Expand Up @@ -436,7 +436,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
additionalArguments = Seq(),
logLevel = Level.INFO,
logMasking = true,
jvmMode = false
jvm = None
)
.get

Expand All @@ -462,7 +462,7 @@ class LauncherRunnerSpec extends RuntimeVersionManagerTest with FlakySpec {
additionalArguments = Seq(),
logLevel = Level.INFO,
logMasking = true,
jvmMode = false
jvm = None
)
.get

Expand Down
Loading
Loading