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 runEngineDistribution --run test/Base_Tests "filter" #8940

Merged
merged 6 commits into from
Feb 4, 2024
Merged

Better runEngineDistribution --run test/Base_Tests "filter" #8940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
576c851
Speed runEngineDistribution up by caching runtime.jar
JaroslavTulach Feb 2, 2024
0a5b83f
Allow passing a filter test/to Base_Tests execution
JaroslavTulach Feb 2, 2024
68e03c2
Adding forgotten Numbers_Spec
JaroslavTulach Feb 2, 2024
ed80614
Pass filter also to Table_Tests
JaroslavTulach Feb 2, 2024
a1cbcd6
Remove unused JPMSUtils.removeAllModuleInfoExcept merge strategy
Akirathan Feb 2, 2024
ddd821f
Merge branch 'develop' into wip/jtulach/BetterTestBaseTests
mergify[bot] Feb 4, 2024
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
2 changes: 1 addition & 1 deletion build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2019,7 +2019,7 @@ lazy val `runtime-fat-jar` =
case PathList("META-INF", "services", xs @ _*) =>
MergeStrategy.concat
case PathList(xs @ _*) if xs.last.contains("module-info") =>
JPMSUtils.removeAllModuleInfoExcept("runtime")
MergeStrategy.preferProject
Copy link
Member

Choose a reason for hiding this comment

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

According to the docs in sbt-assembly: https://github.com/sbt/sbt-assembly/blob/develop/src/main/scala/sbtassembly/MergeStrategy.scala#L82-L85 preferProject picks the dependency that is the first project/internal dependency.

The goal of JPMSUtils.removeAllModuleInfoExcept("runtime") is to drop all the module-info.class files from the resulting JAR except for a single one from the runtime project. I tried locally this change, and the JAR resulting from runtime-fat-jar/assembly unfortunately still contains more module-info.class files:

$ find . -name '*module-info*'
./META-INF/versions/9/module-info.class
./module-info.class
$ find . -name '*module-info*' | xargs javap
module [email protected] {
  requires java.base;
  exports com.fasterxml.jackson.core;
  exports com.fasterxml.jackson.core.async;
  exports com.fasterxml.jackson.core.base;
  exports com.fasterxml.jackson.core.exc;
  exports com.fasterxml.jackson.core.filter;
  exports com.fasterxml.jackson.core.format;
  exports com.fasterxml.jackson.core.io;
  exports com.fasterxml.jackson.core.json;
  exports com.fasterxml.jackson.core.json.async;
  exports com.fasterxml.jackson.core.sym;
  exports com.fasterxml.jackson.core.type;
  exports com.fasterxml.jackson.core.util;
  uses com.fasterxml.jackson.core.ObjectCodec;
}
Compiled from "module-info.java"
open module org.enso.runtime {
  requires java.base;
  requires java.compiler;
  requires java.desktop;
  requires java.se;
  requires jdk.unsupported;
  requires org.graalvm.polyglot;
  requires org.graalvm.truffle;
  requires static org.slf4j;
  uses org.slf4j.spi.SLF4JServiceProvider;
  uses org.enso.interpreter.instrument.HandlerFactory;
  provides  com.oracle.truffle.api.provider.TruffleLanguageProvider with
    org.enso.interpreter.EnsoLanguageProvider,
    org.enso.interpreter.epb.EpbLanguageProvider;
  provides  com.oracle.truffle.api.instrumentation.provider.TruffleInstrumentProvider with
    org.enso.interpreter.instrument.ReplDebuggerInstrumentProvider,
    org.enso.interpreter.instrument.RuntimeServerInstrumentProvider,
    org.enso.interpreter.instrument.IdExecutionInstrumentProvider;
}

If the JVM does not complain about it, we might just leave your change and remove JPMSUtils.removeAllInfoExcept merge strategy.

Copy link
Member

Choose a reason for hiding this comment

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

@JaroslavTulach What is your opinion on the following:

$ jar --validate -f runtime.jar
module-info.class in a versioned directory contains incorrect name
module-info.class in a versioned directory contains missing "requires"
module-info.class in a versioned directory contains missing "requires"
module-info.class in a versioned directory contains missing "requires"
module-info.class in a versioned directory contains different "exports"
module-info.class in a versioned directory contains different "provides"
META-INF/versions/9/module-info.class: module-info.class in a versioned directory contains different "version"
entry: META-INF/versions/11/com/fasterxml/jackson/core/io/doubleparser/FastDoubleSwar.class, contains a class with different api from earlier version
entry: META-INF/versions/17/com/fasterxml/jackson/core/io/doubleparser/FastDoubleSwar.class, contains a class with different api from earlier version
entry: META-INF/versions/19/com/fasterxml/jackson/core/io/doubleparser/FastDoubleSwar.class, contains a class with different api from earlier version

?

Copy link
Member

Choose a reason for hiding this comment

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

Removed the, now unused, custom merge strategy in a1cbcd6. The tests, and my local brief experimentation, seems to be OK with that. If everything works, this change fixes #8601

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for your review and a1cbcd6 fix. I am trying to build runtime.jar from scratch and verify your observations. Doing:

enso$ sbt buildEngineDistribution
enso$ unzip -v runtime.jar | grep module-info
     698  Defl:N      275  61% 2010-01-01 00:00 55091239  META-INF/versions/9/module-info.class
     902  Defl:N      452  50% 2010-01-01 00:00 0dada4e7  module-info.class

I have found two module-info.class files. After 9a233d2cf2 I see only single module info:

enso$ unzip -v runtime.jar | grep module-info
     902  Defl:N      452  50% 2010-01-01 00:00 0dada4e7  module-info.class
enso$ jar --validate -f runtime.jar 
entry: META-INF/versions/11/com/fasterxml/jackson/core/io/doubleparser/FastDoubleSwar.class, contains a class with different api from earlier version
entry: META-INF/versions/17/com/fasterxml/jackson/core/io/doubleparser/FastDoubleSwar.class, contains a class with different api from earlier version
entry: META-INF/versions/19/com/fasterxml/jackson/core/io/doubleparser/FastDoubleSwar.class, contains a class with different api from earlier version

that seems OKeyish. The module info itself looks fine as well:

enso$ unzip runtime.jar module-info.class
enso$ javap module-info.class 
Compiled from "module-info.java"
open module org.enso.runtime {
  requires java.base;
  requires java.compiler;
  requires java.desktop;
  requires java.se;
  requires jdk.unsupported;
  requires org.graalvm.polyglot;
  requires org.graalvm.truffle;
  requires static org.slf4j;
  uses org.slf4j.spi.SLF4JServiceProvider;
  uses org.enso.interpreter.instrument.HandlerFactory;
  provides  com.oracle.truffle.api.provider.TruffleLanguageProvider with
    org.enso.interpreter.EnsoLanguageProvider,
    org.enso.interpreter.epb.EpbLanguageProvider;
  provides  com.oracle.truffle.api.instrumentation.provider.TruffleInstrumentProvider with
    org.enso.interpreter.instrument.ReplDebuggerInstrumentProvider,
    org.enso.interpreter.instrument.RuntimeServerInstrumentProvider,
    org.enso.interpreter.instrument.IdExecutionInstrumentProvider;
}

I guess this is integratable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alas, my git push failed on conflicts and I merged incomplete PR. Sorry for that. Here is a continuation:

case _ => MergeStrategy.first
}
)
Expand Down
31 changes: 0 additions & 31 deletions project/JPMSUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,37 +127,6 @@ object JPMSUtils {
truffleRelatedArtifacts
}

/** There may be multiple module-info.class files comming from different
* dependencies. We care only about the one from the `runtime` project.
* The following merge strategy ensures that all other module-info.class
* files are discarded from the resulting uber Jar.
*
* @param projName Project name for which the module-info.class is retained.
*/
def removeAllModuleInfoExcept(
projName: String
): MergeStrategy = {
CustomMergeStrategy(
strategyName =
s"Discard all module-info except for module-info from project $projName",
notifyIfGTE = 1
) { conflictingDeps: Vector[Dependency] =>
val runtimeModuleInfoOpt = conflictingDeps.collectFirst {
case project @ Project(name, _, _, stream) if name == projName =>
project
}
runtimeModuleInfoOpt match {
case Some(runtimeModuleInfo) =>
Right(
Vector(
JarEntry(runtimeModuleInfo.target, runtimeModuleInfo.stream)
)
)
case None => Right(Vector())
}
}
}

/** Compiles a single `module-info.java` source file with the default java compiler (
* the one that is defined for the project). Before the module-info is compiled, all the
* class files from `scopeFilter` are copied into the `target` directory of the current project.
Expand Down
6 changes: 4 additions & 2 deletions test/Base_Tests/src/Main.enso
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ import project.System.Temporary_File_Spec

import project.Random_Spec

main =
main filter=Nothing =
suite = Test.build suite_builder->
Any_Spec.add_specs suite_builder
Array_Spec.add_specs suite_builder
Expand Down Expand Up @@ -126,6 +126,7 @@ main =
Instrumentor_Spec.add_specs suite_builder
Meta_Location_Spec.add_specs suite_builder
Names_Spec.add_specs suite_builder
Numbers_Spec.add_specs suite_builder
Equals_Spec.add_specs suite_builder
Ordering_Spec.add_specs suite_builder
Comparator_Spec.add_specs suite_builder
Expand Down Expand Up @@ -165,4 +166,5 @@ main =
System_Spec.add_specs suite_builder
Random_Spec.add_specs suite_builder
XML_Spec.add_specs suite_builder
suite.run_with_filter

suite.run_with_filter filter
5 changes: 3 additions & 2 deletions test/Table_Tests/src/Main.enso
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@ import project.Helpers
import project.In_Memory
import project.IO

main =
main filter=Nothing =
suite = Test.build suite_builder->
In_Memory.Main.add_specs suite_builder
IO.Main.add_specs suite_builder
Formatting.Main.add_specs suite_builder
Database.Main.add_specs suite_builder
Helpers.Main.add_specs suite_builder
suite.run_with_filter

suite.run_with_filter filter
Copy link
Member Author

Choose a reason for hiding this comment

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

Please do Table_Tests as well.

Done in ed80614, however as @Akirathan pointed out, this only filters by group name. A simple, standardized mechanism to turn argument of main function into useful filter is needed.

Loading