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

Implement wasmer config #3428

Merged
merged 37 commits into from
Dec 22, 2022
Merged

Implement wasmer config #3428

merged 37 commits into from
Dec 22, 2022

Conversation

fschutt
Copy link
Contributor

@fschutt fschutt commented Dec 15, 2022

Fixes #3383.

@fschutt fschutt requested a review from syrusakbary as a code owner December 15, 2022 15:51
@ptitSeb
Copy link
Contributor

ptitSeb commented Dec 16, 2022

I would prefer a simpler:
config XXX instead of config get XXX
and
set-config XXX YYY instead of config set XXX YYY

But yeah, the switch from config --XXX is a bit strange to me, as it's the common way to query config on various linux tools (pkg-config, llvm-config, sdl2-config...).

That's just personnal taste, nothing blocking.

@ptitSeb ptitSeb self-requested a review December 16, 2022 14:00
tests/integration/cli/tests/config.rs Show resolved Hide resolved
lib/registry/src/config.rs Outdated Show resolved Hide resolved
lib/registry/src/config.rs Outdated Show resolved Hide resolved
lib/cli/src/commands/config.rs Outdated Show resolved Hide resolved
lib/c-api/README.md Outdated Show resolved Hide resolved
lib/c-api/README.md Outdated Show resolved Hide resolved
lib/c-api/README.md Outdated Show resolved Hide resolved
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

wasmer config --pkg-config and others (--include-dir) should NOT live in wasmer config get in any case. wasmer config get {KEY} are for settings that can be configured by the user via the counterpart wasmer config set {KEY} {VALUE}.

We need to preserve old behavior for the previously existing config flags

@fschutt
Copy link
Contributor Author

fschutt commented Dec 20, 2022

@syrusakbary the default config now looks like this:

wax_cooldown = 0
telemetry_enabled = false
update_notifications_enabled = false

[registry]
active_registry = "https://registry.wapm.dev/graphql"
tokens = []

[proxy]
  1. Do we still need the wax_cooldown, since we don't have wax anymore?
  2. We need to watch out that the switch from waom.toml to wasmer.toml is as smooth as possible:

toml::from_str(&config_toml).map_err(|e| format!("could not parse {path:?}: {e}"))

... will fail if we do non-backwards compatible breaking changes to the wasmer.toml. Would be better to return Self::default() here in the error case.

@fschutt fschutt requested a review from syrusakbary December 20, 2022 10:46
@syrusakbary
Copy link
Member

Do we still need the wax_cooldown, since we don't have wax anymore?

Probably not, but we should assure that when renaming the old wapm.toml to wasmer.toml the reading doesn't crash

@fschutt fschutt enabled auto-merge December 22, 2022 10:37
@fschutt fschutt dismissed theduke’s stale review December 22, 2022 12:26

Changes implemented

Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

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

LGTM, relying on @syrusakbary sign off yesterday.

@fschutt fschutt merged commit c1b8f20 into master Dec 22, 2022
@fschutt fschutt deleted the config branch December 22, 2022 13:32
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.

Implement wasmer config
4 participants