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.

Sign up for GitHub

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

Jump to bottom

wasix: Switch Console to WasiRunner + Various WASI Improvements #3996

Closed
wants to merge 16 commits into from
Closed

wasix: Switch Console to WasiRunner + Various WASI Improvements #3996

wants to merge 16 commits into from

Conversation

theduke
Copy link
Contributor

@theduke theduke commented Jun 13, 2023 •
edited
Loading

This PR switches the wasix Console to use the WasiRunner instead of custom
setup.

This removes code duplication, and prevents potential resulting from differences
in bootstrapping for WASI environments.

To make this happen, the PR also extends the WasiRunner with an option to provide
a custom TmpFileSystem.

NOTE: this PR is based on #3989, which
must be merged first.

  • chore: Remove unused name_map from WasiFs
  • wasix: Allow passing a custom root fs to WasiRunner/CommonWasiOptions
  • wasix: Switch the Console over to the WasiRunner
  • chore: remove redundant webc_runner[_wasi] features
  • feat(wasix): Extend Runtime trait with module loading/compiling

@theduke theduke requested a review from syrusakbary as a code owner June 13, 2023 19:58
@theduke theduke requested a review from Michael-F-Bryan June 13, 2023 20:10
@theduke theduke marked this pull request as draft June 13, 2023 20:10
@theduke theduke force-pushed the wasix-console-improvements branch 5 times, most recently from f852325 to dad91ed Compare June 13, 2023 22:40
Michael-F-Bryan
Michael-F-Bryan suggested changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan left a comment

Choose a reason for hiding this comment

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

we're setting up WasiRunner correctly as far as I can tell. The optional root_fs in CommonWasiOptions::prepare_webc_env() is a bit of a code smell, but I think that says more about the complexity of what we're trying to do and the limitations of our filesystem layer.

I've left a couple trivial suggestions about using structured logs a bit more because it'll help when spelunking through logs later on.

I don't think we should merge this without adding some tests to the Console, though. Try adding a test for something non-trivial like python/python to make sure the filesystem is being set up properly. Unit tests in the console module would be best, but I understand Console isn't in the most testable state at the moment.

lib/wasix/src/lib.rs Outdated Show resolved Hide resolved
lib/wasix/src/runtime/mod.rs Outdated Show resolved Hide resolved
lib/wasix/src/bin_factory/exec.rs Outdated Show resolved Hide resolved
lib/wasix/src/bin_factory/exec.rs Outdated Show resolved Hide resolved
lib/wasix/src/bin_factory/exec.rs Outdated Show resolved Hide resolved
@theduke
Copy link
Contributor Author

theduke commented Jun 14, 2023

I don't think we should merge this without adding some tests to the Console, though.

Depending on when we merge this, I think we can also just remove the console now.

It is only used by Deploy, and now it has become a very simple wrapper that does almost nothing, so I should be able to switch Deploy away from it.

@theduke theduke force-pushed the wasix-console-improvements branch 3 times, most recently from bc16131 to 84fe8da Compare June 21, 2023 03:23
@theduke theduke mentioned this pull request Jun 21, 2023
Merged
@Michael-F-Bryan
Copy link
Contributor

@theduke when you get back to this PR, can you apply the change mentioned in #4019 (review)?

@theduke theduke force-pushed the wasix-console-improvements branch from f27ccbd to 16124c0 Compare June 21, 2023 17:38
@theduke theduke requested a review from Michael-F-Bryan June 21, 2023 17:46
@theduke theduke force-pushed the wasix-console-improvements branch from b53917d to f14f07a Compare June 21, 2023 17:53
@theduke theduke marked this pull request as ready for review June 22, 2023 07:17
Michael-F-Bryan
Michael-F-Bryan approved these changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan left a comment

Choose a reason for hiding this comment

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

I've left some suggestions, but there's nothing blocking this PR from merging 👍

lib/cli/src/commands/run.rs Outdated Show resolved Hide resolved
lib/wasix/src/bin_factory/exec.rs Outdated Show resolved Hide resolved
lib/wasix/src/lib.rs Outdated Show resolved Hide resolved
lib/wasix/src/lib.rs Outdated Show resolved Hide resolved
lib/wasix/src/os/console/mod.rs Outdated Show resolved Hide resolved
lib/wasix/src/os/console/mod.rs Outdated Show resolved Hide resolved
lib/wasix/src/runners/wasi_common.rs Outdated Show resolved Hide resolved
lib/wasix/tests/runners.rs Outdated Show resolved Hide resolved
@theduke theduke force-pushed the wasix-console-improvements branch 3 times, most recently from 3cc1331 to b8a15ff Compare June 27, 2023 07:24
@theduke theduke force-pushed the wasix-console-improvements branch from b8a15ff to 681686d Compare July 5, 2023 08:31
theduke added 6 commits July 5, 2023 11:47
@theduke
chore: Remove unused name_map from WasiFs
7c78673 
Doesn't seem to be used anywhere, so it's removed...
@theduke
wasix: Allow passing a custom root fs to WasiRunner/CommonWasiOptions
a699a7b 
Needed in some cases for customization of the root fs.
@theduke
wasix: Switch the Console over to the WasiRunner
fcf59c8 
Remove code duplication by reusing code paths
@theduke
chore: remove redundant webc_runner[_wasi] features
2985c5b 
Remove he webc_runner and webc_runner_rt_wasi features from the wasmer_wasix crate.

These features do not require any additional dependencies, and just add
complexity for no good reason.

There was historical motivation for these features, because the
wasmer::Engine did not support module compilation on all target
architectures in the past, but now it is no longer neccessary.
@theduke
feat(wasix): Extend Runtime trait with module loading/compiling
c98af52 
Add Runtime::load_module / Runtime::load_module_sync methods that are
responsible for loading (compiling + caching) Webassembly modules.

This allows implementers to customize module compilation and caching,
for example to prevent concurrent compilation of the same module.
@theduke
wasix: use common Runtime::laod_module helper in spawn_exec
cc16f31 
Reduce code duplicaton.
theduke added 7 commits July 5, 2023 11:47
@theduke
Use async load_module() to prevent tokio nested runtime panic
d17a144
@theduke
chore: improve logging in bin_factory
c31327f
@theduke
tests(wasix): Add execution test for Console
4065b41 
Adds a simple test that runs dash through the console.
@theduke
wasix: Improve SpawnError::Other
1da1ac2 
Use Box<dyn Error> instead of a string.

Also adds an AnyhowStdError wrapper to allow converting an anyhow::Error to a
Box<dyn Error>.

Needed for better error handling/propagation.
@theduke
cli: Use Runtime::load_module_sync in runners
71a9b9b 
Remove duplicated code.
@theduke
tests(wasix): Constraint Console test
5240d34
@theduke
chore(wasix): Cleanup of various review suggestions
63d223d
@theduke theduke force-pushed the wasix-console-improvements branch from 681686d to 63d223d Compare July 5, 2023 09:47
@theduke
tests: Fix console dash test
c275362 
Needed to enable the package loader.
@theduke theduke enabled auto-merge July 5, 2023 14:08
@theduke
Copy link
Contributor Author

theduke commented Jul 21, 2023

Commits from this PR were included in #4050 and hence are already merged.

@theduke theduke closed this Jul 21, 2023
auto-merge was automatically disabled July 21, 2023 11:05

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Michael-F-Bryan Michael-F-Bryan Michael-F-Bryan approved these changes

@syrusakbary syrusakbary Awaiting requested review from syrusakbary

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@theduke @Michael-F-Bryan