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

Get rid of impureOutputHash; fix possible bug #6346

Merged
merged 1 commit into from
Feb 12, 2025

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 <[email protected]>
@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