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

wasmer 1.0.0, build with path and compiler flags, update test #68603

Closed
wants to merge 1 commit into from

Conversation

richiksc
Copy link
Contributor

@richiksc richiksc commented Jan 9, 2021

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Created with brew bump-formula-pr, then manually edited. There is already a PR for wasmer 1.0.0 (#68341), but without the needed formula changes.

wasmer 1.0.0 has moved their cli project into the lib/cli directory, and now you must specify which compiler wasmer must be built with. cranelift is the default compiler provided by Wasmer in their release binaries, and supports both ARM64 and x86_64.

The output of wasmer run --invoke is also much less verbose in Wasmer 1.0.0, so the test assertion had to be updated accordingly.

Closes #68341.

@carlocab, you asked me to tag you so you could add the 'CI-force-arm' label and review this.

@BrewTestBot BrewTestBot added no ARM bottle Formula has no ARM bottle rust Rust use is a significant feature of the PR or issue labels Jan 9, 2021
@carlocab carlocab added the CI-force-arm [DEPRECATED] Don't pass --skip-unbottled-arm to brew test-bot. label Jan 9, 2021
@carlocab
Copy link
Member

carlocab commented Jan 9, 2021

Thanks, @richiksc!

@richiksc
Copy link
Contributor Author

richiksc commented Jan 9, 2021

🎉 Builds and tests succesfully on ARM CI!

@carlocab
Copy link
Member

carlocab commented Jan 9, 2021

What are the different options for the --features flag, and what do they do? It seems there are many possible ones, including jit, llvm, native...

@richiksc
Copy link
Contributor Author

richiksc commented Jan 9, 2021

@carlocab Should we build with the --release flag? Looks like it's not part of std_cargo_args (it should be, in my opinion).

@carlocab
Copy link
Member

carlocab commented Jan 9, 2021

cargo install builds with the release profile by default. It's cargo build that you have to specify it for explicitly

@richiksc
Copy link
Contributor Author

richiksc commented Jan 9, 2021

cargo install builds with the release profile by default. It's cargo build that you have to specify it for explicitly

@carlocab Oh, I didn't know that, sorry! Good to know.

@richiksc
Copy link
Contributor Author

richiksc commented Jan 9, 2021

@carlocab Here is the list of possible options for the --features flag:
https://github.com/wasmerio/wasmer/blob/master/lib/cli/Cargo.toml#L50.

These features are enabled by default, unless the --no-default-features flag is passed in.

default = [
    "wat",
    "wast",
    "jit",
    "native",
    "object-file",
    "cache",
    "wasi",
    "emscripten"
]

The recommended way to build in Wasmer docs is make build-wasmer. That auto-detects compilers based on system availability, and the only values it passes into --features are $(compiler_features), which is the list of supported compilers.

The cranelift compiler seems to be enabled for all builds, regardless of system architecture or OS. If it can find LLVM installed on the system through llvm-config, it'll add llvm to the compilers list. On x64 systems, the singlepass compiler is added as well.

So, we are already building with the features listed in default, and cranelift is the default universal compiler that works on all architectures.

A few questions arise:

  1. Should we install LLVM as a dependency in the Homebrew build so we build with llvm compiler support? Is there anyway to use the system LLVM/clang? Wasmer's CI seems to build with llvm on macOS-x86_64 but without on macOS-arm64. Looking at issues, it looks like it requires LLVM 10, but the llvm Homebrew formula only supports llvm@11 and llvm@9.
  2. Should we enable singlepass as a compiler on macOS-x86_64 builds? It would be closer to the official build.
  3. Should we just build with make build-wasmer instead of cargo install? This would build with the recommended options for the host system by default, however, the LLVM question still arises. make build-wasmer uses cargo build --release instead of cargo install under the hood, and I don't know if that's a problem for Homebrew or not.

@carlocab
Copy link
Member

carlocab commented Jan 9, 2021

Thanks for the thorough research!

Should we install LLVM as a dependency in the Homebrew build so we build with llvm compiler support?

I'd rather not add features that pull in large dependencies unless they're specifically requested by users.

Should we enable singlepass as a compiler on macOS-x86_64 builds? It would be closer to the official build.

What does the singlepass compiler do? If it doesn't increase the build size too much, I suppose it should be fine.

Should we just build with make build-wasmer instead of cargo install?

Do they use --locked when doing cargo build? If they do then maybe we can...

@richiksc
Copy link
Contributor Author

richiksc commented Jan 9, 2021

What does the singlepass compiler do?

It compiles in a "single pass", meaning it doesn't make any optimizations. For this compiler, compile time is much faster but execution time is slower. A full explanation of the three compilers can be found here.

Do they use --locked when doing cargo build? If they do then maybe we can...

Unfortunately, no, they don't use --locked. We could either ask them to use --locked / open a PR (a logical change to me, but I'm not a wasmer maintainer), or we'd have to continue using cargo install.

@carlocab
Copy link
Member

carlocab commented Jan 9, 2021

It compiles in a "single pass", meaning it doesn't make any optimizations.

If it's part of their default distribution I guess it couldn't hurt to include it.

we'd have to continue using cargo install.

Let's go with this; the documentation of the --locked flag for cargo build is a bit unclear, so I'm actually not certain as to what it does exactly... In fact, I'm fairly certain you don't want the --locked flag for cargo build, as that's when you want your dependencies to be resolved.

@SMillerDev
Copy link
Member

the documentation of the --locked flag for cargo build is a bit unclear, so I'm actually not certain as to what it does exactly

It uses all versions from a cargo.lock file (I think that's the name) instead of getting the most recent ones.

@carlocab
Copy link
Member

carlocab commented Jan 9, 2021

It uses all versions from a cargo.lock file

That's what I understand too, but looking at man cargo-build isn't super clear about that. You also actually want to run cargo build at least once without the --locked flag, since that run is when you will generate the Cargo.lock file for distribution. This means that an upstream PR specifying --locked in their build system is probably the wrong way to go.

@SMillerDev
Copy link
Member

Unless that commit adds a lockfile

@richiksc
Copy link
Contributor Author

richiksc commented Jan 9, 2021

What about a PR to add a "install-wasmer" task to the Makefile? It would call cargo install with --locked and reuse the same compiler_features variable.

@carlocab
Copy link
Member

carlocab commented Jan 9, 2021

Unless that commit adds a lockfile

To make up for the fact that the PR breaks their build system by stopping it from generating Cargo.lock files? That seems like the wrong way to go about it.

What about a PR to add a "install-wasmer" task to the Makefile?

This sounds good. I think switching to that doesn't need to block this PR, however. Can you open the appropriate PR to update this formula when that has been settled upstream?

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks again for your first contribution to Homebrew, @richiksc! I hope this is the first of many similarly high-quality PRs.

@richiksc
Copy link
Contributor Author

richiksc commented Jan 9, 2021

@carlocab Heads up, looks like you forgot to trigger a merge. Thanks for all your help and quick responses!

@chenrui333
Copy link
Member

Unfortunately, no, they don't use --locked. We could either ask them to use --locked / open a PR (a logical change to me, but I'm not a wasmer maintainer), or we'd have to continue using cargo install.

maybe we should file a pr to update the lockfile.

@chenrui333
Copy link
Member

Thanks @richiksc!

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@richiksc
Copy link
Contributor Author

richiksc commented Jan 9, 2021

Unfortunately, no, they don't use --locked. We could either ask them to use --locked / open a PR (a logical change to me, but I'm not a wasmer maintainer), or we'd have to continue using cargo install.

maybe we should file a pr to update the lockfile.

It would be a bad idea to use --locked in the make build-wasmer task, due to the reasons outlined in #68603 (comment). If you're referring to updating Cargo.lock itself, it is already up to date and the lockfile in its current form builds succesfully on all CI with --locked.

@richiksc richiksc deleted the bump-wasmer-1.0.0 branch January 9, 2021 18:17
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 12, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-force-arm [DEPRECATED] Don't pass --skip-unbottled-arm to brew test-bot. no ARM bottle Formula has no ARM bottle outdated PR was locked due to age rust Rust use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants