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

Better detection for Enso's NI when launching Language Server #12034

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

hubertp
Copy link
Collaborator

@hubertp hubertp commented Jan 10, 2025

Pull Request Description

Previosly we were wrongly relying on the presence of the file. That way,
a bash script meant that NI integration was wrongly used.
This change uses Tika, but it has already been present in other
subprojects so no additional dependency shall be added.

Follow up on #11880.

Checklist

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

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

Previosly we were wrongly relying on the presence of the file. That way,
a bash script meant that NI integration was wrongly used.
This change uses Tika, but it has already been present in other
subprojects so no additional dependency shall be added.

Follow up on #11880.
@hubertp hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 10, 2025
@JaroslavTulach
Copy link
Member

Previosly we were wrongly relying on the presence of the file. That way,
a bash script meant that NI integration was wrongly used.

It was probably not too bad to invoke bin/enso shell script. If java is available around, then the shell script launches the JVM and passes its arguments in it - e.g. it might have worked.

} else None
}

private def isBinary(path: Path): Boolean = {
Copy link
Member

Choose a reason for hiding this comment

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

Alternative check would be:

return !Files.readLines("bin/enso").get(0).startsWith("#!/bin/sh");

that is a common prefix of most of the shell scripts. However we'd have to add such header to our shell script:

enso$ git diff
diff --git distribution/bin/enso distribution/bin/enso
index 1939fc99dc..93a45f4a6e 100755
--- distribution/bin/enso
+++ distribution/bin/enso
@@ -1,3 +1,5 @@
+#!/bin/sh
+
 COMP_PATH=$(dirname "$0")/../component
 
 JAVA_OPTS="--add-opens=java.base/java.nio=ALL-UNNAMED $JAVA_OPTS"

but as you say, Tika was already available as it is used somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That wouldn't work on Windows

Copy link
Member

Choose a reason for hiding this comment

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

But on windows the difference is in extension:

  • enso.bat - is the shell script
  • enso.exe - is the native binary

We only need the content file check for Unixes.

"org.scalatest" %% "scalatest" % scalatestVersion % Test
),
Compile / moduleDependencies ++= Seq(
"org.apache.commons" % "commons-compress" % commonsCompressVersion,
"org.apache.tika" % "tika-core" % tikaVersion,
Copy link
Member

Choose a reason for hiding this comment

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

tika is added as a moduleDependency, but module-info.java for runtime-version-manager is not changed? How is it possible that this works? There should be something like requires org.apache.tika added to the module descriptor.

It compiles because you are using tika in Scala source, but my gut feeling is that it might crash in runtime at some point on NoClassDefFound.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that the new functionality in NativeExecCommand is tested?

Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

Ensure that NativeExecCommand is tested. I strongly doubt this will not crash with NoClassDefFoundError when no module descriptor was changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants