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

More interesting dyn drv test case #12425

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ericson2314
Copy link
Member

Motivation

The existing ones did not shine enough light on this feature.

Context

depends on #9415


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

This fixes dynamic derivations, reverting NixOS#9081.

I believe that this time around, NixOS#9052 is fixed. When I first rebased
this, tests were failing (which wasn't the case before). The cause of
those test failures were due to the crude job in which the outer goal
tried to exit with the inner goal's status.

Now, that error handling has been reworked to be more faithful. The exit
exit status and exception of the inner goal is returned by the outer
goal. The exception was what was causing the test failures, but I
believe it was not having the right error code (there is more than one
for failure) that caused NixOS#9081.

The only cost of doing things the "right way" was that I had to
introduce a hacky `preserveException` boolean. I don't like this, but,
then again, none of us like anything about how the scheduler works.
Issue NixOS#11927 is still there to clean everything up, subsuming the need
for any `preserveException` because I doubt we will be fishing
information out of state machines like this at all.

This reverts commit 8440afb.

Co-Authored-By: Eelco Dolstra <[email protected]>
@Ericson2314 Ericson2314 requested a review from edolstra as a code owner February 5, 2025 23:53
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Feb 5, 2025
@Ericson2314 Ericson2314 force-pushed the more-interesting-dyn-drv-test-case branch from 8158351 to 841c18b Compare February 6, 2025 04:59
@Ericson2314
Copy link
Member Author

Ericson2314 commented Feb 7, 2025

I'm not saying we should def add python to the testsuite deps, but this is a lot easier to read:

with import ./config.nix;

builtins.outputOf (mkDerivation {
  name = "make-derivations.drv";

  requiredSystemFeatures = [ "recursive-nix" ];

  buildCommand = ''
    import os
    import json
    import subprocess

    os.environ["PATH"] = os.pathsep.join([os.getenv("NIX_BIN_DIR", ""), os.environ["PATH"]])
    os.environ["NIX_CONFIG"] = "extra-experimental-features = nix-command ca-derivations dynamic-derivations"

    deps = {
        "a": [],
        "b": ["a"],
        "c": ["a"],
        "d": ["b", "c"],
        "e": ["b", "c", "d"],
    }

    # Cannot just literally include this, or Nix will think it is the
    # *outer* derivation that's trying to refer to itself, and
    # substitute the string too soon.
    placeholder = subprocess.check_output(["nix", "eval", "--raw", "--expr", "builtins.placeholder \"out\""]).decode().strip()

    drvs = {}
    for word in ["a", "b", "c", "d", "e"]:
        input_drvs = {}

        for dep in deps[word]:
            input_drvs[drvs[dep]] = {
                "outputs": ["out"],
                "dynamicOutputs": {},
            }

        json_data = {
            "args": ["-c", f"set -xeu; echo \"word env var {word} is ${word}\" >> \"$out\""],
            "builder": os.environ["shell"],
            "env": {
                "out": placeholder,
                word: f"hello, from {word}!",
                "PATH": json.dumps(os.environ["PATH"]),
            },
            "inputDrvs": input_drvs,
            "inputSrcs": [],
            "name": f"build-{word}",
            "outputs": {
                "out": {
                    "method": "nar",
                    "hashAlgo": "sha256",
                },
            },
            "system": os.environ["system"],
        }

        drvs[word] = subprocess.check_output(["nix", "derivation", "add"], input=json.dumps(json_data).encode()).decode().strip()

    with open(os.environ["out"], "w") as out_file:
        out_file.write(drvs["e"])
  '';

  __contentAddressed = true;
  outputHashMode = "text";
  outputHashAlgo = "sha256";
}).outPath "out"

(thank you chat gpt)

@Ericson2314 Ericson2314 force-pushed the more-interesting-dyn-drv-test-case branch from ba36989 to b0db834 Compare February 12, 2025 20:06
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

I wouldn't mind having python in the test suite, as it is already part of the build anyway. It doesn't increase the bootstrapping closure, and tests are not required for that anyway (as if a second argument is relevant)

Comment on lines 24 to 27
# Cannot just literally include this, or Nix will think it is the
# *outer* derivation that's trying to refer to itself, and
# substitute the string too soon.
placeholder=$(nix eval --raw --expr 'builtins.placeholder "out"')
Copy link
Member

Choose a reason for hiding this comment

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

If this is a common occurrence, I guess we could create placeholders that unwrap into other placeholders.

@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

@Ericson2314 Ericson2314 force-pushed the more-interesting-dyn-drv-test-case branch from b0db834 to 820df19 Compare February 12, 2025 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants