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

cargo publish doesn't work on wasmer-c-api #1802

Closed
nlewycky opened this issue Nov 6, 2020 · 4 comments
Closed

cargo publish doesn't work on wasmer-c-api #1802

nlewycky opened this issue Nov 6, 2020 · 4 comments
Assignees
Labels
bug Something isn't working 📦 lib-c-api About wasmer-c-api

Comments

@nlewycky
Copy link
Contributor

nlewycky commented Nov 6, 2020

Describe the bug

scripts/publish.py runs cargo publish on every crate in Wasmer. This fails to publish the wasmer-c-api for two reasons:

  1. wasmer-c-api doesn't build with the default features. cargo publish --features=jit,native,object-file builds though.
  2. the build.rs file changes the wasmer_wasm.h in the source directory. cargo publish --no-verify can proceed despite that.
@nlewycky nlewycky added the bug Something isn't working label Nov 6, 2020
@syrusakbary
Copy link
Member

syrusakbary commented Nov 6, 2020

For 2, the only thing we need to do is assuring to build the library first, commit any files and then publish?

@nlewycky
Copy link
Contributor Author

nlewycky commented Nov 6, 2020

I really don't think we should be modifying the source directory as part of any build action of any kind, ever. Put another way, we should be able to make all of wasmer directories read-only except the target/ directory and build test and publish should all work.

The reason we don't do this now is that we want to make sure we can review changes to that file and we don't have any way to review changes to it without putting it in the source directory. Last time this came up I didn't have any suggestions, but I just had an idea:

  • we keep a copy of wasmer_wasm.h in the source tree under a different name like wasmer_wasm.h-reference
  • add to make test a build of that file and verify that the generated wasmer_wasm.h matches our reference. If it doesn't, make test fails.
  • when intentionally modifying the generator for wasmer_wasm.h, the developer can update the reference which will be included in the diff. We could even have a make update-wasmer-wasm-h rule to do it, but this rule would never be ordinarily run by our builds, our CI, or our packaging, only with a manual run.
  • a tool like scripts/publish.py could run make test-wasmer-wasm-h before the rest of the publish to make sure it's up to date and fail before publishing some of the crates.

@jubianchi
Copy link
Contributor

The test-capi-* targets actually build the .h. We could add something like git diff --exit-code -- lib/c-api/wasmer_wasm.h to the end of test-capi recipe and it will make the build fail if we forgot to generate and commit the .h.

Doing such thing we don't have to deal with a reference file (and thus, having to care about two files instead of only one today).

@Hywan
Copy link
Contributor

Hywan commented Feb 5, 2021

I believe the problem has been solved with #2076. Closing.

@Hywan Hywan closed this as completed Feb 5, 2021
@Hywan Hywan self-assigned this Feb 5, 2021
@Hywan Hywan added the 📦 lib-c-api About wasmer-c-api label Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 📦 lib-c-api About wasmer-c-api
Projects
None yet
Development

No branches or pull requests

4 participants