Page MenuHomeSolus

Update to babashka 0.8.2
ClosedPublic

Authored by oly on Mon, May 9, 6:31 PM.

Details

Summary

Update to babashka 0.8.2

Changelog available here

Test Plan

Ran /usr/bin/bb -e "(prn \"hello\")" to test

Diff Detail

Repository
R5480 babashka
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

oly requested review of this revision.Mon, May 9, 6:31 PM
oly created this revision.
oly updated this revision to Diff 32297.Mon, May 9, 6:36 PM

Updating commit message.

Staudey requested changes to this revision.Mon, May 9, 6:52 PM
Staudey added a subscriber: Staudey.

Your title and first line of summary still call it "inclusion of babashka" instead of "Update babashka to 0.8.2" (or similar)
You should also provide the changelog, or a link to the changelog(s), in your summary.

In the test plan, it's not necessary to mention that you "installed generated eopkg". That's the most basic requirement, and simply assumed.

This revision now requires changes to proceed.Mon, May 9, 6:52 PM
oly updated this revision to Diff 32298.Mon, May 9, 7:59 PM

Update commit message and add link to changelog.

Does this build in Openjdk-17?

In D13210#225897, @oly wrote:

Update commit message and add link to changelog.

Thanks! Unfortunately it's still not quite where I want it yet. Two points/nitpicks:

  1. Please don't put the changelog in the first line/heading of the summary but a line below it, and use something like in the form of Changelog available [here](link-to-changelog) instead of simply dropping in the link. While you're at it, please also replace the link with "https://github.com/babashka/babashka/blob/v0.8.2/CHANGELOG.md" so it's tagged to this specific version and won't change when somebody looks at it later (not the worst thing in the world, but as I said, while you're at it ๐Ÿ˜‰ )
  2. Please also update the contents of this diff here on phabricator (see that it still says "Inclusion..." and so on)

(and Reilly's suggestion is also a good point; might as well update it to the new JDK version, if possible)

oly updated this revision to Diff 32307.Tue, May 10, 6:49 PM

Changed to use java version 17 and changed commit messages in branch.

oly added a comment.Tue, May 10, 6:54 PM
In D13210#225897, @oly wrote:

Update commit message and add link to changelog.

Thanks! Unfortunately it's still not quite where I want it yet. Two points/nitpicks:

  1. Please don't put the changelog in the first line/heading of the summary but a line below it, and use something like in the form of Changelog available [here](link-to-changelog) instead of simply dropping in the link. While you're at it, please also replace the link with "https://github.com/babashka/babashka/blob/v0.8.2/CHANGELOG.md" so it's tagged to this specific version and won't change when somebody looks at it later (not the worst thing in the world, but as I said, while you're at it ๐Ÿ˜‰ )
  2. Please also update the contents of this diff here on phabricator (see that it still says "Inclusion..." and so on)

(and Reilly's suggestion is also a good point; might as well update it to the new JDK version, if possible)

Updated to use java 17 and adjusted commit partially.

Yeah no problem I always get in a mess with arc diff, how can I change the message so it does not say Initial inclusion of Babashka resolves it does not show in the git log so I guess there is a arc command to change it ?

In D13210#225962, @oly wrote:

how can I change the message so it does not say Initial inclusion of Babashka resolves it does not show in the git log so I guess there is a arc command to change it ?

You can do it with the "Edit revision" button in the top right

oly retitled this revision from Initial inclusion of Babashka resolves T10072 to Update to babashka 0.8.2.Tue, May 10, 7:05 PM
oly edited the summary of this revision. (Show Details)

You can do it with the "Edit revision" button in the top right

Okay thanks that's easier, hopefully that's everything addressed

Are you sure that you're actually using openjdk-17 for the build? The JAVA_CMD variable is still pointing to openjdk-11 (it appears that openjdk-11 is a rundep of leiningen so it's still getting pulled in)

oly added a comment.Tue, May 10, 8:10 PM

Are you sure that you're actually using openjdk-17 for the build? The JAVA_CMD variable is still pointing to openjdk-11 (it appears that openjdk-11 is a rundep of leiningen so it's still getting pulled in)

Good spot your quite right I will change rebuild and test again and upload probably tomorrow now.

oly updated this revision to Diff 32309.Wed, May 11, 4:33 PM

Update JAVA_CMD to use java 17

algent added a subscriber: algent.Wed, May 11, 4:56 PM

I would recommend to update graalvm too, and then build babashka against it.

Yeah, seems like it still builds against openjdk-11, despite the JAVA_CMD env, most likely due to graalvm being built with openjdk-11 (a special bundled version).

oly added a comment.Wed, May 11, 6:25 PM

I will take a look at graal usually take longer as my personal laptop does not have the ram to build it. saying that I did read they improved the usage in the newer version so maybe i will get lucky :)

It looks like Graal does support being built to be compatible with JDK 17 level features

Staudey edited the test plan for this revision. (Show Details)Wed, May 11, 7:15 PM
Staudey accepted this revision.Wed, May 11, 7:18 PM

Looks good to me now, thanks!
I'm just going to also add the abi_used_libs/symbols changes due to the graalvm rebuild when I push this.

This revision is now accepted and ready to land.Wed, May 11, 7:18 PM
This revision was automatically updated to reflect the committed changes.