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

binlore: migrate override lore to package passthru #304956

Closed

Conversation

abathur
Copy link
Member

@abathur abathur commented Apr 18, 2024

Background

Note: trying to keep this from sprawling; there's a longer background/motivation in the issue below if you want more context:

resholve ~needs metadata about which executables are wrappers and which might exec their args.

I implemented these concepts in a separate ~package called binlore because I suspect there are other uses of this (and similar) metadata (and I didn't want to bog down on a quest to add metadata of unproven utility to core nixpkgs machinery).

Aside: I named this metadata lore. I'm not married to the name, but I do think it needs something to avoid confusion with package.meta. I'll use lore from here on.

binlore uses YARA rules and post-processing logic to scan executables and generate lore. It also merges manual overrides (if available) with generated lore.

(The current implementation is quick but not very smart, so it needs manual overrides when generated lore is wrong.)

Problem

The manual overrides mentioned above are bundled with binlore's source, but this has scaled poorly. Moving them into nixpkgs is the goal of this PR.

The main reasons it scales poorly are:
  • People working on nixpkgs can't readily self-service fixes/improvements.

  • Some obvious limitations of the longest-pname-prefix-match lore override mechanism have started to bite in past several months.

  • Now that xdg-utils is packaged with resholve, updates to overrides in binlore's source must go through staging.

Description

This PR moves lore overrides from binlore's source repo to the passthru of the target packages.

Broad strokes:

  • Change how lore is merged/gathered:

    • Stop applying overrides from binlore's source.

    • Merge lore from a package's $out/nix-support/<loretype> if it exists. This mechanism prepares binlore to collect lore generated by makeWrapper and friends.

      (I'm not including those changes here. It's a huge rebuild that makes all of this harder to test, so I'll save it for a follow-up PR.)

    • Merge lore from <package>.lore if it exists.

      (Happy to debate this name. I shied away from using overrideLore since override is more or less a contract in nixpkgs by this point and I don't want to mislead.)

  • 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. I again shied away from using override since that term is more or less a contract in nixpkgs by this point.)

  • Reimplement most overrides using passthru.lore = binlore.synthesize ...;. (I'm not porting all overrides. I'm paring 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.

TODO

Opened as draft to gather feedback. Will probably reopen against staging when it feels ~ready. For now, hoping to figure out:

  • roughly how much consensus/consent this will take
  • how much documentation this needs and where it should be
  • whether any names/terms should change (ideally before I write docs per above)

My branch for this is based on a recent nixpkgs-unstable commit and I generally test with nix-build -A resholve.tests --arg config '{ allowBroken = false; }'.

@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Apr 18, 2024
@ofborg ofborg bot requested a review from peterhoeg April 18, 2024 03:03
@abathur abathur changed the title binlore: migrate override lore to package passthru, resholve: expand passthru tests binlore: migrate override lore to package passthru Apr 18, 2024
@abathur abathur force-pushed the move_lore_overrides_to_passthru branch from fb7090f to 496add0 Compare April 18, 2024 03:23
@abathur abathur removed the request for review from peterhoeg April 18, 2024 12:09
@abathur
Copy link
Member Author

abathur commented Apr 18, 2024

@ofborg build resholve.tests

@abathur abathur force-pushed the move_lore_overrides_to_passthru branch from 496add0 to b8f5d9b Compare April 18, 2024 13:34
@ofborg ofborg bot requested a review from peterhoeg April 18, 2024 14:10
@abathur
Copy link
Member Author

abathur commented Apr 18, 2024

@ofborg build resholve.tests

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.)
@abathur abathur force-pushed the move_lore_overrides_to_passthru branch from b8f5d9b to 865a718 Compare April 18, 2024 14:48
@abathur
Copy link
Member Author

abathur commented Apr 18, 2024

@ofborg build resholve.tests

@abathur abathur removed the request for review from peterhoeg April 19, 2024 14:04
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 3, 2024
@abathur
Copy link
Member Author

abathur commented May 4, 2024

There's a merge conflict, but that's fine for now--it doesn't affect the main questions here, and I already plan to close and reopen against staging once this is actually ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants