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

updated Warg and refactored #6

Merged
merged 7 commits into from
May 14, 2024
Merged

Conversation

calvinrp
Copy link
Collaborator

@calvinrp calvinrp commented May 9, 2024

No description provided.

crates/wasm-pkg-loader/src/config.rs Outdated Show resolved Hide resolved
Comment on lines 25 to 48
let config = ClientConfig::from_default_file()?;
let is_missing_config = config.is_none();
let mut config = config.unwrap_or_default();

// if default registry is not configured, fallback to Warg home URL
if config.get_default_registry().is_none() {
match warg_client::Config::from_default_file()? {
Some(warg_client::Config { home_url, .. }) if home_url.is_some() => {
let url = url::Url::parse(&home_url.unwrap())?;
config.default_registry(
url.host_str()
.ok_or_else(|| anyhow!("invalid Warg home url"))?,
);
}
_ => {}
};
}

// if not configured and not using Warg home URL as default registry,
// then set the `wasi` default registry
if is_missing_config && config.get_default_registry().is_none() {
config.namespace_registry("wasi", "bytecodealliance.org");
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's discuss this in #3

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just caught up on that thread. I think this is in inline with what was discussed there.

If global wasm-pkg-tools config file is not defined but the global warg config file is defined, then it falls back to using the warg config given this tool can use the warg_client.

If there is no global config for either, it uses bytecodealliance.org.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having two different sources for "default registry" interacting this way seems potentially confusing. It might make sense to consolidate config between warg and this library a bit to end up with a similar UX; that's what I'm trying to hash out in the discussion thread.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Complete agreement. Do you think this implementation makes sense for this PR? And then we work on the rest in follow on PRs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want to land this kind of fundamental config logic without discussion: #10

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lann I think we have a path forward on the config, from our discussions. As for this PR and this config, how should we proceed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just revert this chunk of the PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lann Reverted

crates/wasm-pkg-loader/src/source/warg.rs Outdated Show resolved Hide resolved
calvinrp added a commit to bytecodealliance/registry that referenced this pull request May 9, 2024
- This allows us to have a simpler
`FileSystemClient::new_with_default_config()` that gets the expected
`auth_token`.

Would simplify
bytecodealliance/cargo-component#288,
bytecodealliance/wac#103,
bytecodealliance/wasm-pkg-tools#6,
esoterra/wow#3

- Added client download as a stream methods
- Validates the content digest as downloading from the registry
@lann
Copy link
Collaborator

lann commented May 14, 2024

Oh yeah sorry I snuck some CI in behind your back there 😅

@calvinrp calvinrp merged commit 56a2190 into bytecodealliance:main May 14, 2024
1 check passed
@calvinrp calvinrp deleted the updated-warg branch May 14, 2024 15:37
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