Skip to content
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(ops): add revise family of functions #281

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

nrdxp
Copy link
Contributor

@nrdxp nrdxp commented Mar 21, 2023

Utility functions 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 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.

reviseOCI 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.

@nrdxp nrdxp force-pushed the revise-oci branch 3 times, most recently from 0697d9d to 2e6be57 Compare March 21, 2023 01:49
@blaggacao
Copy link
Collaborator

blaggacao commented Mar 21, 2023

@gytis-ivaskevicius @michalrus

This is PR and the technique behind it is quite relevant for our repositories where stakeholders expect to see the most significant revision that generated any particular build.

Copy link
Collaborator

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

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

A first pass, focusing on understanding and trying to ingest the docstrings.

I still have some knowledge gaps on the detailed working of sansrev.

I understand the dry-run capability on a constant revision (not-a-commit) and how it's tactically leveraged. But I don't understand the implementation (yet) 😢.

That's what the next round will be for, hopefully 🤞

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...


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?

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

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`.

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?

cells/lib/ops/reviseOCI.nix Show resolved Hide resolved
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?

Comment on lines +15 to +19
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.
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 😅

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a use-case in making callPackages overridable? I know the callPackage argument API is cheap via global context (which itself if expensive).

But still: reading the code here, I can't but wonder about why having it overridable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because your nixpkgs from standard and the one you use globally in the project may not be the same. Overriding one or the other in the flake is error prone since it has other affects, so to give users an escape hatch, I made it overridable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm afraid that callPackage is not a public api on targets, at all. Targets of type package are already of type derivation.

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 think what you mean is pkgFun, that aren't yet of block type packages and don't constitute a target themselves.

Copy link
Contributor Author

@nrdxp nrdxp Mar 24, 2023

Choose a reason for hiding this comment

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

It's not meant to be called on a derivation, it's meant to be called just like callPackage. Calling it on an already finished derivation would be "too late". The name target does not refer to Standard targets, but the first argument of callPackage which can be a path or a set.

cells/lib/ops.nix Show resolved Hide resolved
@nrdxp nrdxp force-pushed the revise-oci branch 2 times, most recently from 48e1308 to fcfa81c Compare March 21, 2023 17:11
@nrdxp nrdxp changed the title feat(ops): add revise family of packages feat(ops): add revise family of functions Mar 23, 2023
@blaggacao blaggacao force-pushed the main branch 10 times, most recently from f453ff3 to df4cd51 Compare June 16, 2023 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants