[sandboxes] devbox ls: print versions #2277
Merged
+40
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gcurtis
approved these changes
Sep 17, 2024
| // Continue to print the package even if we can't resolve the version | ||
| // so that the user can see the error for this package, as well as get the | ||
| // results for the other packages | ||
| resolvedVersion = "<error resolving version>" |
There was a problem hiding this comment.
Maybe omit the version from the list and print it on stderr at the end?
mikeland73
approved these changes
Sep 17, 2024
Comment on lines
+835
to
+839
| if err := p.resolve(); err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
If package is not in lockfile this is just current resolved version, but that's probably ok.
| @@ -848,6 +848,11 @@ func (d *Devbox) AllPackageNamesIncludingRemovedTriggerPackages() []string { | |||
| return result | |||
| } | |||
|
|
|||
| func (d *Devbox) AllPackagesIncludingRemovedTriggerPackages() []*devpkg.Package { | |||
There was a problem hiding this comment.
Change AllPackageNamesIncludingRemovedTriggerPackages to use this?
There was a problem hiding this comment.
done
There's a small subtle difference that I think is okay in practice, but let me call it out.
- In
AllPackageNamesIncludingRemovedTriggerPackages, thep.VersionedName()will append the Version if it exists - In devpkg's Package.Versioned(), if the version is missing we'll append
@latest
I think this doesn't matter in practice, because we always append @latest during devbox add <name>.
|
|
||
| // Print the resolved version, unless the user has specified a version already | ||
| if strings.HasSuffix(pkg.Versioned(), "latest") && resolvedVersion != "" { | ||
| // Runx packages have a "v" prefix (why?). Trim for consistency. |
There was a problem hiding this comment.
technically semver has (or can have?) a prefix v
|
@savil Is this ready to be merged? |
savil
force-pushed
the
savil/list-versions
branch
from
8373b7f
to
642058b
Compare
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
mikeland73
added a commit
that referenced
this pull request
Oct 1, 2024
## Summary Fixes issue introduced in #2277 This comment explains it pretty well: https://github.com/jetify-com/devbox/pull/2277/files#r1767085866 except: > I think this doesn't matter in practice, because we always append @latest during devbox add <name>. This is not 100% true. In the very small number of packages that are not in nixhub but are valid attribute paths, (e.g. `stdenv.cc.cc.lib`) we store the key in the lockfile without `@latest` :(. The key missmatch caused lockfile.Tidy to remove entries. My bad for making the suggestion that lead to this, this was really subtle difference. ## How was it tested? Added `stdenv.cc.cc.lib` on devbox.json linux only, installed on linux and copied over lockfile to macos. Did `devbox install` and ensured lockfile was not modified.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
For packages without an explicit version, this PR will print the version for
the user in
devbox ls.Fixes #2274
How was it tested?
Notice that versions are printed for go@latest and the runx packages, but
not for cowsay since it was added via
devbox add [email protected]Also,
devbox ls -c examples/flakes/phpanddevbox ls -c examples/flakes/remote