-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
Single package fixpoint step 2: introduce mkPackage
#296769
base: master
Are you sure you want to change the base?
Conversation
e23626c
to
548835c
Compare
So continuing from NixOS/nix#10610 (comment), my thought is similar to the talk of NixOS module system modularity. Just as we'd like multiple PostgreSQL "instances", for example, I think we should be very careful to destinguish a package in isolation, which does not care how it is used (e.g. choosing its deps) --- it is function, from an instantiate package in a package set, which is not a function. |
I agree. For this it is important that the package presents itself as a function with a stable interface. Suppose you're packaging
In this situation, (1) is wrong because it chooses its deps, and it's not a stable function this way, because callers need to be aware of the current default version. With this implementation of the single fixpoint idea, they can preserve the existing function interface, and set a default in the same file - without touching
{ mkPackageWithDeps, layers }:
mkPackageWithDeps ({ pkgs, stdenv, postgresql, ... }: [
(layers.derivation { inherit stdenv; })
(this: old: {
name = "app";
deps = old.deps // {
postgresql = pkgs.postgresql_14;
};
setup.buildInputs = [ postgresql ];
}
])
|
But what happens if I try to instantiate this package within a package set that doesn't have a I would prefer something like either
for a bit more of a by-construction guarantee the package function stands alone without the defaults. |
pkgs/by-name/he/hello/package.nix
Outdated
, testers | ||
, hello | ||
}: | ||
{ mkPackageWithDeps, layers }: mkPackageWithDeps ({ stdenv, fetchurl, testers, lib, callPackage, nixos }: [ |
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.
Consider adding layers
as well as a function overrideLayers
to the call here, such that they don't come from the normal fixpoint. Otherwise it's not possible to override that layers
, which someone will inevitably need.
It also makes the separation of concerns a bit nicer:
# 1 parameter to declare that we'll be doing it the layered way
{ mkPackageWithDeps }: mkPackageWithDeps ({
# all the rest goes here
layers, ...
}: [
# ...
])
That changes the fixpoint "state" from just overlays to { overlays = <composition of functions>; layers = <attrset of functions brought into scope by mkPackageWithDeps>; }
, but that seems manageable and not something users have to be aware of, as long as it works correctly. It's still just a single fixpoint, so it seems feasible to get it right.
This could be implemented as syntax sugar. The { mkPackageWithDefaults }: mkPackageWithDefaults {
function = ...;
dep-defaults = ...;
} If we don't implement it as syntax sugar, the "obvious" code would not have a single fixpoint, which would instantly make overriding incoherent. Make it syntax sugar, and we avoid that problem. |
Ah, but this is just what I am taking issue with, I want to force packages to not mix together the package function and the default argument choosing. I agree making people edit If we think packages are "straight away" put in the big fixed point, this can all seem academic, but if we think we'll first build a library of package functions, and then build the big fixed point from that, the distinction gets more practice. The "library of package functions" (recipe book) is very important to me as the only thing which is truly modular. |
Sure, but then you're not compatible with
That sounds a lot like what was done in the past and what has got us into the current situation, unless we have the discipline not to add unnecessary fixpoints in the process, as you suggest. I am a bit skeptical though:
|
Yeah I want to do one round of importing all the functions and calling nothing, maximally flexible. No fixed points, no overriding, non of that. Just library of recipes, everything is in "fully functored form" as the OCaml people would say. And then I want to do another round of one big fixed point, putting everything together.
You are contrasting that the layers thing you have? I haven't thought about that so much but I think there is no conflict. |
@Ericson2314 I think we agree on the overall strategy. |
I particularly like this idea because it could make it possible to "override" elements of the package set that are not drvs or attrsets in their "final form" (where a |
@roberth This looks really cool! I was wondering if you had mocked up a use of this type of function in a similar manner to dream2nix(since you mentioned it in the issue); I was thinking of trying to do that both as a learning opportunity and to see if I could figure out dynamic derivations as well, but I also didn't want to redo anything you were working on, especially since these things would probably be pretty hard for me given that all of this is pretty new. Also wanted to ask @DavHau since you also might be working on something like this, or might have some insight. |
@purepani, that sounds like a lot to do simultaneously. My goal was to show that it can be done, and establish a starting point. Haven't been able to spend much time on it since. I'd be happy to explain things if you want to try it. |
While I definitely agree and I am pleased with the result. I would have one recommendation which is to make it at first such that the existing function can still be used without making any changes, and then progressively replace one by the other. The idea would be to avoid having any down-time as these kind of changes would likely takes months / years before they are complete, and having something backward compatible would also help with the migration process. Thus |
@nbp Not sure I understand what you mean by downtime. This PR adds an alternative to
That sounds rather invasive.
I don't see how returning functions and arguments helps with those expectations or reducing the number of fixpoints per package. Rather it hints at adding another one. |
I think you're applying a solution you've thought of for S.O.S. here, but prematurely. My understanding of that concept is that it operates at the package set level, unlike this PR. So I'd be happy to replace |
Maybe what you're asking for is a way to skip ahead to that point where we can change all package files to be single-fixpoint style? |
I'll definitely let you know if I have any questions once I have a chance to dig into it! |
Having some time to think about this, I'm wondering if it would make sense to have some sort of "typing information" file(like header files in C++), and sort of introduce gradual typing into this.
Then you could prerun, and cache the typing information, and then use that information to help in any sort of merging process, only recompiling when any header files change. This feels like something that could be more useful to implement on a language level, but I thought I'd put it out there since it's an extension of what was being talked about earlier. |
Something like this is probably half way in terms of performance between the module system and hand-coded merging (as we do with We could go faster by removing the priorities, or slower by always checking the value. To really go faster though, I think we need a To bring this closer, we could make a gradual change to Meanwhile @nbp is working on a proposal (in his free time, afaik) to make deep overrides easier to write, which might also run faster in the evaluator just by virtue of being interpreted more directly instead of a bunch of function calls with all the Nix function bells and whistles. Idk, maybe we should get someone from SpiderMonkey on Nix ;) |
I think the next step is to try and convert an existing
Steps
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/is-it-possible-for-us-to-remove-builtins-functionargs/51960/4 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/community-team-updates/56458/11 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/whats-the-status-of-the-single-package-fixpoint-proposal/58692/1 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/whats-the-status-of-the-single-package-fixpoint-proposal/58692/2 |
715fbfa
to
391d055
Compare
391d055
to
c187546
Compare
Creates objects that have overlay-based private attrs in their closure. (cherry picked from commit 0067cf4)
This shows the `deps` pattern, merging the following into a single fixpoint, from NixOS#273815 - package function args (`this.deps`) - mkDerivation args (`this.setup`) - derivation args (`this.drvAttrs`) - package attributes (`this.public`)
Demo: ```diff diff --git a/pkgs/by-name/he/hello/package.nix b/pkgs/by-name/he/hello/package.nix index 0cf0ae493b72..69921e14ffc6 100644 --- a/pkgs/by-name/he/hello/package.nix +++ b/pkgs/by-name/he/hello/package.nix @@ -1,9 +1,13 @@ -{ mkPackageWithDeps, layers }: mkPackageWithDeps ({ stdenv, fetchurl, testers, lib, callPackage, nixos }: [ +{ mkPackageWithDeps, layers }: mkPackageWithDeps ({ stdenv, fetchurl, testers, lib, callPackage, nixos, haskellPackages, omnidoc }: [ (layers.derivation { inherit stdenv; }) (this: old: { name = "hello"; version = "2.12.1"; + deps = old.deps // { + omnidoc = haskellPackages.pandoc; + }; + setup = old.setup or {} // { src = fetchurl { url = "mirror://gnu/hello/hello-${this.version}.tar.gz"; @@ -24,6 +28,10 @@ run = callPackage ./test.nix { hello = this.public; }; }; + public = old.public // { # passthru, just to show that it works + inherit omnidoc; + }; + meta = with lib; { description = "A program that produces a familiar, friendly greeting"; longDescription = '' ```
This also makes the old `mkPackageWithDeps` style the default, and improves error reporting a bit.
The solution inside makeOverridable was fragile, and did not catch the potential issue that the new code reports. It's still not super pretty, but architecturally it won't get better than this because `by-name/**/package.nix` is tied to `callPackage`. Alternative solution: require a different file name, so that `callPackage` can be avoided.
Giving no access at all would be really annoying for debugging. Unlike `mkDerivation`, which makes no distinction between intentional and internal package attributes, this gives a signal that when you're using it, you are more likely to have to change your code at some point. (besides keeping the public package attrs _relevant_)
This allows the all-packages.nix entry to be removed thanks to pkgs/by-name 🎉
Nothing special for now. |
b88cfe5
to
d7035f7
Compare
mkPackage
The real _derivation system_ is best retrieved from `drvPath`. Putting this in an attribute with this simple name is ambiguous, because it can refer to either the `hostPlatform.system` or `buildPlatform.system`, which are unequal in cross compilation. The _system_ attribute tends to refer to the _derivation system_ which is the `buildPlatform.system`, but this is an implementation detail of the package internals, and it is not relevant to any in-language consumers of the package, especially considering that knowledge about one `system` does not imply anything about the many possible `system`s that can be in the derivation's build closure, which tends to be equally relevant for the process of realisation.
8ff7139
to
5e36866
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.
Just pointing out how the fixed points change; see review comments
+1 fixpoints: encapsulate
-1 fixpoints: callPackage
-1 fixpoints: mkDerivation
for a total change of -1 accidentally individually overridable things
encapsulate = | ||
layerZero: | ||
let | ||
fixed = layerZero ({ extend = f: encapsulate (extends f layerZero); } // fixed); |
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.
This forms the new fixpoint.
]) | ||
'' | ||
else | ||
f allArgs |
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.
This avoids adding the second fixpoint (callPackage
/makeOverridable
/.override
).
I should say that this is an overly broad interpretation of fixpoint, but a recursion is definitely there, so I'll just use the term. (even if result
isn't passed back to makeOverridable
's f
, the overriding dynamics are equally problematic)
Description of changes
This provides an alternative to
mkDerivation
- a different interface which solves a significant number of problems.Proposal: Bring all packaging layers into a single fixpoint #273815
Includes: lib: Add encapsulate, attrsets that have overlay-based private attrs in their closure #158781
Follows make-derivation.nix: Factor out
makeDerivationArgument
#295378Features and problems solved:
mkPackage
will be usable with alternatives tolayers.derivation
)doc
output to improve bootstrapping and avoid some rebuilds__cleanAttrs
: Clean packages that don't expose all internals #217243passthru
layers
can be combined, e.g. python + javascript in a single derivationTODO
derivation
.setup
, i.e. where known stdenv/setup/mkDerivation arguments gomkDerivation
interface on top of this?Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.