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

remove near-fmt dep from near-vm-runner #9266

Merged
merged 8 commits into from
Jul 11, 2023

Conversation

Ekleog-NEAR
Copy link
Collaborator

Part of #8197

This changes the behavior, in ways that hopefully would not be problematic.

@jakmeier AFAICT you’re one of the last ones to have touched io traces, WDYT about this change?

@Ekleog-NEAR Ekleog-NEAR requested a review from a team as a code owner June 29, 2023 15:56
@Ekleog-NEAR Ekleog-NEAR added C-housekeeping Category: Refactoring, cleanups, code quality S-automerge labels Jun 29, 2023
@jakmeier
Copy link
Contributor

Oh btw, I just realized that near-fmt is already published to crates.io, just like near-o11y.
I guess it's still good to remove the dependency but I'm not sure why this was strictly necessary.

@near-bulldozer near-bulldozer bot merged commit dc39893 into near:master Jul 11, 2023
@Ekleog-NEAR
Copy link
Collaborator Author

Oh btw, I just realized that near-fmt is already published to crates.io, just like near-o11y.
I guess it's still good to remove the dependency but I'm not sure why this was strictly necessary.

So technically, as near-fmt and near-o11y do not appear in the API of contract runtime (except for the format of log messages, which is actually the reason for this review, so… ^^’), it would have been possible to keep the dependency there.

However, both of them are currently published only as an internal detail because we need to publish near-vm-runner for sandbox and similar out-of-nearcore usage. So we often bump the major version, and it’d be iffy to try to sync that up with a separately-published version of contract runtime that’d have its own semver guarantees.

TLDR: we could have lived with the deps and make things somehow work in this specific case (because they don’t appear in the API of contract runtime), but it’d mean iffy handling of version upgrades if we didn’t decide to actually stabilize near-fmt into a proper crate in some way or form, similarly to what we did with finite-wasm.

nikurt pushed a commit that referenced this pull request Jul 12, 2023
Part of #8197

This changes the behavior, in ways that hopefully would not be problematic.

@jakmeier AFAICT you’re one of the last ones to have touched io traces, WDYT about this change?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-housekeeping Category: Refactoring, cleanups, code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants