Skip to content

Get rid of impureOutputHash; fix possible bug#6346

Merged
Ericson2314 merged 1 commit intoNixOS:masterfrom
Ericson2314:impure-derivations-ng
Feb 12, 2025
Merged

Get rid of impureOutputHash; fix possible bug#6346
Ericson2314 merged 1 commit intoNixOS:masterfrom
Ericson2314:impure-derivations-ng

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Mar 31, 2022

Motivation

I do not believe there is any problem with computing hashDerivationModulo the normal way with impure derivations.

Conversely, the way this used to work is very suspicious because two almost-equal derivations that only differ in depending on different impure derivations could have the same drv hash modulo. That is very suspicious because there is no reason to think those two different impure derivations will end up producing the same content-addressed data!

Context

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or bug fix: updated release notes

@edolstra
Copy link
Member

edolstra commented Apr 1, 2022

The point of making DerivationType::ContentAddressed a record (and derivation type no longer an enum) was that we already handled this case and didn't need a new variant.

I don't agree that this already handles that case. An impure derivation is not conceptually the same as a derivation that isn't sandboxed and not fixed-output. You could imagine derivations that are not sandboxed and not fixed-output, but are still expected to have a reproducible result (e.g. a derivation that uses ccache to build something remotely).

@Ericson2314
Copy link
Member Author

We conversely, one might want to cache actually impure derivations more normally, in a "per transaction" substitution map.

Still, I will split this up so the second commit doesn't hold back the first.

@Ericson2314 Ericson2314 force-pushed the impure-derivations-ng branch from aa99703 to 27b5d2a Compare April 1, 2022 13:39
@Ericson2314 Ericson2314 changed the title Simplify the new impure derivations implementation Get rid of impureOutputHash Apr 1, 2022
@stale stale bot added the stale label Nov 1, 2022
@Ericson2314 Ericson2314 force-pushed the impure-derivations-ng branch from 1094e42 to 76692cc Compare January 16, 2023 20:57
@stale stale bot removed stale labels Jan 16, 2023
@Ericson2314 Ericson2314 force-pushed the impure-derivations-ng branch from 76692cc to 643e782 Compare January 16, 2023 21:03
@Ericson2314 Ericson2314 changed the title Get rid of impureOutputHash Get rid of impureOutputHash; fix possible bug Jan 16, 2023
@edolstra edolstra self-assigned this Feb 3, 2023
@Ericson2314
Copy link
Member Author

In the meeting today @edolstra mentioned that, as we discussed in the past, we don't necessary want to make impure derivations work "just like" CA derivations.

A thing I forgot to point out was that floating CA derivations are already not handled specially. The only special case in hashDerivationModulo is for fixed CA derivations -- input address and floating CA are handled the same way.

Impure derivations are certainly not fixed output, so I don't think we are going to run into any problems here.

@fricklerhandwerk fricklerhandwerk added the store Issues and pull requests concerning the Nix store label Mar 3, 2023
@edolstra edolstra removed their assignment Apr 26, 2024
@tomberek
Copy link
Contributor

tomberek commented Oct 9, 2024

@Ericson2314
Copy link
Member Author

Yeah, but low priority.

I do not believe there is any problem with computing
`hashDerivationModulo` the normal way with impure derivations.

Conversely, the way this used to work is very suspicious because two
almost-equal derivations that only differ in depending on different
impure derivations could have the same drv hash modulo. That is very
suspicious because there is no reason to think those two different
impure derivations will end up producing the same content-addressed
data!

Co-authored-by: Alain Zscheile <zseri.devel@ytrizja.de>
@Ericson2314 Ericson2314 force-pushed the impure-derivations-ng branch from 643e782 to 50912d0 Compare February 12, 2025 06:37
@Ericson2314 Ericson2314 merged commit 0abc264 into NixOS:master Feb 12, 2025
12 checks passed
@Ericson2314 Ericson2314 deleted the impure-derivations-ng branch February 12, 2025 20:22
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-02-12-nix-team-meeting-minutes-212-5/60216/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

store Issues and pull requests concerning the Nix store

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants