-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: add OCI image with Loom on Python3 #6
Conversation
8a63b1e
to
2e88a03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly fine, left some suggestions to make things a bit more idiomatic.
goftests = callPackage ./../goftests { }; | ||
parsable = callPackage ./../parsable { }; | ||
pymetis = callPackage ./../pymetis { }; | ||
distributions = callPackage ./../distributions {inherit goftests parsable;}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're gonna use a custom scoped callPackage
it seems like it'd be a bit more idiomatic to have a attrset of private packages merged into the callPackage
scope. That way packages within the flake can use the args injection to reference them from other packages without having to deal with the paths, but they won't be exposed directly under the flake packages
field.
in pkgs.dockerTools.buildImage { | ||
name = "probcomp/inferenceql.loom"; | ||
tag = systemWithLinux; | ||
fromImage = ociImgBase; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest not using fromImage
and just building a single layered image for each instead by composing in whatever base config in Nix-land. You'll end up with a smaller image that's easier to reason about.
parsable | ||
; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
passthru.ociImage = callPackage ./ociImage.nix { }; |
So you'd end up with nix build .#loom.ociImage
rather than a separate sibling package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I like the inheritance here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am weighing this abstraction against like .#oci.loom
...
49fbeea
to
d6d5b4d
Compare
@Schaechtle , would love your take on these. I think i am inclined to pull it in as-is and address some of the modularization @srounce talks about later, because it will be better decided once we have more context about how central this repo is compared to nix-code in each component repo. |
yes. That sounds right to me. |
FYI, the following runs fine on my machine
|
d6d5b4d
to
5ee8fd9
Compare
Co-authored-by: Samuel Rounce <[email protected]>
Co-authored-by: Samuel Rounce <[email protected]>
2abd7fa
to
9e19987
Compare
This PR adds a Nix derivation for a layered OCI image that contains Loom and its dependencies for python3.
Note that currently, Loom's dependency
distributions
does not have a compilation pathway for ARM architectures, so although this defines packages on all architectures, you must build the image with.#packages.x86_64-linux.ociImgLoom
.Note that about half of the changes here are a refactor to support use of
callPy3Package
fromcrossPkgsLinux
.