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.

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

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
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3bde413
Better detection for Enso's NI when launching LS
hubertp Jan 10, 2025
cdcd4e8
legal
hubertp Jan 10, 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
Next Next commit
Better detection for Enso's NI when launching LS 
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.
  • Loading branch information
@hubertp
hubertp committed Jan 10, 2025
commit 3bde413da72139f4cca2dff323d9f173cb36bfe0
2 changes: 2 additions & 0 deletions build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4495,10 +4495,12 @@ lazy val `runtime-version-manager` = project
libraryDependencies ++= Seq(
"com.typesafe.scala-logging" %% "scala-logging" % scalaLoggingVersion,
"org.apache.commons" % "commons-compress" % commonsCompressVersion,
"org.apache.tika" % "tika-core" % tikaVersion,
"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
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it works.

"org.slf4j" % "slf4j-api" % slf4jVersion
),
Compile / internalModuleDependencies := Seq(
Expand Down
22 changes: 20 additions & 2 deletions ...sion-manager/src/main/scala/org/enso/runtimeversionmanager/runner/NativeExecCommand.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import org.enso.cli.OS
import org.enso.distribution.{DistributionManager, Environment}
import org.enso.runtimeversionmanager.components.Engine

import org.apache.tika.config.TikaConfig
import org.apache.tika.Tika

import java.nio.file.Path

case class NativeExecCommand(executablePath: Path) extends ExecCommand {
Expand All @@ -26,7 +29,22 @@ object NativeExecCommand {
val fullExecPath =
dm.paths.engines.resolve(version).resolve("bin").resolve(execName)

if (fullExecPath.toFile.exists()) Some(NativeExecCommand(fullExecPath))
else None
if (fullExecPath.toFile.exists() && isBinary(fullExecPath)) {
Some(NativeExecCommand(fullExecPath))
} 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.

try {
val config = TikaConfig.getDefaultConfig()
val tika = new Tika(config)
val mimeTypes = config.getMimeRepository
val mime = tika.detect(path);
val tpe = mimeTypes.forName(mime).getType.getType
tpe != null && tpe == "application"
} catch {
case _: Throwable =>
false
}
}
}