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

Allow diff-closures to output json #7243

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

Conversation

Pamplemousse
Copy link
Member

No description provided.

@edolstra
Copy link
Member

What does the JSON output look like?

@Pamplemousse
Copy link
Member Author

Here's an excerpt of what I can get with outputs/out/bin/nix --extra-experimental-features nix-command store diff-closures /run/current-system /nix/store/5ks2pk9n0lwmck32552ysjf1d7662pmv-nixos-system-foo-22.05.20221023.471d921

{
  "bash-interactive": {
    "after": "",
    "before": "",
    "sizeDelta": "+6384.3"
  },
  "bind": {
    "after": "9.18.7",
    "before": "9.18.3",
    "sizeDelta": "+44.8"
  },
  "blas": {
    "after": "",
    "before": "3",
    "sizeDelta": "-56241.8"
  },
  "chrootenv": {
    "after": "ε",
    "before": "",
    "sizeDelta": "+17.5"
  },
 ...
}

@SuperSandro2000
Copy link
Member

    "after": "ε",
    "before": "",

Can we use null, empty string or something that everyone knows how to type on their keyboard?

@edolstra
Copy link
Member

edolstra commented Nov 1, 2022

Yeah, using null sounds good.

sizeDelta should just be a number, not a string, and should be the delta in bytes.

before and after should probably be versionBefore and versionAfter, to make clear what it's referring to.

It would be nice to include the before/after store paths that are being compared.

@Pamplemousse
Copy link
Member Author

Can we use null, empty string or something that everyone knows how to type on their keyboard?

I favored consistency and kept the symbols in use in the "regular" display.

@Pamplemousse
Copy link
Member Author

It would be nice to include the before/after store paths that are being compared.

I was just looking for a mean to exploit the data that is currently being rendered by diff-closures automatically, not necessarily try to "better" the output of the command.

@fricklerhandwerk fricklerhandwerk added UX The way in which users interact with Nix. Higher level than UI. new-cli Relating to the "nix" command feature Feature request or proposal labels Mar 3, 2023
@Ericson2314
Copy link
Member

It would be cool to finish this! Nice to have JSON for all commands.

@Pamplemousse
Copy link
Member Author

It would be cool to finish this!

I addressed the comments from the PR, either by updating the code, or by explaining why the display makes use of certain symbols.

I didn't get any extra feedback here, and have asked for some on Matrix months ago without any replies.
As far as I can tell, this is "finished".

@Ericson2314
Copy link
Member

I enqueued this on the project board to try to get some feedback from the rest of the team.

@thufschmitt thufschmitt added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Jul 28, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-07-24-nix-team-meeting-minutes-75/31112/1

@bew
Copy link
Contributor

bew commented Jul 31, 2023

When the derivation name does not strictly follow the name-version format, diff-closure usually displays wrong derivation name (or "").
For exemple when making custom intermediate derivations for my configs, adding a version doesn't really make sense, and adding a -0 or -unversioned seems like a workaround for nice display.

To help downstream users of this json, have you considered adding the Nix store paths for before/after in addition to only the version?

That would also allow doing more than just display the before/after versions and size delta, like using external diff tools on the paths to get more diff information.

Pamplemousse added 2 commits October 31, 2023 08:44
Signed-off-by: Pamplemousse <git@xaviermaso.com>
Signed-off-by: Pamplemousse <git@xaviermaso.com>
Copy link
Contributor

@iFreilicht iFreilicht left a comment

Choose a reason for hiding this comment

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

In general I like this change, but I do have one critique about the output format. Granted, I'm not a maintainer, so my feedback isn't as binding as that of others might be.

"after": "20.08.1",
"before": "20.08.2",
"sizeDelta": 13900
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I share the concern that @SuperSandro2000 originally raised about these symbols not being typable on most keyboards, but I also understand that just null is not explicit enough here because there are two different ways of a version not existing. As "after" and "before" can also contain multiple versions separated by commas, I would propose the following solution:

Make "after" and "before" lists of strings. (aka no version of this package exists/existed) is represented by an empty array [], and ε (aka an empty version of this package exists/existed) is represented by an array containing a single empty string [ "" ].

I feel this would make the output much more idiomatic to the semantics of JSON.

If you decide not to do this, I think it would at least be useful to add examples with , ε and multiple versions here to show that the JSON output is exactly like the printed one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea.

However, I feel we should keep the two outputs (JSON, and regular print) consistent.

What do the other maintainers think about changing ε for [ "" ] and for [] in the old display?

@edolstra edolstra removed their assignment Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation feature Feature request or proposal idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. new-cli Relating to the "nix" command UX The way in which users interact with Nix. Higher level than UI.
Projects
Status: 🏁 Review
Development

Successfully merging this pull request may close these issues.

None yet

9 participants