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

fix(c-api) Rename lib's name to wasmer #2457

Merged
merged 2 commits into from
Jul 8, 2021
Merged

fix(c-api) Rename lib's name to wasmer #2457

merged 2 commits into from
Jul 8, 2021

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Jul 8, 2021

Description

This patch does several things.

  1. For the crate wasmer-c-api, the library name is modified from
    wasmer_c_api to wasmer in Cargo.toml. That way, the new
    library files are named libwasmer.* rather than
    libwasmer_c_api.*. That's the primaly goal of this patch. The
    rest is a consequence of this point. Why do we want that? Because
    the build.rs script of the wasmer-c-api crate will configure
    the soname (on Linux), the install_name + current_version +
    compatibility_version (on macOS), and the out-implib +
    output-def (on Windows) for a library named libwasmer, which is
    the name we provide in the Wasmer package for the Wasmer
    libraries. If we want everything to be testable, we cannot use
    libwasmer in soname for a file named libwasmer_c_api for
    example. If we rename the file when packaging (as it's done prior
    this patch), we would need to re-update all those information in
    the Makefile. It implies to duplicate the code in 2 places. So
    let's do things right directly and only once: We want the library
    to be named libwasmer, let's do that.

  2. For the crate wasmer-c-api, since the library name has been
    renamed to wasmer, it creates a conflict with the wasmer crate
    which is a dependency. Consequently, this patch also updates the
    Cargo.toml file to modifiy the dependency name with the special
    package attribute (see
    https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#renaming-dependencies-in-cargotoml
    to learn more). So now, the wasmer refers to the wasmer_c_api
    crate, and wasmer-api refers to the wasmer crate.

  3. The code of the wasmer-c-api crate is updated accordingly. The
    WasmerEnv derive procedural macro fails because it expects a
    crate named wasmer (which is now wasmer_api), so we implement
    the WasmerEnv trait by hand.

  4. The patch updates the build.rs script of the wasmer-c-api
    crate:

    1. In the build_cdylib_link_arg function: The dependency to the
      cdylib-link-lines crate has been removed because the output is
      not exactly the one we expect. So we compute all the
      cargo:rustc-cdylib-link-arg=… lines by hand. The version number
      no longer appears in the library file name for example.

    2. In the build_inline_c_env_vars function: Values passed to
      LDFLAGS have been updated to be libwasmer rather than
      libwasmer_c_api.

    3. A new shared_object_dir helper function has been created
      because it's used in build_inline_c_env_vars and in
      build_cdylib_link_arg.

  5. The Makefile has been updated:

    1. In package-capi, we no longer rename libwasmer_c_api to
      libwasmer since the name is correctly defined since the
      beginning now.

    Calling install_name_tool on macOS is no longer required since
    install_name is correctly set by the linker in the build.rs
    script of wasmer-c-api.

    1. In package-docs, some stuffs have been fixed, like the
      SOURCE_VERSION variable that didn't exist, so removed, or the
      mkdir command that was incorrect etc.

    2. In build-docs, the wasmer-c-api crate is excluded from the
      list of crates to generate the documentation for. Mostly because
      the build-docs-capi recipe exists, and we must use it to
      generate the documentation of wasmer-c-api now.

    3. In build-docs-capi, we generate the documentation for the
      wasmer-c-api crate. But rustdoc uses the library name for the
      directory name in the target/doc/ directory. Since the library
      name is now wasmer, it creates a conflict with the wasmer
      crate. Consequently, we update the library name by using sed on
      the Cargo.toml file before running cargo doc, to finally
      restore Cargo.toml as it was previously.

Fixes #2429.

Review

  • Add a short description of the change to the CHANGELOG.md file

This patch does several things.

1. For the crate `wasmer-c-api`, the library name is modified from
   `wasmer_c_api` to `wasmer` in `Cargo.toml`. That way, the new
   library files are named `libwasmer.*` rather than
   `libwasmer_c_api.*`. That's the primaly goal of this patch. The
   rest is a consequence of this point. Why do we want that? Because
   the `build.rs` script of the `wasmer-c-api` crate will configure
   the `soname` (on Linux), the `install_name` + `current_version` +
   `compatibility_version` (on macOS), and the `out-implib` +
   `output-def` (on Windows) for a library named `libwasmer`, which is
   the name we provide in the Wasmer package for the Wasmer
   libraries. If we want everything to be testable, we cannot use
   `libwasmer` in `soname` for a file named `libwasmer_c_api` for
   example. If we rename the file when packaging (as it's done prior
   this patch), we would need to re-update all those information in
   the `Makefile`. It implies to duplicate the code in 2 places. So
   let's do things right directly and only once: We want the library
   to be named `libwasmer`, let's do that.

2. For the crate `wasmer-c-api`, since the library name has been
   renamed to `wasmer`, it creates a conflict with the `wasmer` crate
   which is a dependency. Consequently, this patch also updates the
   `Cargo.toml` file to modifiy the dependency name with the special
   `package` attribute (see
   https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#renaming-dependencies-in-cargotoml
   to learn more). So now, the `wasmer` refers to the `wasmer_c_api`
   crate, and `wasmer-api` refers to the `wasmer` crate.

3. The code of the `wasmer-c-api` crate is updated accordingly. The
   `WasmerEnv` derive procedural macro fails because it expects a
   crate named `wasmer` (which is now `wasmer_api`), so we implement
   the `WasmerEnv` trait by hand.

4. The patch updates the `build.rs` script of the `wasmer-c-api`
   crate:

  1. In the `build_cdylib_link_arg` function: The dependency to the
     `cdylib-link-lines` crate has been removed because the output is
     not exactly the one we expect. So we compute all the
     `cargo:rustc-cdylib-link-arg=…` lines by hand. The version number
     no longer appears in the library file name for example.

  2. In the `build_inline_c_env_vars` function: Values passed to
     `LDFLAGS` have been updated to be `libwasmer` rather than
     `libwasmer_c_api`.

  3. A new `shared_object_dir` helper function has been created
     because it's used in `build_inline_c_env_vars` and in
     `build_cdylib_link_arg`.

5. The `Makefile` has been updated:

  1. In `package-capi`, we no longer rename `libwasmer_c_api` to
     `libwasmer` since the name is correctly defined since the
     beginning now.

     Calling `install_name_tool` on macOS is no longer required since
     `install_name` is correctly set by the linker in the `build.rs`
     script of `wasmer-c-api`.

  2. In `package-docs`, some stuffs have been fixed, like the
     `SOURCE_VERSION` variable that didn't exist, so removed, or the
     `mkdir` command that was incorrect etc.

  3. In `build-docs`, the `wasmer-c-api` crate is excluded from the
     list of crates to generate the documentation for. Mostly because
     the `build-docs-capi` recipe exists, and we must use it to
     generate the documentation of `wasmer-c-api` now.

  4. In `build-docs-capi`, we generate the documentation for the
     `wasmer-c-api` crate. But `rustdoc` uses the library name for the
     directory name in the `target/doc/` directory. Since the library
     name is now `wasmer`, it creates a conflict with the `wasmer`
     crate. Consequently, we update the library name by using `sed` on
     the `Cargo.toml` file before running `cargo doc`, to finally
     restore `Cargo.toml` as it was previously.
@Hywan
Copy link
Contributor Author

Hywan commented Jul 8, 2021

bors try

bors bot added a commit that referenced this pull request Jul 8, 2021
@Hywan
Copy link
Contributor Author

Hywan commented Jul 8, 2021

On macOS:

$ objdump -macho -dylib-id target/release/libwasmer.dylib
target/release/libwasmer.dylib:
@rpath/libwasmer.dylib

install_name is correct set without calling install_name_tool in the Makefile.

@bors
Copy link
Contributor

bors bot commented Jul 8, 2021

try

Build failed:

@Hywan
Copy link
Contributor Author

Hywan commented Jul 8, 2021

bors try

bors bot added a commit that referenced this pull request Jul 8, 2021
@bors
Copy link
Contributor

bors bot commented Jul 8, 2021

Comment on lines -612 to -613
echo '<!-- Build $(SOURCE_VERSION) --><meta http-equiv="refresh" content="0; url=crates/wasmer/index.html">' > package/docs/index.html
echo '<!-- Build $(SOURCE_VERSION) --><meta http-equiv="refresh" content="0; url=wasmer/index.html">' > package/docs/crates/index.html
Copy link
Member

Choose a reason for hiding this comment

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

Why did we remove <!-- Build $(SOURCE_VERSION) -->?

This is super useful for debugging the html and see which version was built with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's not defined :-). This variable doesn't exist.

@Hywan
Copy link
Contributor Author

Hywan commented Jul 8, 2021

bors r+

Comment on lines -111 to +119
#[derive(wasmer::WasmerEnv, Clone)]
#[derive(Clone)]
#[repr(C)]
struct WrapperEnv {
env: *mut c_void,
env_finalizer: Arc<Option<wasm_env_finalizer_t>>,
}

impl wasmer_api::WasmerEnv for WrapperEnv {}

Copy link
Member

Choose a reason for hiding this comment

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

Ummm.... of everything this is the thing that I think could be more inconsistent. I'm concerned it might brake some things as derive(wasmer::WasmerEnv) has more implications in general than just the impl.

For wasmer_js we also have this problem (where WasmerEnv is defined in wasmer_js). Perhaps we can have a wasmer_derive feature flag to use one or other master crate name?

Also, since wasmer_api::WasmerEnv works... why it wouldn't work this?

    #[derive(wasmer_api::WasmerEnv, Clone)]
    #[repr(C)]
    struct WrapperEnv {
        env: *mut c_void,
        env_finalizer: Arc<Option<wasm_env_finalizer_t>>,
    }

@syrusakbary syrusakbary mentioned this pull request Jul 8, 2021
@bors
Copy link
Contributor

bors bot commented Jul 8, 2021

@bors bors bot merged commit 5f30188 into master Jul 8, 2021
@bors bors bot deleted the fix-c-api-soname branch July 8, 2021 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 bot Bip bip 📚 documentation Do you like to read? 🎉 enhancement New feature! 📦 lib-c-api About wasmer-c-api 🧪 tests I love tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linux: Missing SONAME in "libwasmer.so" hinders usage in CMake
2 participants