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

The wasmer crate's sys, js, and jsc feature flags expose an inconsistent public API #4033

Closed
Michael-F-Bryan opened this issue Jun 26, 2023 · 2 comments · Fixed by #4398
Closed
Assignees
Labels
priority-low Low priority issue
Milestone

Comments

@Michael-F-Bryan
Copy link
Contributor

Michael-F-Bryan commented Jun 26, 2023

Describe the bug

As part of #4031 I use wasmer::Artifact::is_deserializable() to detect whether a compiled module has been LZW compressed or not. The make test-js build is currently failing because Artifact is only available with the sys target, not js and jsc.

error[E0433]: failed to resolve: could not find `Artifact` in `wasmer`
   --> lib/wasix/src/runtime/module_cache/filesystem.rs:159:16
    |
159 |     if wasmer::Artifact::is_deserializable(&leading_bytes) {
    |                ^^^^^^^^ could not find `Artifact` in `wasmer`

(GitHub Actions log)

The root cause is that we aren't selective when re-exporting types from underlying implementations. We'll keep having these issues as long as we're doing pub use sys::*.

wasmer/lib/api/src/lib.rs

Lines 448 to 464 in 77898a7

#[cfg(feature = "sys")]
mod sys;
#[cfg(feature = "sys")]
pub use sys::*;
#[cfg(feature = "js")]
mod js;
#[cfg(feature = "js")]
pub use js::*;
#[cfg(feature = "jsc")]
mod jsc;
#[cfg(feature = "jsc")]
pub use jsc::*;

I'm guessing this regression was originally introduced in #3556 and we didn't notice it when JSC support was added in #3825.

Proposed Solution

The best way to fix this would be to change the way we re-export things to always re-export the same types. That way backend-specific items like Artifact won't be part of our universal wasmer API.

cfg_if::cfg_if! {
  if #[cfg(feature = "sys")] {
    pub mod sys;
    use sys as imp;
  } else if #[cfg(feature = "js")] {
    pub mod js;
    use js as imp;
  } else if #[cfg(feature = "jsc")] {
    pub mod jsc;
    use jsc as imp;
  } else {
    // Use a fallback implementation
     #[path = "unsupported/mod.rs"]
    mod imp;
  }
}

pub use imp::{Module, Instance, ...};

This has the added benefit that users will still be able to access items from the sys, js, and jsc modules as an escape hatch. We'll also get those nice "Available on crate feature sys only" annotations from rustdoc if we set #![cfg_attr(docsrs, feature(doc_cfg))].

Given we literally just released Wasmer 4.0 with a bunch of breaking changes and this will remove items from wasmer's public API, I don't think this is feasible.

Alternative Solution

Another solution is to create a dummy implementation of wasmer::Artifact (and all the other missing types) that is used whenever the js and jsc feature flags are active.

This is similar to how the Rust standard library has a std::sys module containing all OS-specific details, and whenever a particular target doesn't implement something it'll fall back to the implementation in std::sys::unsupported.

Alternative Solution 2

A quickfix could be to mark the re-export of Artifact as deprecated, saying people should use the wasmer_compiler crate directly.

pub use wasmer_compiler::{Artifact, EngineBuilder, Features, Tunables};

This approach kinda works, but it'll become a constant game of wack-a-mole as long as those pub use sys::* exports exist.

@Michael-F-Bryan Michael-F-Bryan changed the title The wasmer::Artifact type isn't available with the js or jsc features The wasmer crate's sys, js, and jsc feature flags expose an inconsistent public API Jun 26, 2023
@theduke
Copy link
Contributor

theduke commented Jun 26, 2023

I do believe that was on purpose/sort of makes sense, because there is no artifact in the JS (c?) targets.

@ptitSeb ptitSeb added the priority-low Low priority issue label Jun 27, 2023
@ptitSeb ptitSeb modified the milestones: v5.0, v4.1 Jun 27, 2023
@syrusakbary
Copy link
Member

syrusakbary commented Jun 27, 2023

We have this on the api/engine.rs:

#[cfg(feature = "sys")]
pub use wasmer_compiler::{Artifact, CompilerConfig, EngineInner, Features, Tunables};

Ideally, we should have this exports as part of the sys submodule in the wasmer api instead of the root.

What we can do is to create wrapper types (just via pub type =...) and add the deprecated macro decorator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-low Low priority issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants