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

Merged
merged 6 commits into from
Feb 4, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Feb 2, 2024

Closes #8601

Pull Request Description

This PR provides following fixes:

  • speed runEngineDistribution up by proper time stampingruntime.jar and avoiding it to be re-built with every execution
  • allow one to pass a textual filter to test/Base_Tests
  • includes Numbers_Spec in the run

As a result one can use following command to execute whole Integers group from Numbers_Spec:

$ runEngineDistribution --run test/Base_Tests Integers
45 tests succeeded.

appropriately use runEngineDistribution --run test/Base_Tests Floats to execute floats, and so on, so on.

Checklist

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

  • All code follows the
    Scala,
    style guides.
  • All code has been tested:
    • Unit tests have been added to test suite.

Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Looks good.
Please do Table_Tests as well.

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.

@@ -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:

@Akirathan
Copy link
Member

@JaroslavTulach @radeusgd Idea for passing filters as cmd-line args:

main group_filter=Nothing spec_filter=Nothing =
    convert_filter filter =
        case filter of
            _ : Text ->
                Regex.compile filter
            Nothing -> Nothing
    IO.println <| "Group_Filter = " + (group_filter |> convert_filter _ |> _.to_text)
    IO.println <| "Spec_Filter = " + (spec_filter |> convert_filter _ |> _.to_text)

When I run the aforementioned with enso --run tmp.enso one two, it results in:

Group_Filter = (Regex.Value False TRegexObject{source=/one/usg})
Spec_Filter = (Regex.Value False TRegexObject{source=/two/usg})

The only "problem" is when you want to run only some test spec and you don't care about the group name. In that case, you should write something like this:

enso --run tmp.enso '.*' "My test spec name"

What are your thoughts? This is simple and it works. If you are OK with that, we can just replace all the main methods in our tests with the one above.

@JaroslavTulach JaroslavTulach added CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Keep up to date Automatically update this PR to the latest develop. labels Feb 4, 2024
@JaroslavTulach
Copy link
Member Author

What are your thoughts?

I believe the CLI option shall be simple. E.g. user should be able to specify a single word argument (not two arguments) and it should do the trick.

The search should be heuristic - e.g. alá google search, not an SQL query. Requiring two arguments like in

$ enso --run tmp.enso '.*' "My test spec name"

example just feels too rationalistic and complicated. E.g. I'd prefer pragmatism over rationalism in this CLI interface.

@JaroslavTulach JaroslavTulach merged commit 2028d6b into develop Feb 4, 2024
25 of 26 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/BetterTestBaseTests branch February 4, 2024 10:39
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. CI: Keep up to date Automatically update this PR to the latest develop. 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.

Do not reassemble Uber jars when no changes occurred
6 participants