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

fix(warg): Removes call to cache for versions #72

Merged

Conversation

thomastaylor312
Copy link
Collaborator

The implementation for listing versions was pulling from the cache. This caused it to miss updates to versions when being used in cargo-component.

I discovered this while working on cargo-component and it was blocking tests from working

@thomastaylor312
Copy link
Collaborator Author

Note that I also pushed the fix for non-https urls here from #68 so it was decoupled from that

let url = warg_meta.url.unwrap_or_else(|| {
// If we just pass registry plain, warg will assume it is https. This is a workaround to
// assume that a local registry is http.
if registry.host() == "localhost" || registry.host() == "127.0.0.1" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

add "0.0.0.0"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

0.0.0.0 is a weird one because it means all interfaces for some things and I think it is rejected (maybe?) on Windows

@calvinrp
Copy link
Collaborator

Could you point me to where in cargo-component that you are running into the cache issue? I just want to confirm that we should always skip the cache vs explicitly doing a update() before package() (https://github.com/bytecodealliance/registry/blob/main/crates/client/src/lib.rs#L621-L622).

@thomastaylor312
Copy link
Collaborator Author

Calling update is probably the better choice here. Still working my way around the API

@thomastaylor312
Copy link
Collaborator Author

@calvinrp So it looks like the update method updates all packages rather than just a specific one being fetched. That feels like it could be possibly expensive operation to run every time someone wants to fetch a package. Is that still the best option? I feel like fetching only for a specific package every time is a bit more predictable. I just don't know if it is going to cause heavy load on a server to list all packages to a client and then update the checkpoints (even if those checkpoints are empty data updates)

@calvinrp
Copy link
Collaborator

@thomastaylor312 Sorry, I'm just trying to understand why the caching is problem. On how this being called.

The implementation for listing versions was pulling from the cache. This
caused it to miss updates to versions when being used in cargo-component.

I discovered this while working on cargo-component and it was blocking
tests from working

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
@thomastaylor312
Copy link
Collaborator Author

@calvinrp and I synced offline. For now we are going to call update every time, but I left a comment explaining possible tradeoffs for later

@thomastaylor312 thomastaylor312 merged commit eed0684 into bytecodealliance:main Jul 16, 2024
1 check passed
@thomastaylor312 thomastaylor312 deleted the fix/cached_versions branch July 16, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants