Skip to content

bug fix: FillNarInfoCache for versioned packages when adding package #1437

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

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

savil
Copy link
Collaborator

@savil savil commented Aug 31, 2023

Summary

The call to versionedPkg.ValidateExists() would error because the NarInfoCache
was not pre-filled before we called IsInBinaryCache in it.

Now, we call FillNarInfoCache on the versioned packages as well.

Did some minor refactoring so that we only do FillNarInfoCache for packages that
are to-be-added (thereby omitting existing packages) to be just a bit efficient.

How was it tested?

export DEVBOX_FEATURE_REMOVE_NIXPKGS=1

devbox add vim
# BEFORE: this would add `vim` to devbox.json
# AFTER: this adds `vim@latest` to devbox.json

Copy link
Collaborator Author

savil commented Aug 31, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@savil savil requested review from mikeland73 and ipince August 31, 2023 13:21
@savil savil merged commit db9134f into main Aug 31, 2023
@savil savil deleted the savil/fill-narinfo-cache-versioned branch August 31, 2023 15:42
mikeland73 added a commit that referenced this pull request Sep 8, 2023
…1463)

## Summary

Restores functionality implemented by
#1264 and refactored in
#1279

which was accidentally broken by
#1437

tldr: the `FillNarInfoCache` function breaks if packages are not in
search, but we support non-search packages via fallback (.e.g.
stdenv.cc.cc.lib)

I'm keeping surface area as small as possible, but as a follow up I
think we can remove the `devpkg.FillNarInfoCache` calls from `impl.Add`
if we simply ignore the`IsInBinaryCache` errors in
[ValidateExists](https://github.com/jetpack-io/devbox/blob/main/internal/devpkg/validation.go#L10-L26)
or remove `IsInBinaryCache` from that function completely (I'm not
convinced it is needed, @savil @ipince correct me if I'm wrong)

## How was it tested?
`devbox add stdenv.cc.cc.lib`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants