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

Package libraries Java code as Java Modules #10714

Draft
wants to merge 49 commits into
base: develop
Choose a base branch
from

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jul 30, 2024

Pull Request Description

Implements #7082 by creating a module layer per library. Removes the obsolete add_to_class_path concept.

Important Notes

Checklist

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

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • 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.
  • Unit tests have been written where possible.
  • Report decent exception of layer resolution problem

@JaroslavTulach JaroslavTulach linked an issue Jul 30, 2024 that may be closed by this pull request
@JaroslavTulach
Copy link
Member Author

Question for @radeusgd - we seem to get the encapsulation with all the consequences. There is dozen of failures:

I can think of ways to fix the tests, but some failures show intended cross-library usage we don't have any alternative yet.

@radeusgd
Copy link
Member

Question for @radeusgd - we seem to get the encapsulation with all the consequences. There is dozen of failures:

I can think of ways to fix the tests, but some failures show intended cross-library usage we don't have any alternative yet.

I imagine we can come up with some workarounds, although that may not be trivial. I'm worried the most about the read autodetection - does it look like the classpath separation breaks the SPI? How can we fix that?

Alternatively, as suggested in #7082 (title Re-exporting), could we provide a way for a library to opt-in to being able to access modules of another lib? e.g. Standard.Table asking to be able to user Java helpers of Standard.Base, e.g. by adding some entry in its package.yaml? That would still prevent users from accessing the internals (unless they also add such an override) while allowing us to more easily share utilities between our libraries.

@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jul 30, 2024
@JaroslavTulach
Copy link
Member Author

I can think of ways to fix the tests

My plan was:

enso$ git diff distribution/ test/Base_Tests/src/Data/Text/Utils_Spec.enso
diff --git distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Extra_Imports.enso distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Extra_Imports.enso
index 7771401cf4..c42ee36ec6 100644
--- distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Extra_Imports.enso
+++ distribution/lib/Standard/Base/0.0.0-dev/src/Internal/Extra_Imports.enso
@@ -9,7 +9,7 @@ private
 polyglot java import java.io.PrintStream
 
 # needed by Util_Spec
-polyglot java import org.enso.base.text.CaseFoldedString
+polyglot java import org.enso.base.text.CaseFoldedString as JCaseFoldedString
 polyglot java import org.enso.base.text.CaseFoldedString.Grapheme
 
 # needed by Comparator_Spec:
@@ -29,3 +29,5 @@ polyglot java import java.util.function.Function
 polyglot java import java.lang.Thread
 polyglot java import java.lang.Thread.State
 polyglot java import java.lang.Float
+
+CaseFoldedString = JCaseFoldedString
diff --git test/Base_Tests/src/Data/Text/Utils_Spec.enso test/Base_Tests/src/Data/Text/Utils_Spec.enso
index d0b987bea2..7d8d006ff7 100644
--- test/Base_Tests/src/Data/Text/Utils_Spec.enso
+++ test/Base_Tests/src/Data/Text/Utils_Spec.enso
@@ -1,7 +1,7 @@
 from Standard.Base import all
 
 polyglot java import org.enso.base.Text_Utils
-polyglot java import org.enso.base.text.CaseFoldedString
+import Standard.Base.Internal.Extra_Imports.CaseFoldedString
 polyglot java import com.ibm.icu.text.BreakIterator
 
 from Standard.Test import all

but that requires us to run with --disable-private-check. I can add such option, but then I am getting test failures at

[FAILED] Private constructors: [6/11, 26ms]
  | => enso / runEngineDistribution 111s
    - [FAILED] cannot directly call private constructor [4ms]
        Reason: Expected a Panic Private_Access to be thrown, but the action succeeded and returned [(Cons 42)] (at test/Base_Tests/src/Semantic/Private_Spec.enso:18:13-65).
    - [FAILED] can get reference to private constructor, but cannot call it [2ms]
        Reason: Expected a Panic Private_Access to be thrown, but the action succeeded and returned [(Cons 42)] (at test/Base_Tests/src/Semantic/Private_Spec.enso:27:13-58).
    - [FAILED] cannot get private field [2ms]
        Reason: Expected a Panic Private_Access to be thrown, but the action succeeded and returned [42] (at test/Base_Tests/src/Semantic/Private_Spec.enso:35:13-56).
    - [FAILED] cannot call private constructor from Java [3ms]
        Reason: Expected a Panic Private_Access to be thrown, but the action succeeded and returned [(Cons 42)] (at test/Base_Tests/src/Semantic/Private_Spec.enso:59:13-88).
    - [FAILED] cannot call private constructor from a lambda method [3ms]
        Reason: Expected a Panic Private_Access to be thrown, but the action succeeded and returned [(Cons 42)] (at test/Base_Tests/src/Semantic/Private_Spec.enso:62-64).

[FAILED] Private methods: [2/5, 9ms]

I guess we cannot have both: Run with --disable-private-check and these Private tests.

@JaroslavTulach
Copy link
Member Author

The next test failure to solve - Module org.enso.std.base reads more than one module named com.ibm.icu

Copy link
Member

@Akirathan Akirathan Sep 27, 2024

Choose a reason for hiding this comment

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

This seems like a remnant after merging with develop. On develop, the whole runtime-fat-jar directory was deleted.

@@ -8,3 +8,6 @@ authors:
maintainers:
- name: Enso Team
email: contact@enso.org
requires:
- Standard.Base
Copy link
Member Author

Choose a reason for hiding this comment

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

I am running into issues.

  • if Standard.AWS has its own isolated layer, then it complains that it cannot find org.enso.base.file_system.FileSystemSPI
  • but if I connect Standard.AWS layer with Standard.Base, then it complains Jackson is there twice!

It is hard to have both - isolation and connection between the layers.

Addressed by 405e88d

@@ -8,3 +8,6 @@ authors:
maintainers:
- name: Enso Team
email: contact@enso.org
requires:
- Standard.Base
- Standard.Database
Copy link
Member Author

Choose a reason for hiding this comment

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

With 57e64e0 I am getting the following when running test/AWS_Tests

Execution finished with an error: java.lang.module.ResolutionException: Modules aws.java.sdk.sts and aws.java.sdk.redshift export package com.amazonaws.auth.policy.actions to module software.amazon.awssdk.metrics
        at <java> java.base/java.lang.module.Resolver.resolveFail(Resolver.java:900)
        at <java> java.base/java.lang.module.Resolver.failTwoSuppliers(Resolver.java:814)
        at <java> java.base/java.lang.module.Resolver.checkExportSuppliers(Resolver.java:735)
        at <java> java.base/java.lang.module.Resolver.finish(Resolver.java:380)
        at <java> java.base/java.lang.module.Configuration.<init>(Configuration.java:139)
        at <java> java.base/java.lang.module.Configuration.resolveAndBind(Configuration.java:493)
        at <java> org.enso.runtime/org.enso.interpreter.runtime.EnsoClassPath.create(EnsoClassPath.java:62)

We are not the only ones seeing this problem: aws/aws-sdk-java#1658 (comment)

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Oct 2, 2024

Currently solving problem with JDBC drivers:

sbt:enso> runEngineDistribution --run test/Table_Tests should.gracefully.handle.columns.from.different.backends
[FAILED] [SQLite In-Memory] (Core_Spec) Table.set: [0/1, 23ms]

    - [FAILED] should gracefully handle columns from different backends [23ms]
        Reason: Expected error Integrity_Error, but error There was an SQL error: No suitable driver found for jdbc:sqlite::memory:. has been returned (at test/Table_Tests/src/Common_Table_Operations/Core_Spec.enso:192:26-60).

The problem is that DriverManager does some fancy checks with Reflection.getCallerClass() and fails because std-base ClassLoader doesn't see drivers from std-database (that's what we wanted!).

Fixed by 3194f6d

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Oct 3, 2024

Most of the Table_Tests are fixed by [today's commits]6992103). There are now three failing tests:

sbt:enso> runEngineDistribution --run test/Table_Tests should.recognise.a.SQLite.database.file|should.recognise.a.sqlite.file.by.extension.for.writing|should.connect.to.a.db.file
[FAILED] SQLite_Format should allow connecting to SQLite files: [0/3, 287ms]

    - [FAILED] should recognise a SQLite database file [166ms]
        Reason: Expected a value of type Standard.Database.Connection.SQLite_Format.SQLite_Format but got a value of type Standard.Base.Nothing.Nothing instead (at /home/devel/NetBeansProjects/enso/enso.ide/test/Table_Tests/src/Database/SQLite_Spec.enso:422:13-80).


    - [FAILED] should recognise a sqlite file by extension for writing [52ms]
        Reason: Expected a value of type Standard.Database.Connection.SQLite_Format.SQLite_Format but got a value of type Standard.Base.Nothing.Nothing instead (at /home/devel/NetBeansProjects/enso/enso.ide/test/Table_Tests/src/Database/SQLite_Spec.enso:425:13-114).


    - [FAILED] should connect to a db file [68ms]
        Reason: An unexpected dataflow error ((Unsupported_Type (File_Format_Metadata.Value '/home/devel/NetBeansProjects/enso/enso.ide/test/Table_Tests/data/transient/sqlite_test.db' 'sqlite_test.db' '.db' File.read_first_bytes[File.enso:732-734] self=org.enso.interpreter.runtime.data.EnsoFile@5248c05a n=_ Nothing))) has been matched (at /home/devel/NetBeansProjects/enso/enso.ide/test/Table_Tests/src/Database/SQLite_Spec.enso:450:13-50).
        at <enso> Auto_Detect.read<arg-1>(/home/devel/NetBeansProjects/enso/enso.ide/built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Base/0.0.0-dev/src/System/File_Format.enso:57:35-80)


0 tests succeeded.
3 tests failed.
0 tests skipped.
0 groups skipped.

When there is a resolution problem, report normal exception, don't crash the JVM:

Execution finished with an error: java.lang.module.FindException: Module org.enso.std.table not found, required by org.enso.test.java_helpers
        at <java> java.base/java.lang.module.Resolver.findFail(Resolver.java:892)
        at <java> java.base/java.lang.module.Resolver.resolve(Resolver.java:192)
        at <java> java.base/java.lang.module.Resolver.resolve(Resolver.java:141)
        at <java> java.base/java.lang.module.Configuration.resolveAndBind(Configuration.java:491)
        at <java> org.enso.runtime/org.enso.interpreter.runtime.EnsoClassPath.create(EnsoClassPath.java:58)
        at <java> org.enso.runtime/org.enso.interpreter.runtime.EnsoContext.findClassPath(EnsoContext.java:497)
        at <java> org.enso.runtime/org.enso.interpreter.runtime.EnsoContext.lookupJavaClass(EnsoContext.java:582)
        at <java> org.enso.runtime/org.enso.interpreter.runtime.IrToTruffle.$anonfun$registerPolyglotImports$2(IrToTruffle.scala:247)
        at <java> org.enso.runtime/org.enso.interpreter.runtime.util.CachingSupplier.get(CachingSupplier.java:48)
        at <java> org.enso.runtime/org.enso.interpreter.runtime.IrToTruffle$ExpressionProcessor.processCaseBranch(IrToTruffle.scala:1761)
        at <java> org.enso.runtime/org.enso.interpreter.runtime.IrToTruffle$ExpressionProcessor.$anonfun$processCase$1(IrToTruffle.scala:1421)
        at <java> scala.library@2.13.11/scala.collection.Iterator$$anon$9.next(Iterator.scala:584)

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encapsulation of polyglot/java Dependencies
3 participants