Skip to content

feat(ops): add revise family of functions #281

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion cells/lib/ops.nix
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
cell,
}: let
inherit (inputs.cells.std.errors) requireInput;
inherit (import "${inputs.self}/deprecation.nix" inputs) warnWriteShellEntrypoint;
inherit (inputs.nixpkgs) lib;
in {
hashOfPath = path: baseNameOf (lib.head (lib.splitString "-" path));

mkMicrovm = import ./ops/mkMicrovm.nix {
inputs = requireInput "microvm" "github:astro/microvm.nix" "std.lib.ops.mkMicrovm";
};
Expand All @@ -18,4 +20,8 @@ in {
mkOCI = import ./ops/mkOCI.nix {inherit inputs cell;};
mkDevOCI = import ./ops/mkDevOCI.nix {inherit inputs cell;};
mkStandardOCI = import ./ops/mkStandardOCI.nix {inherit inputs cell;};

revise = import ./ops/revise.nix {inherit inputs cell;};
revisePackage = import ./ops/revisePackage.nix {inherit inputs cell;};
reviseOCI = import ./ops/reviseOCI.nix {inherit inputs cell;};
}
55 changes: 55 additions & 0 deletions cells/lib/ops/revise.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
{
inputs,
cell,
}: let
inherit (inputs) nixpkgs;
l = nixpkgs.lib // builtins;
in
/*
For use with `revisePackage` and `reviseOCI` to build containers in a mono-repo style
environment where the source code is contained in the same repository as the std code,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that is an unnecessary restriction.

This helps whenever unrelated changes should not bump the rev of a build.

Copy link
Contributor Author

@nrdxp nrdxp Mar 21, 2023

Choose a reason for hiding this comment

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

Potentially, I have found another use for sansrev. I currently use it in the devshell so that developers don't have to pointlessly wait for Mamba to rebuild each time the revision changes. I used to not even expose the package, just its hash, but I changed it to expose the package for just this reason.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One use case may be a published installer of something in a monorepo that also needs its most significant revision baked into it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Allows to find the most significant commit in a monorepo where the source is filtered, but still needs a revision. For use with `revisePackage` and `reviseOCI`.

specifically so that one may detect meaningful changes to the image via its tag in the
special case where the package's output includes the revision of the source code (e.g. for
displaying the version to the user).

Without special processing, this kind of package would cause the OCI image tag to change
Copy link
Collaborator

Choose a reason for hiding this comment

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

Explanation at the example of OCI Images:

To make it clear this specificity is at the service of an example, not a tacit narrowing of the api (as revisePackage was mentioned explicitly before).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or generalize the explanation since reviseOCI has the same parragraph...

on each new revision whether the actual contents of the image changed or not. Combined
with `std.incl`, one may have a very strong indicator for when the contents of the image
actually includes meaningful changes which avoids flooding the remote registry with superflous
copies.

Args:
op: Optional function which takes the package as an argument.
pkg: The package you wish to revise.
fn: Optional functor with a reference to `pkg` if needed by `op`.

Returns:
The package with a clone of itself in the passthru where the expected revision is set to
"not-a-commit" for later use by `reviseOCI`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

and `revisePackage`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well the order as far as the user is concerned is revisePackage -> revise (on the operable) -> reviseOCI, so its speaking to that sequence. The use of revise in revisePackage is strictly an implementation detail

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean revidePackage can't be used standalone for this purpose?

*/
op: pkg: fn: let
result = op (fn pkg);
dummy = "not-a-commit";
rev = pkg.src.rev or pkg.src.origSrc.rev or dummy;
in
if pkg ? sansrev || (rev != dummy && result == pkg)
Copy link
Collaborator

Choose a reason for hiding this comment

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

sansrev looks like a contract that could be documented. If not public, I, personally would benefit from a couple of code comments below.

Copy link
Contributor Author

@nrdxp nrdxp Mar 21, 2023

Choose a reason for hiding this comment

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

the section after the || is to tighten the contract from revisePackage. The first contract (existence of sansrev) is universal but the second case is only valid in ussage of revisePackage and so that is made explicit since this condition can only be met if the two functors passed in are both the id function (as they are in revisePackage).

Copy link
Collaborator

@blaggacao blaggacao Mar 24, 2023

Choose a reason for hiding this comment

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

I've made an idempotent refactoring and I don't understand what this function does:

  op: pkg: fn: let
    result = op (fn pkg);
    dummy = "not-a-commit";
    rev = pkg.src.rev or pkg.src.origSrc.rev or dummy;

    sansrev = let
      pkg' = op (fn (pkg.sansrev or (pkg.override {rev = dummy;})));
      passthru = pkg'.passthru or {} // {outHash = cell.ops.hashOfPath pkg'.outPath;};
    in
      pkg'.overrideAttrs (_: {inherit passthru;});
    passthru = result.passthru or {} // {inherit sansrev;};
  in
    if pkg ? sansrev || (rev != dummy && result == pkg)
    then result.overrideAttrs (_: {inherit passthru;})
    else result

If it does more than one thing, can it be stripped down (potentially giving up non-essential functionality)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I gain the impression this function could also be called 42 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's called like revise mkOerable packages.operable (operable: { /* ... */ }) The functor is necessary to defer the binding of operable

then
result.overrideAttrs (_: {
passthru =
result.passthru
or {}
// {
sansrev = let
pkg' = op (fn (pkg.sansrev or (pkg.override {rev = dummy;})));
in
pkg'.overrideAttrs (_: {
passthru =
pkg'.passthru
or {}
// {
outHash = cell.ops.hashOfPath pkg'.outPath;
};
});
};
})
else result
64 changes: 64 additions & 0 deletions cells/lib/ops/reviseOCI.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
{
inputs,
cell,
}: let
inherit (inputs) nixpkgs;
l = nixpkgs.lib // builtins;
in
/*
Utility function to allow for building containers in a mono-repo style environment where
the source code is contained in the same repository as the std code, specifically so that
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above: unecessary restriction?

Copy link
Contributor Author

@nrdxp nrdxp Mar 21, 2023

Choose a reason for hiding this comment

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

It has two uses, I think both are spelled out here. What other usecases could you imagine?

one may detect meaningful changes to the image via its tag in the special case where the
package's output includes the revision of the source code (e.g. for displaying the version
to the user).

Without special processing, this kind of package would cause the OCI image tag to change
on each new revision whether the actual contents of the image changed or not. Combined
with `std.incl`, one may have a very strong indicator for when the contents of the image
actually includes meaningful changes which avoids flooding the remote registry with superflous
copies.

This function can also be called where the package does not need the revsion at build time
but you simply want to tag the image by its hash for later processing by the proviso, and
you also want to include additional tags on the image, such as the revision.

Args:
args: arguments to the mkOCI function to be called.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Args is the whole caller args, now (args @). Maybe that's been refactored? Imo the ... covers it well.

mkOCI: defaults to `mkStandardOCI`
operable: The operable to include in the container
...: The same arguments expected by the given standard OCI builder

Returns:
An image tagged with the output hash of an identical image, except where the target package
and operable are both built with the revision input set to "not-a-commit" instead of the true
revision, so that the hash does not change unless something inside the image does.
*/
args' @ {
operable,
mkOCI ? cell.ops.mkStandardOCI,
...
}: let
args = builtins.removeAttrs args' ["mkOCI"];
revision = cell.ops.revise mkOCI args.operable (operable: args // {inherit operable;});
in
if args.operable ? sansrev
then
mkOCI (args
// {
meta =
args.meta
or {}
// {
tags = [revision.sansrev.outHash] ++ (args.meta.tags or []);
};
})
else
mkOCI (args
// {
meta =
args.meta
or {}
// {
tags = [(cell.ops.hashOfPath revision.outPath)] ++ (args.meta.tags or []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, now I wonder even more how the magic of sansrev signalling and dry-running works.

Maybe revise.nix would indeed be a good place to explain.

Copy link
Contributor Author

@nrdxp nrdxp Mar 21, 2023

Choose a reason for hiding this comment

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

The whole point of sansrev is to have a version of the package that doesn't change when the revision does. The only way to guarantee this is to take the rev as an argument to callPackage (hence why rev is a required argument to revisePackage), and pass in the default not-a-commit unconditionally to it.

I tried more clever approaches like modifying src.origSrc.rev but the hash of the outPath hash of the src is different than the version created when the tree is genuinely dirty, which is ugly and confusing.

Another way of thinking of it is that sansrev is a copy of the package state when the tree is genuinely dirty.

In order for this to be a highly strong guarantee, we need to also track changes to the operable and the image itself so that the hash does actually change when those change (say adding args to mkStandardOCI or modifying the operable script).

So the process is: sansrev from original package -> operable built from this sansrev package -> OCI image built from this sansrev operable -> use the hash of that "sansrev" image as the tag so that any changes at all to any of the tree layers will induce a change to the hash.

Copy link
Collaborator

@blaggacao blaggacao Mar 24, 2023

Choose a reason for hiding this comment

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

Thanks, the overall strategy is clear to me now, but I'm more advocating for code comments on behalf of a future self or present/future other.

Can you curry and spice up the code with a couple of code comments that guide the code reader through the logic of sansrev?

};
})
38 changes: 38 additions & 0 deletions cells/lib/ops/revisePackage.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
{
inputs,
cell,
}: let
inherit (inputs) nixpkgs;
l = nixpkgs.lib // builtins;
in
/*
For use with `revise` and `reviseOCI` to build containers in a mono-repo style
environment where the source code is contained in the same repository as the std code,
specifically so that one may detect meaningful changes to the image via its tag in the
special case where the package's output includes the revision of the source code (e.g. for
displaying the version to the user).

Without special processing, this kind of package would cause the OCI image tag to change
on each new revision whether the actual contents of the image changed or not. Combined
with `std.incl`, one may have a very strong indicator for when the contents of the image
actually includes meaningful changes which avoids flooding the remote registry with superflous
copies.
Comment on lines +15 to +19
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could explain how this applies to packages here ☺️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've struggled to explain this from the beginning, so if you have any suggestions I'm all for it 😅


Args:
target: Same as the first argument to upstream `callPackage`.
args: Arguments to `callPackage`.

Returns:
The package with a clone of itself in the passthru where the expected revision is set to
"not-a-commit" for later use by `revise` & `reviseOCI`.
*/
target: args @ {
rev,
callPackage ? nixpkgs.callPackage,
...
}: let
pkg = callPackage target (builtins.removeAttrs args ["callPackage"]);
in
if pkg ? sansrev
then pkg
else cell.ops.revise (_: _) pkg (_: _)