Skip to content

Make Native Image opt-out #12515

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

Merged
merged 10 commits into from
Mar 17, 2025
Merged

Make Native Image opt-out #12515

merged 10 commits into from
Mar 17, 2025

Conversation

hubertp
Copy link
Collaborator

@hubertp hubertp commented Mar 16, 2025

Pull Request Description

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.

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,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.

This change makes Native Image build opt-out rather than opt-in.
To use JVM mode, IDE has to start with `ENSO_LAUNCHER=shell`.
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

  • ENSO_LAUNCHER would be a new user facing API
  • it has to be documented
  • meaning of ENSO_JVM_OPTS and ENSO_LAUNCHER combination is not clear
  • such a combination has to be documented/tested should ENSO_LAUNCHER environment variable really become support API for the next release

@JaroslavTulach
Copy link
Member

According to @jdunkerley some users are using ENSO_JVM_OPTS already:

  • this PR would break their workflows
    • very likely the project-manager would not be able to invoke the engine at all
    • failing with uncomprehensible failure
  • it changes the meaning of ENSO_JVM_OPTS
  • contary to Use NI mode by default unless an env variable is set #12501 which
    • attempts to keep compatiblity
    • keeps meaning of ENSO_JVM_OPTS unchanged

@hubertp
Copy link
Collaborator Author

hubertp commented Mar 17, 2025

I agree that ENSO_LAUNCHER is not the best way. Since the other PR got no-merge, I will update this PR so that project-manager/IDE accepts --jvm instead. That should be fairly non-controversial and easy to add.

@JaroslavTulach
Copy link
Member

As the one who argued in favor of --jvm in the past, I shall support the project-manager --jvm approach....

I will update this PR so that project-manager/IDE accepts --jvm instead. That should be fairly non-controversial and easy to add.

...and I will!

  • project-manager --jvm option is consistent with enso --jvm
  • we wanted such a consistency and we still want it - e.g. it seems like a good solution
  • --jvm focuses on the core issues of today: opt-out from native image mode
  • we leave the memory settings & co. options for power users via ENSO_JVM_OPTS for now

One question:

  • how does one pass --jvm option to AppImage?

@hubertp
Copy link
Collaborator Author

hubertp commented Mar 17, 2025

  • how does one pass --jvm option to AppImage?

I'm already on it.

`--jvm` is passed to project manager and enables JVM mode.
Copy link

github-actions bot commented Mar 17, 2025

🧪 Storybook is successfully deployed!

📊 Dashboard:

@hubertp hubertp added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Mar 17, 2025
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

  • I like how the PR is shaping with --jvm argument
  • I still believe context.rs change shouldn't be part of this PR

One Google_Api_Test is known to be outdated/broken. Increased timeout
for building index - Windows appears to have problems.
@hubertp
Copy link
Collaborator Author

hubertp commented Mar 17, 2025

Ready for review, folks.

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

With 05329f9 it all seems fine to me.

Copy link
Contributor

@somebody1234 somebody1234 left a comment

Choose a reason for hiding this comment

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

approving changes in app/ide-desktop

@@ -276,7 +276,7 @@ object DistributionPackage {
"JAVA_OPTS" -> "-Dorg.jline.terminal.dumb=true"
).run
// Poor man's solution to stuck index generation
val GENERATING_INDEX_TIMEOUT = 60 * 2 // 2 minutes
val GENERATING_INDEX_TIMEOUT = 60 * 4 // 2 minutes
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot to cleanup or

Suggested change
val GENERATING_INDEX_TIMEOUT = 60 * 4 // 2 minutes
val GENERATING_INDEX_TIMEOUT = 60 * 4 // 4 minutes

Copy link
Contributor

Choose a reason for hiding this comment

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

in ideal world

scala> {
     | import scala.concurrent.duration._
     | 4.minutes.toSeconds
     | }
val res4: Long = 240

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, I will add it to next PR. Didn't want to trigger yet another build, it would take ages and this needed to get it to nightly.

@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Mar 17, 2025
@mergify mergify bot merged commit c47dba1 into develop Mar 17, 2025
81 of 83 checks passed
@mergify mergify bot deleted the wip/hubert/12483-ni-opt-out branch March 17, 2025 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Native Image opt-out
6 participants