[nix profile list] parse using --json in new nix version #1308

savil · 2023-07-25T18:58:52Z

Summary

Problem:
Latest nix version of 2.17 changes the output format of nix profile list to be:

Index:              0
Flake attribute:    legacyPackages.aarch64-linux.curl
Original flake URL: github:NixOS/nixpkgs/3c614fbc76fc152f3e1bc4b2263da6d90adf80fb
Locked flake URL:   github:NixOS/nixpkgs/3c614fbc76fc152f3e1bc4b2263da6d90adf80fb
Store paths:        /nix/store/3qcvhxxmgcdj6izfs8d3m8csdlsj92ng-curl-8.1.1-bin /nix/store/6skgivna073ziw95xvbksvd5c8n5vi5w-curl-8.1.1-man

Previously, the "keys" didn't exist in the output. This broke how we were parsing the output.

Fix in pseudo code:

try invoke: nix profile list --json
if error:
  do old code: nix profile list

the older nix versions don't support --json

How was it tested?

in some devbox projects:

# clear old state
> rm -rf .devbox

> devbox shell

# run some commands specific to that project

savil · 2023-07-25T18:59:05Z

Current dependencies on/for this PR:

main
- PR [easy][minor fix] add error object to error message's format string inputs #1307
  - PR [nix profile list] parse using --json in new nix version #1308 👈
    - PR check which nix version is used in CICD cli-tests #1312

This comment was auto-generated by Graphite.

mikeland73 · 2023-07-25T21:56:38Z

internal/nix/nix.go


+// versionRegex parses the output of `nix --version` to capture the semver version string
+// example output: nix (Nix) 2.13.3
+var versionRegex = regexp.MustCompile(`nix\s\(Nix\)\s([0-9]+.[0-9]+.[0-9]+)`)


nit, you can use instead of \s

I think we should make this more forgiving. the second term should be (.*) with trimmed space?

And then you can use strings.TrimPrefix("nix (Nix)") to make this code simpler.

yeah, I'll switch to TrimPrefix

mikeland73 · 2023-07-25T22:00:46Z

internal/nix/nixprofile/profile.go

 ) (map[string]*NixProfileListItem, error) {

-	lines, err := nix.ProfileList(writer, profileDir)
+	version, err := nix.Version()


Don't return error here. That puts us at a risk of failing even when we would do correct thing.

(e.g. what if in some past version nix --version returned something weird, we're better off just trying with useJSON = false)

not sure I follow. What do you mean "failing even when we do correct thing?"

In general, I'd prefer erroring here if we fail to get the nix version properly. Otherwise, we'll error below with a more obscure error...

If we fail to read the version and ignore the error, we can continue doing what we were doing before. Otherwise we'll have introduced a new error and potentially break old nix installations that were already working.

I'd just try running nix profile list --json and if it fails try nix profile list (without the flag). Then you don't need to do the version comparison stuff. Example with old Nix:

$ nix profile list --json error: unrecognised flag '--json' Try 'nix --help' for more information. $ nix profile list 0 flake:nixpkgs#legacyPackages.aarch64-darwin.fuse github:NixOS/nixpkgs/d0f2758381caca8b4fb4a6cac61721cc9de06bd9#legacyPackages.aarch64-darwin.fuse /nix/store/8pqmbsp8zkq8656k5yaravbaxixaaa3c-macfuse-stubs-4.4.1

Here's a concrete example:

Imagine that in nix v2.1.0 running nix --version returns 2.1.0 without the nix (Nix) prefix. Then this function would return an error and we would stop execution. But in this case, version would be "" which would currently set useJSON to false. So it's a case where ignoring the error would cause correct execution but returning an error would cause execution to break.

We don't want to break any old versions of nix, so I think just assuming useJSON is false if we can't parse the version is the correct way to go.

mikeland73 · 2023-07-25T22:09:27Z

internal/nix/nixprofile/profile.go

+	// if version is >= 2.17, we can use the json output
+	useJSON := vercheck.SemverCompare(version, "2.17") >= 0


Use 2.17.0 (2.17 is technically not valid semver)

This defaults to false if version is not semver. Is that OK? (I think probably yes)

ugh... good point

let me check the behavior of this vercheck.SemverCompare for 2.17 like output.

btw, do we know if nix --version prints 2.17 or 2.17.0?

2.17.0

vercheck.SemverCompare seems to work because semver.parse can read versions missing the patch and defaults them to 0. But I'm not sure that a missing patch number is generally considered semver.

savil · 2023-07-25T22:20:53Z

internal/nix/nixprofile/profile.go

+				unlockedReference: element.OriginalURL,
+				lockedReference:   element.URL,


@mikeland73 do we think I got this mapping right?

No, it's not 1 to 1 on either locked or unlocked. I'm taking a look

savil · 2023-07-25T22:33:59Z

internal/nix/nixprofile/profile.go

 	}

+	// if version is >= 2.17.0, we can use the json output
+	useJSON := vercheck.SemverCompare(version, "2.17.0") >= 0


I am somewhat concerned about this failing in an unexpected way. The underlying function call will return:

// An invalid semantic version string is considered less than a valid one. // All invalid semantic version strings compare equal to each other.

According to https://lazamar.co.uk/nix-versions/?channel=nixpkgs-unstable&package=nix, there may be some prerelease versions of nix that have non-standard semver versions.

However, these prerelease versions also appear to be rather old, and not present in newer releases of nix. So, maybe this is okay?

I think this is OK. non semver versions will be considered older which defaults to the non --json.

I think as a follow up after this PR we could:

(only use the --json flag if we're sure version is higher (e.g. both are semver)) and then try to parse things both ways (try one and if it fails try the other)

gcurtis

Looks good. If we can get away with not needing to parse the version, then that would be nice.

gcurtis · 2023-07-25T22:34:58Z

internal/nix/nixprofile/profile.go

 ) (map[string]*NixProfileListItem, error) {

-	lines, err := nix.ProfileList(writer, profileDir)
+	version, err := nix.Version()


I'd just try running nix profile list --json and if it fails try nix profile list (without the flag). Then you don't need to do the version comparison stuff. Example with old Nix:

$ nix profile list --json error: unrecognised flag '--json' Try 'nix --help' for more information. $ nix profile list 0 flake:nixpkgs#legacyPackages.aarch64-darwin.fuse github:NixOS/nixpkgs/d0f2758381caca8b4fb4a6cac61721cc9de06bd9#legacyPackages.aarch64-darwin.fuse /nix/store/8pqmbsp8zkq8656k5yaravbaxixaaa3c-macfuse-stubs-4.4.1

savil · 2023-07-25T22:52:08Z

@gcurtis @mikeland73 PTAL: simplified to "try with --json" and "fallback to non-json way upon any error"

savil · 2023-07-25T23:19:55Z

internal/nix/nixprofile/profile.go

+	}
+
+	output, err = nix.ProfileList(writer, profileDir, false /*useJSON*/)
+	if err == nil {


ugh, this should be err != nil

…ckedReference with json output

## Summary Since we saw CLI format change (See #1308) we should test against many nix versions. Possible followup: In `main` should we run all nix versions? ## How was it tested? CICD

sentry-io · 2023-08-01T05:41:31Z

Suspect Issues

This pull request has been deployed and Sentry has observed the following issues:

‼️ **exec.Error: <redacted errors.withStack>: error running "nix profile list": <redacted exec.Error>: <redacted... go.jetpack.io/devbox/internal/nix in ProfileList View Issue
‼️ **exec.Error: <redacted errors.withStack>: error running "nix profile list": <redacted exec.Error>: <redacted... go.jetpack.io/devbox/internal/nix in ProfileList View Issue
‼️ **exec.ExitError: <redacted errors.withStack>: <redacted errors.withStack>: error running "nix profile list": <re... go.jetpack.io/devbox/internal/nix in ProfileList View Issue
‼️ **exec.ExitError: <redacted errors.withStack>: error running "nix profile list": <redacted exec.ExitError> go.jetpack.io/devbox/internal/nix in ProfileList View Issue
‼️ *exec.ExitError: error running command in Devbox: <redacted errors.withStack>: error running "nix profile list": ... go.jetpack.io/devbox/internal/nix in ProfileList View Issue

_{Did you find this useful? React with a 👍 or 👎}

savil mentioned this pull request Jul 25, 2023

[easy][minor fix] add error object to error message's format string inputs #1307

Merged

Base automatically changed from savil/nix-profile-error to main July 25, 2023 19:54

print for debugging

48b6e9e

savil force-pushed the savil/nix-profile-error-debug branch 2 times, most recently from 4189b2a to e90ae04 Compare July 25, 2023 21:46

savil changed the title ~~print for debugging~~ [nix profile list] parse using --json in new nix version Jul 25, 2023

savil force-pushed the savil/nix-profile-error-debug branch from e90ae04 to 2fd0217 Compare July 25, 2023 21:49

adding nix version, and using --json for nix profile list

cf23256

savil force-pushed the savil/nix-profile-error-debug branch from 2fd0217 to cf23256 Compare July 25, 2023 21:51

fix profile list output for older nix versions

4e1b2ee

mikeland73 reviewed Jul 25, 2023

View reviewed changes

savil commented Jul 25, 2023

View reviewed changes

simplify nix.Version, and use 2.17.0 for semver comparison

ea03fb1

savil requested review from gcurtis, mikeland73 and ipince and removed request for gcurtis July 25, 2023 22:31

savil marked this pull request as ready for review July 25, 2023 22:32

savil commented Jul 25, 2023

View reviewed changes

add vercheck_test for SemverCompare

2c4fc03

gcurtis approved these changes Jul 25, 2023

View reviewed changes

try --json first, and fallback to non json upon any error

c6aa2fd

savil requested a review from gcurtis July 25, 2023 22:51

gcurtis approved these changes Jul 25, 2023

View reviewed changes

savil commented Jul 25, 2023

View reviewed changes

fix bug with error check; append attrPath to unlockedReference and lo…

72df9dd

…ckedReference with json output

savil mentioned this pull request Jul 25, 2023

check which nix version is used in CICD cli-tests #1312

Closed

mikeland73 approved these changes Jul 25, 2023

View reviewed changes

mikeland73 mentioned this pull request Jul 25, 2023

[tests] Add test for old nix versions #1313

Merged

savil merged commit 2cfe2cf into main Jul 25, 2023

savil deleted the savil/nix-profile-error-debug branch July 25, 2023 23:54

savil mentioned this pull request Oct 24, 2023

[Bug]: devbox generate direnv fails when profile list is empty. #1585

Closed

		// if version is >= 2.17, we can use the json output
		useJSON := vercheck.SemverCompare(version, "2.17") >= 0

		unlockedReference: element.OriginalURL,
		lockedReference: element.URL,

[nix profile list] parse using --json in new nix version #1308

[nix profile list] parse using --json in new nix version #1308

Uh oh!

Conversation

savil commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How was it tested?

Uh oh!

savil commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeland73 Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gcurtis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savil commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sentry-io bot commented Aug 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Suspect Issues

Uh oh!

Uh oh!

savil commented Jul 25, 2023 •

edited

Loading

savil commented Jul 25, 2023 •

edited

Loading

mikeland73 Jul 25, 2023 •

edited

Loading

savil commented Jul 25, 2023 •

edited

Loading

sentry-io bot commented Aug 1, 2023 •

edited

Loading