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

Verify ./runner executable on all supported OSes #10329

Merged
merged 4 commits into from
Jun 25, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jun 21, 2024

Pull Request Description

Follow up to #10126. If we want to use the ./runner executable as the default launcher, we need to make sure it builds and runs on all operating systems we support.

Checklist

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

  • All code follows the style guides
  • All CI checks continue to pass

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 21, 2024
@JaroslavTulach JaroslavTulach self-assigned this Jun 21, 2024
@JaroslavTulach
Copy link
Member Author

I guess all the Base_Tests continue to run without any error.

@JaroslavTulach JaroslavTulach changed the title Persistance classes shall be preloaded at NI build time Verify ./runner executable on all supported OSes Jun 21, 2024
@JaroslavTulach JaroslavTulach linked an issue Jun 21, 2024 that may be closed by this pull request
@@ -1800,458 +1800,6 @@
"name":"org.enso.base_test_helpers.RangeStream",
"methods":[{"name":"<init>","parameterTypes":["int", "int"] }]
},
{
Copy link
Member Author

Choose a reason for hiding this comment

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

Additional NI related fix:

Persistance discovers all the available implementations via META-INF/services/ lookup. That would require reflective access to constructor. However such a discovery is now only performed in static initializer of the class - e.g. during build time of Native image. Hence no reflective operations with Persistance subclasses shall be performed during native executable execution. Removing persistance related debris from the reflection configuration file.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Jun 21, 2024

Both issues shall be solved by 71b20a7

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Jun 21, 2024

The windows job has succeeded in 14min. The Mac OS X job succeeded as well but it took one hour. Obviously Linux job was successful in 23min. We could make these numbers lower by removing the NI+Espresso build - that one is highly experimental and needs only to be executed on a single platform.

Update after 1b0d22e that removed Espresso+NI check on Mac and Windows we get :

  • Linux runs for 23min (still with Espresso+NI check)
  • Mac finished in 46min
  • Windows tool 12min

"--add-modules",
s"$NI_MODULES,$JDK_MODULES,$DEBUG_MODULES,$PYTHON_MODULES"
)
val exitCode = scala.sys.process.Process(jlink.toString(), jlinkArgs).!
Copy link
Member Author

Choose a reason for hiding this comment

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

@jdunkerley reports https:/enso-org/enso/actions/runs/9184711886/job/26530345747 failure on Mac Arm. It looks like a space in path issue and it should be fixed by this change.

@@ -433,9 +433,9 @@ impl Processor {
arg::backend::Command::CiCheck {} => {
let config = enso_build::engine::BuildConfigurationFlags {
build_benchmarks: true,
// Windows is not yet supported for the native runner.
build_native_runner: enso_build::ci::big_memory_machine()
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 wondering why we build_native_runner on Mac Arm? Probably the Mac Arm classifies as big_memory_machine...

@JaroslavTulach JaroslavTulach merged commit a5af0c2 into develop Jun 25, 2024
41 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/PersistWithReflection10126 branch June 25, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Run whole Standard.Base in Native Image ./runner
5 participants