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

Native Image run includes SPI for libraries that are not loaded, leading to crashes #11707

Closed
radeusgd opened this issue Nov 29, 2024 · 1 comment · Fixed by #11722
Closed
Assignees
Labels

Comments

@radeusgd
Copy link
Member

radeusgd commented Nov 29, 2024

This has been uncovered during work on #11546 where I have tried 'fixing' the issue that Base_Tests used to depend on Standard.Table.

Apparently, when I run Base_Tests under Native Image without the Table import, the FileSystemSPI still sees the registrations for file formats from Standard.Table. However, since the Standard.Table is no longer loaded, trying to instantiate the modules related to these formats fails, causing our tests to fail.

To make the tests pass I've re-introduced the Standard.Table import to Base_Tests, but overall we should get rid of it.

To reproduce the issue, remove the workaround that has been added in #11546 by applying the following patch:

Subject: [PATCH] workaround for https://github.com/enso-org/enso/issues/11707
---
Index: test/Base_Tests/src/System/File_Read_Spec.enso
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/Base_Tests/src/System/File_Read_Spec.enso b/test/Base_Tests/src/System/File_Read_Spec.enso
--- a/test/Base_Tests/src/System/File_Read_Spec.enso	(revision a600180b634861d96972366c40c281251ded151b)
+++ b/test/Base_Tests/src/System/File_Read_Spec.enso	(date 1732878710671)
@@ -106,11 +106,8 @@
             r2.should_be_a Vector
             r2.should_equal [js_as_text.get, "Hello World!"]
 
-        ## Workaround for https://github.com/enso-org/enso/issues/11707
-           This pending check should be removed once it is fixed.
-        is_table_imported = File_Format.all.map .to_text . contains "Delimited_Format"
-        table_import_pending = if is_table_imported then "Base_Tests should not import Table, but they sometimes do as workaround for #11707. This test can only run if Table is not imported."
-        group_builder.specify "would default to returning as merged Table, but will raise a helpful error if Standard.Table is not loaded" pending=table_import_pending <|
+        group_builder.specify "would default to returning as merged Table, but will raise a helpful error if Standard.Table is not loaded" <|
+            is_table_imported = File_Format.all.map .to_text . contains "Delimited_Format"
             Runtime.assert is_table_imported.not "This test assumes that Base_Tests does not import Standard.Table."
             files = [enso_project.data / "sample.json"]
             r1 = Data.read_many files
Index: test/Base_Tests/src/Main.enso
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/Base_Tests/src/Main.enso b/test/Base_Tests/src/Main.enso
--- a/test/Base_Tests/src/Main.enso	(revision a600180b634861d96972366c40c281251ded151b)
+++ b/test/Base_Tests/src/Main.enso	(date 1732878715460)
@@ -93,10 +93,6 @@
 
 import project.Random_Spec
 
-## Workaround for bug https://github.com/enso-org/enso/issues/11707
-   The Standard.Table import should be removed once the bug is fixed.
-import Standard.Table
-
 
 main filter=Nothing =
     suite = Test.build suite_builder->

Then you need to build the Native Image runner (sbt engine-runner/buildNativeImage) and run it. A small test that you can check out is e.g.:

.\built-distribution\enso-engine-0.0.0-dev-windows-amd64\enso-0.0.0-dev\bin\enso.exe --run .\test\Base_Tests\ -- "JSON Deser"
@radeusgd
Copy link
Member Author

Note that the issue is further obscured by an UnknownIdentifierException being intercepted by Truffle code and mistakenly wrapping it in another error that suggests that get_module is the missing identifier, whereas in fact the missing identifier is a module that fails to be loaded, e.g. Delimited_Format that comes from Standard.Table.

@JaroslavTulach suggested that we should change the type of exception raised to avoid obscuring the real problem:

Index: engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/TopLevelScope.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/TopLevelScope.java b/engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/TopLevelScope.java
--- a/engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/TopLevelScope.java	(revision a600180b634861d96972366c40c281251ded151b)
+++ b/engine/runtime/src/main/java/org/enso/interpreter/runtime/scope/TopLevelScope.java	(date 1732879071797)
@@ -22,6 +22,7 @@
 import org.enso.interpreter.runtime.builtin.Builtins;
 import org.enso.interpreter.runtime.data.EnsoObject;
 import org.enso.interpreter.runtime.data.vector.ArrayLikeHelpers;
+import org.enso.interpreter.runtime.error.PanicException;
 import org.enso.interpreter.runtime.type.Types;
 import org.enso.pkg.Package;
 import org.enso.pkg.QualifiedName;
@@ -125,12 +126,12 @@
   abstract static class InvokeMember {
     @CompilerDirectives.TruffleBoundary
     private static Module getModule(TopLevelScope scope, Object[] arguments)
-        throws ArityException, UnsupportedTypeException, UnknownIdentifierException {
+        throws ArityException, UnsupportedTypeException {
       String moduleName = Types.extractArguments(arguments, String.class);
 
       var module = scope.getModule(moduleName);
       if (module.isEmpty()) {
-        throw UnknownIdentifierException.create(moduleName);
+        throw new PanicException(scope.builtins.error().makeModuleDoesNotExistError(moduleName), null);
       }
 
       return module.get();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🟢 Accepted
Development

Successfully merging a pull request may close this issue.

1 participant