binlore: migrate override lore to package passthru #311811
Conversation
|
@ofborg build resholve.tests |
39250a3
to
5f51680
Compare
|
@ofborg build resholve.tests |
| @@ -125,4 +128,11 @@ stdenv.mkDerivation (finalAttrs: { | |||
| passthru.tests = { | |||
| inherit cmake nix samba; | |||
| }; | |||
|
|
|||
| # bsdtar is detected as "cannot" because its exec is internal to | |||
There was a problem hiding this comment.
Maybe move the comment inside so that it is closer when another lore items are added.
There was a problem hiding this comment.
Is this on the right one? I'm not sure about moving it into the multiline string. I do like moving it closer, but I think I prefer the clearer legibility of it being a comment at the Nix level.
If we need to leave breadcrumbs for more executables, the comments can just be converted into a bulleted list?
| @@ -30,7 +30,9 @@ let | |||
| priority = 10; | |||
| platforms = platforms.${stdenv.hostPlatform.parsed.kernel.name} or platforms.all; | |||
| }; | |||
| passthru = { inherit provider; }; | |||
| passthru = { inherit provider; } // lib.optionalAttrs (builtins.hasAttr "binlore" providers) { | |||
There was a problem hiding this comment.
I wonder how compatible the different providers are. I can imagine GNU adding some extra flags. But we can probably handle that if this becomes a problem.
There was a problem hiding this comment.
Actually, it might make some sense to inherit lore from the individual provider packages.
There was a problem hiding this comment.
If by this you mean push them out into the packages where these all come from, I think I mostly agree.
I tried that first and ended up settling with implementing them here for a few expedience reasons, the foremost of which was not really being able to figure out how to splice it in to some of the macOS commands which are already fairly abstractly sliced out of source releases in a way that makes it hard to tell where and how to intervene and also be able to pair up lore overrides for the commands we care about.
Looking around at the implementation of diskdev_cmds in https://github.com/NixOS/nixpkgs/tree/master/pkgs/os-specific/darwin/apple-source-releases may clarify why I backed off of that (but I'm happy to do it if you can tell how we could accomplish it in a way that's as or more straightforward than what I have now?)
| '' | ||
| for lore_type in ${builtins.toString lore.types}; do | ||
| if [[ -f "${drv}/nix-support/$lore_type" ]]; then | ||
| cat "${drv}/nix-support/$lore_type" >> "$out/$lore_type" |
There was a problem hiding this comment.
Having two different directory paths is weird. Would it be feasible to standardize on e.g. lib/binlore/$lore_type?
There was a problem hiding this comment.
I've waffled a little about where to put this.
Open to changing it (whether here or later) so I'll just think out loud for now:
- I think if we moved it into a
lib/dir it'd be at risk of getting split off into a lib output when it's present (so I guess this is at least somewhat related to your other question about multiple outputs)? - When I was first thinking about doing this I didn't feel like it was my place to take up part of the nix-support namespace and imagined this actually belongs in some new namespace that doesn't imply it's just for nix/nixpkgs internal purposes. I opened an issue in the architecture repo last year but it didn't go anywhere and has been since transferred into this repo (Attaching more kinds of data to packages? #295029).
- In make(Binary)Wrapper: record created wrappers #314937 I'm also tweaking the wrapper generators to output this data, and
nix-support/does feel more semantically appropriate from that perspective. Since this path is ~anticipating the other change, maybe it feels less out-of-place with that in frame?
|
@ofborg build resholve.tests |
|
Looks good to me, but please fix the commit messages and rebase. |
Lore overrides have been included with binlore's source up to now, but this hasn't worked very well. (It isn't as easy to self-service for people working in nixpkgs, and its use of partial pnames for matching breaks down around some edge cases like version numbers appearing early in perl pnames, or multiple packages having identical pnames.)
a48c633
to
8f413d8
Compare
|
Eval error. |
|
Sounds like the problem this one's addressing: |
|
@ofborg eval |
1 similar comment
|
@ofborg eval |
|
Eval is still failing. Maybe you need to rebase? |
|
It looks like the periodic merges into staging have been failing, so I think it isn't in staging yet: https://github.com/NixOS/nixpkgs/actions/workflows/periodic-merge-6h.yml |
|
@ofborg eval |
Move lore overrides from binlore's source repo to the passthru of the target packages.
Description of changes
Change how lore is merged/gathered:
Don't apply overrides from binlore's source.
Merge lore from
$out/nix-support/<loretype>if it exists. Prepares binlore to collect lore generated bymakeWrapperand friends.(Saving that for a follow-up, since it's a huge rebuild that makes this harder to test.)
Merge lore from
<package>.binloreif it exists.(Happy to debate this name. Avoiding
overrideLoresinceoverrideis roughly a contract in nixpkgs.)Add a utility function (
binlore.synthesize) for ~synthesizing lore overrides. It rolls in a small Shell DSL for efficiently expressing overrides.(Happy to debate this name. Avoiding
overridesince it's roughly a contract in nixpkgs.)Reimplement most overrides using
passthru.binlore = binlore.synthesize ...;. (I'm skipping some to pare them back to the set needed for nixpkgs and other public repos found via search.)Expand resholve's passthru tests to better exercise the machinery.
I also have a branch with these changes based on a recent nixpkgs-unstable commit for easier testing. I generally test with
nix-build -A resholve.tests.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.