babashka: use upstream version of Clojure tools

[DerGuteMoritz]

Description of changes

As recommended by @borkdude[1], we should use the Clojure tools version included in a particular upstream release since the code might not always work with more recent versions.

[1] borkdude/deps.clj#113 (comment)

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

This change is 

[SuperSandro2000]

As recommended by @borkdude[1], we should use the Clojure tools version included in a particular upstream release since the code might not always work with more recent versions.

We don't want to vendor things. We should rather use an appropriate closure tools version from nixpkgs or add an override.

Also can we add a short test to know if things break on updates? Than we would be confident that things work even if we don't match the upstream version.

[jlesquembre]

It makes sense to try to use the recommended version, but in that case, we should add an script to automate the updates and keep the version in sync.
With the current approach, next time babashka updates clojure tools, the build will break.

[DerGuteMoritz]

We don't want to vendor things. We should rather use an appropriate closure tools version from nixpkgs or add an override.

@SuperSandro2000 Makes sense! I'll switch it over to using the clojure derivation as before and override its version to the expected one. Should have thought of that!

Also can we add a short test to know if things break on updates? Than we would be confident that things work even if we don't match the upstream version.

I think the best we can do is detect whether we need to update the tracked version on our end. Testing whether it works with a non-matching version seems intractable as the surface area of clojure tools is quite large and fully exposed via bb clojure. Will look into the former!

[DerGuteMoritz]

It makes sense to try to use the recommended version, but in that case, we should add an script to automate the updates and keep the version in sync. With the current approach, next time babashka updates clojure tools, the build will break.

@jlesquembre Good call, that's a better approach overall. I'll rework the patch accordingly (might take a few days, tho).

[jlesquembre]

Good call, that's a better approach overall. I'll rework the patch accordingly (might take a few days, tho).

@DerGuteMoritz thanks for working on it :)

[DerGuteMoritz]

@jlesquembre OK got back to it earlier than expected 😄 Check it out!

Note that updateScript was broken since the introduction of the wrapper: it still invoked update-source-version babashka but it should now have invoked update-source-version babashka-unwrapped instead! Fixed while I was in there anyway.

Hope the changes make sense and be sure to squash before merge (not sure if GH automatically squashes fixup commits?)

[DerGuteMoritz]

Forgot to mention that the bb clojure --version invocation in installCheckPhase will fail if the provided clojure version doesn't match the version expected by babashka. This is because t will then try to auto download the expected version and fail because it may not connect to the internet at that point.

[jlesquembre]

Thanks @DerGuteMoritz , looks good. Before approving, just some minor suggestions:

• Could you format the clojure.nix file with nixpkgs-fmt?
• I think we should rename clojure.nix to babashka-clojure.nix, babashka-clojure-tools.nix or something similar, to make even clearer why we override clojure. Also, a comment about it and a link to https://github.com/borkdude/deps.clj#deps_clj_tools_version in clojure.nix or wrapped.nix would be nice.
• Could you rebase and force push? I don't think nixpkgs repo has enabled the "squash and merge" feature.

@SuperSandro2000 Could you confirm if that is what you had in mind about using an override? To me it looks good, but I want to confirm it.

[DerGuteMoritz]

Hi @jlesquembre, thanks for your feedback which I have now addressed as follows:

• Could you format the clojure.nix file with nixpkgs-fmt?

TIL & done!

• I think we should rename clojure.nix to babashka-clojure.nix, babashka-clojure-tools.nix or something similar, to make even clearer why we override clojure. Also, a comment about it and a link to https://github.com/borkdude/deps.clj#deps_clj_tools_version in clojure.nix or wrapped.nix would be nice.

I went to rename it to clojure-tools.nix instead, the directory already scopes it well enough to be babashka-specific IMHO. Also changed all references to "clojure" within the code to "clojure-tools" - while it expects a clojure-like derivation, it's really the tools we're interested in and since that's also the term used by babashka itself, I think that makes sense.

Instead of adding comments to clojure.nix and wrapped.nix, I added a comment to clojure-tools.nix explaining its purpose. I think readers wondering what that thing is about will be able to find it there and that way it's not duplicated.

• Could you rebase and force push? I don't think nixpkgs repo has enabled the "squash and merge" feature.

Done that, too!

However, there's more:

• Remember how I originally extracted DEPS_CLJ_RELEASED_VERSION from the standalone jar to determine the Clojure tools version? Well, I had a quick chat with @borkdude about it and it turns out that that's not quite the right place to look for it. As the name suggests, it really is the version of deps.clj which usually matches the version of the Clojure tools releases but not necessarily so. For example, for the very recently released version 1.3.185, the old command yields:

  $ curl -fsL "https://github.com/babashka/babashka/releases/download/v1.3.185/babashka-1.3.185-standalone.jar" | bsdtar -qxOf - DEPS_CLJ_RELEASED_VERSION
  1.11.1.1403
  

  But if you look at the bump of deps.clj included in that release, you'll notice that the changelog entry mentions "Catch up with CLI 1.11.1.1413" (which is what prompted my investigation).

  Instead, the correct place to get the version from is this definition. Luckily, that file is also included in source form in the jar, so I changed to update script to extract that instead and parse it using babashka itself. That means I had to wrap the whole derivation in a let ... to be able to do that, so diff now best viewed with whitespace ignored.

• While working on the aforementioned change, I noticed that the error handling in the update script didn't quite behave as I thought it would: Even when the version extraction code failed, it would proceed to run update-source-version babashka.clojure-tools ... with an empty version string. After some headscratching I remembered that command substitutions such as $(...) don't inherit the parent shell's options, crucially set -e, so it would happily march on as if nothing happened. After some digging I found shopt -s inherit_errexit which does make it inherit that option. However, it still didn't seem to behave any different. Puzzled, I trawled through the web for help and ended up here:

  Another pitfall associated with set -e occurs when you use commands that look like assignments but aren't, such as export, declare, typeset or local.
  [...]
  [The exit status of the command substituion] won't trigger the set -e because the exit status of local masks it (the assignment to the variable succeeds, so local returns status 0).

  While not mentioned here, it appears that this also applies to readonly 😬 Not sure why it was used here in the first place but it doesn't seem to be overly important so I decided to just rip it out and voilà, errors now terminate the script even when something goes wrong in these command substitutions 💦

  Maybe we should just rewrite the whole update script in babashka 😄

Anyway, hope this all makes sense. I'm afraid another careful review is necessary now, though 🙏

[jlesquembre]

@DerGuteMoritz thanks, just 2 things I noticed:

• Is there a reason for let babashka-unwrapped = buildGraalvmNativeImage {...} in babashka-unwrapped instead of just buildGraalvmNativeImage {...}?
• You use babashka itself in the update script (which I like :-) ), in that case, I just want to mention that maybe be could replace more bash with babashka (If that makes sense to you, I didn't look in too much detail to the new update script yet)

Thanks again, I'll review it in more details in the upcoming days

UPDATE:

Maybe we should just rewrite the whole update script in babashka

I really like the idea 👍

[DerGuteMoritz]

• Is there a reason for let babashka-unwrapped = buildGraalvmNativeImage {...} in babashka-unwrapped instead of just buildGraalvmNativeImage {...}?

I think you answered the question yourself in the next item:

• You use babashka itself in the update script

i.e. the reason is so that I can do that 🙂 Or is there a nicer way?

As for rewriting the whole script in babashka: I'll give it a shot once this PR has landed. Was a deep enough rabbit hole as it is already and I'm sure that this would be an even deeper dig 😄

[jlesquembre]

@DerGuteMoritz I did another test, a looks good, thanks :)

Regarding the let babashka-unwrapped = buildGraalvmNativeImage {...}, I don't see a better way. I'd like to update buildGraalvmNativeImage to mimic the behavior of mkDerivation, so we could do something like buildGraalvmNativeImage (finalAttrs: ...), and remove the let binding and the rec, but that's something for the future. I'm adding it to my TODO list, but no promises.

Regarding rewriting the script in babashaka, I agree, better to do it on another PR. I want to mention that I have plans to work on some helpers to make easier to integrate babashka in nix derivations. Something similar to https://github.com/DeterminateSystems/nuenv but with babashka, I'd like to write complex derivations with Nix + Babahska, without any bash. Probably I'll start with it on https://github.com/jlesquembre/clj-nix, but if it's successful, I'd like to upstream it to nixpkgs.

[thiagokokada]

Builds are failing in darwin.

[DerGuteMoritz]

@jlesquembre

Regarding the let babashka-unwrapped = buildGraalvmNativeImage {...}, I don't see a better way. I'd like to update buildGraalvmNativeImage to mimic the behavior of mkDerivation, so we could do something like buildGraalvmNativeImage (finalAttrs: ...), and remove the let binding and the rec, but that's something for the future. I'm adding it to my TODO list, but no promises.

Cool, sounds good! OK then, let's keep it as it is for now.

Regarding rewriting the script in babashaka, I agree, better to do it on another PR.

👍

I want to mention that I have plans to work on some helpers to make easier to integrate babashka in nix derivations. Something similar to https://github.com/DeterminateSystems/nuenv but with babashka, I'd like to write complex derivations with Nix + Babahska, without any bash. Probably I'll start with it on https://github.com/jlesquembre/clj-nix, but if it's successful, I'd like to upstream it to nixpkgs.

That would be excellent. Ping me when you have something to try out, then I can give the conversion of the babashka package a go and hopefully provide you with some feedback 🙂

[thiagokokada]

Suggested change

	"-march=compatiblity"
	"-march=compatibility"

[DerGuteMoritz]

Ah damn, I copied & pasted it from #257473 (comment) and didn't notice the typo! How did you? The tests didn't fail after all 🤔 Anyway, fixed now. Also rebased the branch once more for good measure.

[thiagokokada]

Ah I see, I think you didn't realize that we run the builds in our CI.

If you look into the checks, you will eventually see (after ofborg-eval check finishes) something like:

babashka, babashka.passthru.tests on x86_64-linux

You can click in Details to see the build, and if you click you also go to the OfBorg Viewer interface to see more details. Example of the build with the typo:
https://logs.ofborg.org/?key=nixos/nixpkgs.257473&attempt_id=f7c7e3ce-6661-4411-ac5b-d37870f402e1

We run checks for 4 platforms: aarch64-linux, x86_64-linux, aarch64-darwin and x86_64-linux. So even if you don't have a macOS, you can see the build failures directly in the CI (it will take a while though, because the macOS CI is way slower than Linux CI).

In case it doesn't trigger the build step automatically (we extract which package to build from the commit messages, this is one of the reasons that commit messages are important BTW), you can trigger it manually by running @ofborg build <package>.

See more documentation about OfBorg here: https://github.com/NixOS/ofborg

[DerGuteMoritz]

Thanks for the info, much appreciated 🙏 I roughly knew that that's happening but since the overall checks status was "success", I didn't bother to check. Looks like these test jobs are configured to be allowed to fail? But it still makes sense to check them when they do I guess! makes mental note

Anyway, hope we're good to go now finally 💦

[thiagokokada]

Looks like these test jobs are configured to be allowed to fail? But it still makes sense to check them when they do I guess! makes mental note

Yes, we allow the checks to fail because some packages are expect to fail (e.g.: maybe the package is broken in macOS, but works in Linux). Also some packages are broken in master branch for some reason, and we shouldn't break builds just because a package is updated (but still broken).

[jlesquembre]

Thanks @DerGuteMoritz and @thiagokokada . This PR took long, but I'm happy with the outcome, we fixed a couple of issues, including a fix to upstream Babahska 🎉

[DerGuteMoritz]

And thank you, too @jlesquembre! I learned a lot from this whole experience 😄

BTW is updateScript run automatically somewhere? Because there is a new release of babashka already which it should now be able to pick up..!

[jlesquembre]

@DerGuteMoritz the updateScript is run automatically by https://github.com/r-ryantm bot:

• Docs: https://ryantm.github.io/nixpkgs-update/
• Logs: https://r.ryantm.com/log/

As far as I know, it's executed in a best-effort basis, meaning that sometimes you have to wait longer. As far as I remember, usually updates packages 2-3 days after a release, but sometimes I had to wait for a week, or even a bit longer.