-
Notifications
You must be signed in to change notification settings - Fork 43
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
base: main
Are you sure you want to change the base?
Conversation
0697d9d
to
2e6be57
Compare
@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. |
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.
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 |
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.
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).
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.
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`. |
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.
and `revisePackage`
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.
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
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.
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) |
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.
sansrev
looks like a contract that could be documented. If not public, I, personally would benefit from a couple of code comments below.
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.
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
).
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'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)?
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 gain the impression this function could also be called 42
😄
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.
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, |
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 think that is an unnecessary restriction.
This helps whenever unrelated changes should not bump the rev
of a build.
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.
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.
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.
One use case may be a published installer of something in a monorepo that also needs its most significant revision baked into it.
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.
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 |
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.
As above: unecessary restriction?
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.
It has two uses, I think both are spelled out here. What other usecases could you imagine?
args.meta | ||
or {} | ||
// { | ||
tags = [(cell.ops.hashOfPath revision.outPath)] ++ (args.meta.tags or []); |
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.
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.
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.
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.
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.
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
?
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. |
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.
We could explain how this applies to packages 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've struggled to explain this from the beginning, so if you have any suggestions I'm all for it 😅
cells/lib/ops/revisePackage.nix
Outdated
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 |
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.
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.
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.
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.
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'm afraid that callPackage
is not a public api on targets, at all. Targets of type package
are already of type derivation
.
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 think what you mean is pkgFun
, that aren't yet of block type packages
and don't constitute a target themselves.
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.
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.
48e1308
to
fcfa81c
Compare
revise
family of packagesrevise
family of functions
f453ff3
to
df4cd51
Compare
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.