Fixup nix configuration for more robust developer experience #84

ships · 2024-03-13T19:55:42Z

The changes made are as follows:

A more idiomatic Nix code layout; submodules in the nix/ directory and only flake.nix at the top level.
Use clj-nix to pin versions of all Clojure modules. Due to bugs in clj-nix, we are currently unable to use its actual JDK-building capabilities (which would let us slim down the build). However, we can use it to generate a SHA-perfect hermetic dependency directory and load that into the "dumb" Jar wrapper we previously had, removing the need for __noChroot.
Include building an OCI image that compiles for linux regardless of your workstation platform, but still only builds for your architecture.
nix develop -i and the container bash now includes most basic tools like curl, grep, awk, sed, and coreutils.

TODOs

include basic developer tools in the OCI image (or have an extra image for this, which we call "interactive" -- currently there is no link in $PATH even to stuff like Bash. But once we have Bash we would need other things like ls, which, and so forth; let alone tools we find useful that are domain specific)
include bb by default, but make bb defer to the build.clj so that either workflow is supported
better documentation
make the nix develop -i useable (currently since that isolates even from $PATH on your workstation, it is the same basic issue as the docker interactivity experience).
take advantage of buildLayeredImage for better layer sharing if this is a base for other images.

out of scope

Descoping this task because the check phase is for checking AFTER build:

~~include babashka tests in the check phase~~

Descoping this task because we get 90% of the way there without messing with remote builders

~~make the resulting OCI image cross-architecture by default~~

Descoping this task because it will be part of inferenceql/nix:

~~include automation so that this image is available on docker hub immediately and continuously updated~~

KingMob

I'm still a rank nix newb, so I have no comment on that, but I'm happy to see a nix expert's work to learn from!

I do have two comments, though:

This standardizes on JDK 17. Any reason we can't use the latest LTS, JDK 21?
This adds a build.clj file, but we already have a bb.edn task-runner file as well. I think that functionality should be combined. I believe it will be less confusing. (E.g., this PR would add a second clean task.) I have a slight preference for bb.edn for what it's worth.

ships · 2024-03-14T19:47:36Z

@KingMob thanks for these notes, next up for me is looking into the babashka integration so i may be able to nicely deal with the duplication at the same time as adding checks!

ships · 2024-03-14T21:44:30Z

I've updated to use OpenJDK 21, in a future case we may even be able to use GraalVM instead.

I am inclined to remove the dependency on babashka if that is acceptable to contributors; or at least, make the babashka scripts directly invoke scripts declared in build.clj and deps.edn. Still researching how idiomatic that would be. @Schaechtle do you have a feeling about babashka ?

KingMob · 2024-03-15T09:33:32Z

From a longer comment on Slack about the benefits/detriments of removing babashka:

At a glance, I see 3 out of 8 probcomp/iql repos on my disk are using bb, so it doesn't seem like they're wedded to bb.

There are some reasons to prefer bb, though:

Startup time. Java Clojure spends a lot of time initializing, so something like the clean task would go from instantaneous to several seconds. For the rest it wouldn't matter, though (although we'd have to switch to invoking clj-kondo directly from the command line).
Newcomer-friendly. The Clojure CLI options aren't as easy to remember IMO, and bb tasks can store those for you. Otherwise, you end up writing aliases or tiny shell scripts to capture those options anyway. (Or pray you can find it in your shell history from the other month.)
More fully-featured. Sometimes the core team likes to make things that are too minimal. tools.deps/deps.edn enabled some nice new features, but also forced the ecosystem to recreate 90% of the functionality that was missing from leiningen. bb makes things like filesystem operations, task inter-dependencies, and parallel execution easier.

On the flip side, the advantage to removing bb is one less dep, but I'm not sure what benefits that translates to. It's one less concept, yes, but if I had to make people pick just one tool, I'd bet bb is easier to use than clj, especially for non-Clojure users. In terms of stability, babashka is at least as stable as clj core, and for artifacts, we'd save some disk space, but we'd still depend on the underlying sci library, since query uses it.

All that being said, I don't think the switching costs are that high if everyone wanted to ditch babashka.

KingMob · 2024-03-15T14:53:16Z

Hey @ships , can we also get some support for node/npm/pnpm/yarn/whatever? I need to add shadow-cljs because our cljs build is getting more complicated now that I'm adding an external Js lib.

ships · 2024-03-15T20:08:17Z

@KingMob , i would love to track that with an issue and address in a followup PR. Is it able to wait that kinda timeframe ?

ships · 2024-03-15T22:00:40Z

Blocked on OpenGen/GenSQL.gpm.sppl#11 , so that we can refer to the merge sha in deps.edn.

ships · 2024-03-15T22:15:26Z

Okay, @KingMob , i have set it up so the clean directive in babashka delegates to tools.build . I would like to rely on tools.build in automation contexts so will keep the build.clj for now but allow bb as a friendly wrapper in the same way that it wraps clojure -X:test.

The NPM thing i will leave for a followup PR.

KingMob · 2024-03-18T09:51:40Z

@ships I have no time frame in mind whatsoever, just thought it might be nice to leverage nix for the Cljs/Js world if possible, and not too onerous.

ships · 2024-03-18T21:47:12Z

@KingMob , love it. great suggestion! created #87 .

zane

Looks good! Only blocking thing:

issue[1]: Commit headers should adhere to our existing style and use sentence-case.

zane · 2024-03-19T18:43:01Z

build.clj

+
+(defn clean [_]
+  (build/delete {:path "target"})
+  (build/delete {:path "cljs-test-runner-out"}))


note[1]: Ah, I see why you did this. I had thought to have this file be purely concerned with building the JAR, but this works as well. Downside to this is that clean will be slower because it has to start a Java process.

I see @KingMob highlighted this as well in his comment.

I see what you mean. I will revisit tomorrow but am thinking the two clean directives are actually unrelated-- one is build clean, the bb action was a test clean. maybe they can stay separate.

In that case, the bb clean should be renamed to reflect that.

I would still prefer one entry point, even if bb calls out to the build.clj for heavy lifting.

zane · 2024-03-19T18:44:34Z

README.md

+We use `clj-nix` to support tamper-proof reproducible builds of the dependencies for all environments.
+When you upgrade a package in `deps.edn`, it is necessary to update `deps-lock.json` as well, so that
+the nix build universe has knowledge of the hash fingerprints of the new deps version tarballs.


question[1]: What do you think about trying to automate this in a later PR? I imagine we could try to do it via a post-commit hook, but there are probably downsides to that approach that I'm not seeing.

In general, yes for automation. Post-commit hooks I particularly dislike because they interrupt any workflow with assurances about every workflow (imagine if you do git commit and suddenly your system is hanging waiting for Nix to build for the first time). I would rather have check in github actions that prevents merge unless you do the manual action when your PR is ready; or better yet, find a way to do a dependabot-style automatic PR opening thing.

You've convinced me!

+1 for CI automation. I'm experimenting with Jujutsu over git, and I don't think it supports hooks yet.

zane · 2024-03-19T18:53:09Z

deps.edn

@@ -6,6 +6,7 @@
        net.cgrand/macrovich {:mvn/version "0.2.1"}
        net.cgrand/xforms {:mvn/version "0.19.2"}
        io.github.inferenceql/inferenceql.inference {:git/sha "40e77dedf680b7936ce988b66186a86f5c4db6a5"}
+        io.github.inferenceql/inferenceql.gpm.sppl {:git/sha "f745dbb0c17c1a9da21488b7bd3098338ab7d7a2"}


thought[1]: I see you've made inferenceql.gpm.sppl a hard dependency, which is reasonable. Alternatively you could also include it in an alias.

The tradeoffs as I see them: inferenceql.gpm.sppl is optional and has a bunch of dependencies that we'd rather not concern ourselves with most of the time. On the other hand, having it be fully optional could make the build more complex. It is not clear to me, for instance, if / how clj-nix has support for aliases.

I think there is an optimization here which I anticipate including in a change (forthcoming) that moves to more centralized and reuable Nix code in inferenceql/nix. unfortunately, since clj-nix#mk-deps-cache doesn't support selecting an alias, we are obliged to do a little extra chicanery to manage multiple lockfiles based on conditions. Figuring that out will make more sense when the dimensionality of our cross-org nix usage is clearer.

It would be more urgent even now if inferenceql.gpm.sppl's clojure dependencies were more severe, or brought along python stuff automagically, which it does not.

So you're right that overall there is a loose end here that will be worn down one way or another.

zane · 2024-03-19T18:58:56Z

flake.nix

+        # this invocation is a bit odd on the eyes:
+        # callPackage is available only on the system-namespaced
+        #   packages (derived from inputs.nixpkgs automatically by
+        #   flake-parts), but ...
+        ociImg = pkgs.callPackage ./nix/oci {
+          inherit uber pname basicToolsFn;
+          # ... we still must pass in the original nixpkgs because
+          # we need access to a different system's set of packages
+          # when compiling for linux while remaining agnostic of
+          # the workstation platform we are running this on.
+          nixpkgs = inputs.nixpkgs;


quibble[1]: Would prefer sentence case for comments as that's what we're using elsewhere.

This sets up future changes that use `clj-nix` for reproducible dependency resolution and nix packaging flows.

`flake.nix` is the entrypoint to this package. Further details are specified in the README.

This dependency is not strictly necessary in all cases, but the Nix workflow to support a different set of deps for users of SPPL would be rather extra complex.

ships requested review from zane and Schaechtle March 13, 2024 20:12

KingMob reviewed Mar 14, 2024

View reviewed changes

ships marked this pull request as ready for review March 14, 2024 23:54

ships force-pushed the ships/nix-docker branch from 5bb6f54 to 88e14da Compare March 14, 2024 23:57

ships force-pushed the ships/nix-docker branch from 3661e28 to 725bfba Compare March 18, 2024 22:15

ships mentioned this pull request Mar 18, 2024

Packaging: include JS tooling for CLJS use cases #87

Open

zane approved these changes Mar 19, 2024

View reviewed changes

ships force-pushed the ships/nix-docker branch 2 times, most recently from 7ef79f3 to 159e10e Compare March 20, 2024 16:51

ships added 5 commits March 20, 2024 10:04

build: Adopt tools.build for script interfaces

a03ce77

This sets up future changes that use `clj-nix` for reproducible dependency resolution and nix packaging flows.

fix: Ensure output is delivered in --output CSV

bc8459a

feat: Adopt Nix flake distribution pattern

d3e67a2

`flake.nix` is the entrypoint to this package. Further details are specified in the README.

feat: Include the inferenceql.gpm.sppl wrapper dep

9a44670

This dependency is not strictly necessary in all cases, but the Nix workflow to support a different set of deps for users of SPPL would be rather extra complex.

chore: Comment on minimum nix version

c206467

ships force-pushed the ships/nix-docker branch from 159e10e to c206467 Compare March 20, 2024 17:04

ships merged commit 9cd372e into main Mar 20, 2024
3 checks passed

ships deleted the ships/nix-docker branch March 20, 2024 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixup nix configuration for more robust developer experience #84

Fixup nix configuration for more robust developer experience #84

ships commented Mar 13, 2024 •

edited

Loading

KingMob left a comment

ships commented Mar 14, 2024

ships commented Mar 14, 2024

KingMob commented Mar 15, 2024

KingMob commented Mar 15, 2024

ships commented Mar 15, 2024

ships commented Mar 15, 2024

ships commented Mar 15, 2024

KingMob commented Mar 18, 2024

ships commented Mar 18, 2024 •

edited

Loading

zane left a comment

zane Mar 19, 2024

zane Mar 19, 2024

ships Mar 20, 2024

KingMob Mar 20, 2024

zane Mar 19, 2024 •

edited

Loading

ships Mar 20, 2024

zane Mar 20, 2024

KingMob Mar 20, 2024

zane Mar 19, 2024

ships Mar 20, 2024

zane Mar 19, 2024

Fixup nix configuration for more robust developer experience #84

Fixup nix configuration for more robust developer experience #84

Conversation

ships commented Mar 13, 2024 • edited Loading

TODOs

out of scope

KingMob left a comment

Choose a reason for hiding this comment

ships commented Mar 14, 2024

ships commented Mar 14, 2024

KingMob commented Mar 15, 2024

KingMob commented Mar 15, 2024

ships commented Mar 15, 2024

ships commented Mar 15, 2024

ships commented Mar 15, 2024

KingMob commented Mar 18, 2024

ships commented Mar 18, 2024 • edited Loading

zane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zane Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ships commented Mar 13, 2024 •

edited

Loading

ships commented Mar 18, 2024 •

edited

Loading

zane Mar 19, 2024 •

edited

Loading