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

exportReferencesGraph started including a valid: true field #10485

Open
flokli opened this issue Apr 12, 2024 · 4 comments
Open

exportReferencesGraph started including a valid: true field #10485

flokli opened this issue Apr 12, 2024 · 4 comments
Assignees
Labels
bug regression Something doesn't work anymore

Comments

@flokli
Copy link
Contributor

flokli commented Apr 12, 2024

Describe the bug

I started writing some types for the reference graph structure that Nix emits when its exportReferencesGraph feature is used.

I initially included the valid boolean (which seems to be always true).

I then triggered the build on another machine running an older version of Nix (2.3), and my parsing code failed, as the valid field was not present.

While probably not too much code relies on this, and instead looks at individual fields it knows to be present, we should produce the same JSON across versions. It'd be good if this output stays consistent across versions, and the additional field in there gets removed again.

Steps To Reproduce

Build stdenv.mkDerivation { name = "hello"; __structuredAttrs = true; exportReferencesGraph.blub = [ pkgs.hello ]; nativeBuildInputs = [pkgs.jq]; buildCommand = "jq -rc .blub $NIX_ATTRS_JSON_FILE > $out"; } on a machine with current Nix, and on one with Nix 2.3. Observe how the resulting output differs and the Nix version used is observable through that.

Priorities

Add 👍 to issues you find important.

@flokli flokli added the bug label Apr 12, 2024
@Kranzes
Copy link
Member

Kranzes commented Apr 12, 2024

Potentially caused by #9242

@flokli
Copy link
Contributor Author

flokli commented Apr 12, 2024

That PR description even mentions the stability in structured attrs:

This keeps the human oriented CLI logic (which is currently stable) and the core domain logic (export reference graphs with structured attrs, which is stable), separate, which I think is better.

@tomberek
Copy link
Contributor

tomberek commented Apr 14, 2024

New output has a "valid" field.

  {
    "closureSize": 2216152,
    "narHash": "sha256:0z1mdik09afiskvxjzcxl60sq0rzvh7lx6a7hxs2q1s379r6n1n5",
    "narSize": 358784,
    "path": "/nix/store/w4djxksksd1p8m054k537plqnif5858k-libidn2-2.3.4",
    "references": [
      "/nix/store/4r64z7v5l40pg6r0hd169bcs85c8c42b-libunistring-1.1",
      "/nix/store/w4djxksksd1p8m054k537plqnif5858k-libidn2-2.3.4"
    ],
    "valid": true
  }

Perhaps just:

diff --git a/src/libstore/parsed-derivations.cc b/src/libstore/parsed-derivations.cc
index 72f45143d..09801919f 100644
--- a/src/libstore/parsed-derivations.cc
+++ b/src/libstore/parsed-derivations.cc
@@ -151,8 +151,6 @@ static nlohmann::json pathInfoToJSON(
         // Add the path to the object whose metadata we are including.
         jsonPath["path"] = store.printStorePath(storePath);

-        jsonPath["valid"] = true;
-
         jsonPath["closureSize"] = ({
             uint64_t totalNarSize = 0;
             StorePathSet closure;

@thufschmitt thufschmitt added the regression Something doesn't work anymore label Apr 15, 2024
@Ericson2314
Copy link
Member

Ericson2314 commented Apr 24, 2024

This issue has been present since f86f2b9 (merged Mar 2, 2023), when someone thought they were just updating the command-line interface but actually were also changing this feature.

My #9242 tried to separate these two use-cases somewhat, which just made the oddity more visible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug regression Something doesn't work anymore
Projects
Status: ⚖ To discuss
Development

No branches or pull requests

5 participants