Improve detailed design section and other minor things #34
Conversation
Main changes: - CI should check that pkg-fun.nix's return derivations - The standard must be followed for new packages
README.md
Outdated
| Each unit directory must contain at least a `pkg-fun.nix` file, but may contain arbitrary other files and directories. | ||
| 2. Semantics: Calling `pkgs.callPackage pkgs/unit/${shard}/${name}/pkg-fun.nix {}` must return a derivation that can be built directly with `nix-build`. | ||
| 3. Context-freedom: Files inside a unit directory must not reference files outside that unit directory, and the other way around. | ||
| The only exception is the next requirement. |
There was a problem hiding this comment.
| The only exception is the next requirement. | |
| The only exception is the next requirement. Dependencies from outside the unit only come in through the `pkg-fun.nix` function arguments. |
A clarification for those who may be puzzled by this supposed isolation.
There was a problem hiding this comment.
How about this, adding an explanation to each individual requirement (this should also be done for the others):
- Boundaries: Unit directories only interact with the rest of nixpkgs via the
pkgs/unit/${shard}/${name}/pkg-fun.nixfile and its correspondingpkgs.${name}attribute. Concretely:- Files inside a unit directory must not reference files outside that unit directory.
Therefore all dependencies on other packages must come from the package arguments inpkg-fun.nix.
This ensures that files in nixpkgs can be moved around without breaking this package. - Files outside a unit directory must not reference files inside that unit directory.
Therefore other packages can only depend on this package viapkgs.${name}.
This ensures that files within unit directories (exceptpkg-fun.nix) can be freely moved and changed without breaking any other packages.
The only other exception is the next requirement.
- Files inside a unit directory must not reference files outside that unit directory.
There was a problem hiding this comment.
sgtm. You've already titled them, so maybe turn them into subsections?
Also in the first subitem you just wrote: not just moving around, but other changes as well; use the public interface of those packages. EDIT: you explain it better the second time.
In the second: pkgs.${name} or callPackage injection.
There was a problem hiding this comment.
Good idea, I added sections now, I can't fully parse your reply, but I changed it a bit to emphasize the stable interface.
README.md
Outdated
|
|
||
| This convention is optional, but it will be applied to all existing packages where possible. Nixpkgs reviewers may encourage contributors to use this convention without enforcing it. | ||
| This standard must be followed for newly added packages. | ||
| A treewide migration to this standard will be performed for all existing packages that can satisfy the requirements. |
There was a problem hiding this comment.
Does that include args != { }? If not, that's ok, but then:
| A treewide migration to this standard will be performed for all existing packages that can satisfy the requirements. | |
| A treewide migration to this standard will be performed for existing packages that can satisfy the requirements. |
There was a problem hiding this comment.
I think the tooling should do all the migrations that it can, which then includes when args != {}, because if the tooling doesn't do it, users might do it anyways, it is something that's supported after all. It's not great how one can't know what arguments pkg-fun.nix files are called with anymore, but this is the case in current nixpkgs anyways, and migrating or not for args != {} won't change that.
I'm a bit confused by your suggestion though, if only args == {} would be migrated, then that would be an additional requirement for the migration only, but since I don't think we should do that, the sentence is good as is. It should be fine to remove the word all in both cases though, doesn't really change the meaning.
There was a problem hiding this comment.
If the tool can migrate args != { }, all the better.
I assumed that this sentence was about the automated part only. I do think they should eventually be migrated, whether automatically or by hand.
There was a problem hiding this comment.
one can't know what arguments
pkg-fun.nixfiles are called with anymore, but this is the case in current nixpkgs anyway
Correct, which is why we have open issue:
There was a problem hiding this comment.
In the latest commit I removed the all, I also moved these sentences above the requirements.
Main changes: