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

mandrel: update to 23.1.2.0. #48359

Merged
merged 3 commits into from Jan 30, 2024
Merged

Conversation

icp1994
Copy link
Contributor

@icp1994 icp1994 commented Jan 24, 2024

Testing the changes

  • I tested the changes in this PR: YES

Local build testing

  • I built this PR locally for my native architecture: x86_64

One babashka test fail, couldn't figure out why exactly.

cc @leahneukirchen

@@ -59,7 +59,7 @@ post_extract() {
}
Copy link
Member

Choose a reason for hiding this comment

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

This needs change too, to mandrel21

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed that now, but bb -e '(babashka.process/exec "ls")' doesn't work still after the revbump.

Copy link
Member

Choose a reason for hiding this comment

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

We need to fix that before merging then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When building babashka, under --module-path it has

/usr/lib/jvm/mandrel21/lib/svm-preview/builder/svm-foreign.jar:/usr/lib/jvm/mandrel21/lib/jvmci/word.jar:/usr/lib/jvm/mandrel21/lib/jvmci/nativeimage.jar:/usr/lib/jvm/mandrel21/lib/jvmci/collections.jar:/usr/lib/jvm/mandrel21/lib/truffle/truffle-compiler.jar:/usr/lib/jvm/mandrel21/lib/svm/builder/pointsto.jar:/usr/lib/jvm/mandrel21/lib/svm/builder/svm.jar:/usr/lib/jvm/mandrel21/lib/svm/builder/objectfile.jar:/usr/lib/jvm/mandrel21/lib/svm/builder/native-image-base.jar

Isn't that one supposed to have graal-sdk.jar as well?

Copy link
Member

Choose a reason for hiding this comment

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

I tried patching this in but it didn't help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's two build logs for identical babashka versions - bb_old.txt with mandrel-23.0.1.2 JDK 17, bb_new.txt with mandrel-23.1.2.0 JDK 21. Diffing them, the missing graal-sdk.jar in the later's --module-path should be the issue. Maybe a change in graal/mandrel internal across versions? Dunno how to go about debugging that.

Copy link
Member

Choose a reason for hiding this comment

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

So, the difference is that on mandrel 17 this graal-sdk.jar contains lots of things, but on mandrel 21 it just contains some properties. The actual classes are in the svm*jar things.

Babashka exec needs the class org.graalvm.nativeimage.ProcessProperties to exist, which used to be in graal-sdk.jar but is now in jvmci/nativeimage.jar... which is in module-path already tho.

@leahneukirchen
Copy link
Member

This fixes it:

--- a/srcpkgs/babashka/template
+++ b/srcpkgs/babashka/template
@@ -55,7 +55,7 @@ post_extract() {
 
        # Mandrel doesn't have GraalVM-SDK in the default classpath,
        # but some Babashka features require it.
-       vsed -i -e '/:resource-paths/s@]@ "/usr/lib/jvm/mandrel21/lib/jvmci/graal-sdk.jar"]@' project.clj
+       vsed -i -e '/:resource-paths/s@]@ "/usr/lib/jvm/mandrel21/lib/jvmci/nativeimage.jar"]@' project.clj
 }
 
 do_build() {

@icp1994
Copy link
Contributor Author

icp1994 commented Jan 30, 2024

Haha yeah, just finished building that & coming here to say changing that line in resource-path worked 😆

@leahneukirchen
Copy link
Member

Test failures are due to GitHub I think.

@icp1994
Copy link
Contributor Author

icp1994 commented Jan 30, 2024

Failed for me locally too - total five failures

testing me.raynes.core-test

fail in (test162279) (/builddir/babashka-1.3.188/test-resources/lib_tests/me/raynes/core_test.clj:262)
expected: (clojure.core/= (home (system/getproperty "user.name")) env-home)
  actual: (not (clojure.core/= #object[java.io.file 0x66c24010 "/icp"] #object[java.io.file 0x33958938 "/tmp"]))

fail in (expands-to-given-user-162009) (/builddir/babashka-1.3.188/test-resources/lib_tests/me/raynes/core_test.clj:36)
expected: (clojure.core/= (expand-home (str "~" name)) (file user))
  actual: (not (clojure.core/= #object[java.io.file 0x5f3807cd "/icp"] #object[java.io.file 0x1d6ebfaf "/tmp"]))

fail in (expands-to-given-user-162009) (/builddir/babashka-1.3.188/test-resources/lib_tests/me/raynes/core_test.clj:36)
expected: (clojure.core/= (expand-home (format "~%s/foo" name)) (file user "foo"))
  actual: (not (clojure.core/= #object[java.io.file 0x210ec0be "/icp/foo"] #object[java.io.file 0x4f58a0db "/tmp/foo"]))

Ran 40 tests containing 80 assertions.
3 failures, 0 errors.
Testing babashka.process-exec-test
- calling babashka as: ./bb
  - which resolves to: /builddir/babashka-1.3.188/bb
  - babashka v1.3.188
- using exec runner: ["/builddir/babashka-1.3.188/bb" "process/test-native/src/babashka/test_native/run_exec.clj"]
=== exec-failure-to-launch-throws-an-exception-test

=== exec-env-option-test

=== resolves-program-test

=== arg0-mac-and-linux-test


FAIL in (arg0-mac-and-linux-test) (/builddir/babashka-1.3.188/process/test/babashka/process_exec_test.clj:113)
on macOS and Linux, arg0 is supported baseline - not overriden
expected: (= {:args (format "%s %s :ps-me" bb u/wd)} (-> (run-exec (format "%s %s :ps-me" bb u/wd)) :out edn/read-string))
  actual: (not (= {:args "./bb process/script/wd.clj :ps-me"} nil))

FAIL in (arg0-mac-and-linux-test) (/builddir/babashka-1.3.188/process/test/babashka/process_exec_test.clj:113)
on macOS and Linux, arg0 is supported
expected: (= {:args (format "newarg0 %s :ps-me" u/wd)} (-> (run-exec {:arg0 "newarg0"} (format "%s %s :ps-me" bb u/wd)) :out edn/read-string))
  actual: (not (= {:args "newarg0 process/script/wd.clj :ps-me"} nil))
=== pre-start-fn-test

=== cmd-opt-test

=== exec-replaces-runner-that-launches-it-test


Ran 7 tests containing 15 assertions.
2 failures, 0 errors.
{:test 2403, :pass 14908, :fail 5, :error 0}

@leahneukirchen
Copy link
Member

These fail due to the container chroot. Let's merge this.

@leahneukirchen leahneukirchen merged commit 559053a into void-linux:master Jan 30, 2024
6 of 8 checks passed
@icp1994 icp1994 deleted the mandrel branch January 30, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants