Skip to content

resholve: use originalSrc for nixpkgs-update bot#191354

Merged
thiagokokada merged 1 commit intoNixOS:masterfrom
abathur:resholve_fix_outputhash
Sep 17, 2022
Merged

resholve: use originalSrc for nixpkgs-update bot#191354
thiagokokada merged 1 commit intoNixOS:masterfrom
abathur:resholve_fix_outputhash

Conversation

@abathur
Copy link
Copy Markdown
Member

@abathur abathur commented Sep 15, 2022

Description of changes

Effort to fix automatic nixpkgs-update updates for resholved packages in 9f6310d (#191003) did help the bot get further along, but it still failed to find the source outputHash (because the outer derivation's source is the inner derivation; the bot is looking for outer.src.outputHash but before this commit it is at outer.src.src.outputHash). (log)

Change abuses passthru to set the outer src to inner.src in a way that (IIUC) will not affect the build, but will affect ~queries like nixpkgs-update is using (e.g. nix eval -f . --raw pkgs.bats.src.drvAttrs.outputHash)

I'm not at all certain this will satisfy nixpkgs-update; treat me with some skepticism :)

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

This one's a bit weird, so I've confirmed that:

  • nixpkgs-review reports No diff detected, stopping review...

  • I can nix-build both bats and bats.tests (bats is built with resholve)

  • The nixpkgs-update nix eval command I'm trying to fix appears to work:

    $ nix eval -f . --raw pkgs.bats.src.drvAttrs.outputHash
    sha256-joNne/dDVCNtzdTQ64rK8GimT+DOWUa7f410hml2s8Q=

@abathur
Copy link
Copy Markdown
Member Author

abathur commented Sep 15, 2022

@thiagokokada Hoping you can take a look at this since you reviewed the previous PR, but no worries if you don't have time.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Sep 15, 2022
Copy link
Copy Markdown
Contributor

@thiagokokada thiagokokada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the better solution here would be to just have a default passthru.updateScript that knows how to handle the outputHash from the resholve derivations.

Effort to fix automatic nixpkgs-update updates for resholved
packages in 9f6310d did help the bot get further, but it
then failed to find the source outputHash (the outer
derivation's source is the inner derivation; bot looks for
outer.src.outputHash; ours is at outer.src.src.outputHash).

This change uses `originalSrc` to indicate the source of the
inner derivation. Along with nix-community/nixpkgs-update#324, this
enables the bot to fall back on an attr that Nix/nixpkgs are
not directly depending on, supporting automatic updates for
packages built with `resholve.mkDerivation`.
@abathur abathur force-pushed the resholve_fix_outputhash branch from 4b6704f to ff44e1f Compare September 17, 2022 20:27
@abathur
Copy link
Copy Markdown
Member Author

abathur commented Sep 17, 2022

The force-push I just made takes this in another direction, but I want to document a little of the ground covered just in case the record is useful later. Feel free to skip this comment and just read the next one :)

I think the better solution here would be to just have a default passthru.updateScript that knows how to handle the outputHash from the resholve derivations.

I got similar advice from @ryantm, so I spent a few hours banging my head against this with nothing to show for it. Unless I'm missing one or more things (likely, since the documentation here seems to be vaporware :) the majority of all update scripts I can find as examples are extremely idiomatic to the source/release infrastructure of individual packages.

I've tried using the most-generic updaters I've been able to find: genericUpdater and nix-update-script. I haven't been able to get anything that would just work for downstream users of resholve.mkDerivation (the former because it looks like every instance would require per-package customization to use, the latter for exactly the same kind of src-finding trouble.)

Again assuming I'm not missing something, the updateScript approach feels ~right for individual packages, and like a minefield for a builder (unless/until we build something universal, canonical, and well-documented within Nixpkgs that can help cope with edge-case builders).

@abathur
Copy link
Copy Markdown
Member Author

abathur commented Sep 17, 2022

I ended up going with your suggestion to see about fixing this at the nixpkgs-update level in nix-community/nixpkgs-update#324.

This PR no longer engages in misleading sourcery--it uses a new attr that shouldn't be load-bearing in Nix or nixpkgs. :)

@abathur abathur changed the title resholve: use src.outputHash from inner derivation resholve: use originalSrc for nixpkgs-update bot Sep 17, 2022
Comment on lines +188 to +189
# fallback attr for update bot to query our src
originalSrc = unresholved.src;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should document this somewhere. Looks like it could be useful for a few other use cases.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed for this PR though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should document this somewhere. Looks like it could be useful for a few other use cases.

I agree, though I'm at a bit of a loss for where since nixpkgs-update doesn't seem to be mentioned in the nixpkgs manual or the nixos wiki.

I feel like there could/should probably be a whole section there, or maybe a distinct guide, that covers the broad topic of propagating updates to nixpkgs. At least:

  • how to write and run update scripts both bespoke or via genericUpdater, nix-update-script, and any others
  • code-generation best-practices for nix-ecosystem projects that need to maintain an in-tree copy of code they develop out of tree
  • notes on the existence of the update bot, how to help it update your package, etc.

I have at least opened a PR over at nixpkgs-update to make it a little more discoverable: nix-community/nixpkgs-update#325

@thiagokokada thiagokokada merged commit 099436f into NixOS:master Sep 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants