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

feat(*)!: Uses wasm-pkg-client for loading dependencies #322

Merged

Conversation

thomastaylor312
Copy link
Contributor

@thomastaylor312 thomastaylor312 commented Jul 12, 2024

This is a fairly large PR because this dependency is used everywhere. cargo component now uses the new wasm-pkg-tools toolchain to load deps, which means that both OCI and Warg are supported. This tries to stay as close to the original code style as possible, but I did have to make large changes to how dependencies were being resolved to account for the
new library. There are a couple major breaking changes to be aware of and one question to answer. This PR is already quite large, so I figured it would be better to probably merge this and then deal with follow ups to any of the outstanding questions below:

  • Bindings are now generated every time. Where the actual dependency is being loaded from is now abstracted away by the client, so we can't check if those files have changed on disk. I personally don't think this is the worst thing, but if people really want that functionality, we can add support for that into the package tools.
  • Offline deps currently does not work, but that is something that will be added into wasm-pkg-client instead of doing checks within cargo component itself.
  • Currently the registries specified within Cargo.toml are not used in favor of using the config file for wasm-pkg-tools. I am not sure if we wanted to use that to override the wasm-pkg-tools config or just drop it entirely. I would like to address this one before merge.
  • Until wasm-pkg-client has support for publishing, I have left support in to use warg directly. Once we have that added to wasm-pkg-tools, we can change the behavior here

@thomastaylor312 thomastaylor312 marked this pull request as draft July 12, 2024 22:49
Cargo.toml Outdated Show resolved Hide resolved
@thomastaylor312
Copy link
Contributor Author

Just a heads up that I know the tests for wit aren't passing yet because I forgot to update them, but I wanted to get this out there so we could start discussion on it

@thomastaylor312 thomastaylor312 force-pushed the feat/wasm-pkg-tools branch 4 times, most recently from f3840de to 2ae4e2d Compare July 16, 2024 19:13
@thomastaylor312 thomastaylor312 force-pushed the feat/wasm-pkg-tools branch 2 times, most recently from 725b0c0 to f1895a5 Compare July 31, 2024 21:20
@thomastaylor312 thomastaylor312 marked this pull request as ready for review July 31, 2024 21:21
@thomastaylor312
Copy link
Contributor Author

Marking this as ready for review. After talking to @calvinrp, we're going to merge this once we have the next version of cargo-component cut so we can start making changes. We will keep the git reference to wasm-pkg-client until we're ready to release it and cargo component. As for registries, we're ok with removing it for now as it can be specified with the wasm-pkg config. The only exception to setting a registry locally is likely to be for private registry overrides and is likely to be better done with a local wasm-pkg-tools config file. We'll be following up with support in wasm-pkg-client for offline support and publishing

@calvinrp
Copy link
Collaborator

calvinrp commented Aug 5, 2024

@thomastaylor312 Reviewed as best I can. Likely going to be issues that we can fix after merging, as well as things that we will want to revise further. I think we should cut a release for wasm-pkg-client and then update the dependency here (instead of pointing to a git rev).

But I'm OK with proceeding and fixing issues after merging.

@jsturtevant
Copy link

I tried this out and was able build a wasi-http component using this config in the cargo.toml:

[package.metadata.component.target]
package = "wasi:http"
version = "0.2.0"
world = "proxy" 

I saw it downloaded the wasm layer from $HOME/.cache/cargo-component/cache/sha256\:5a568e6e2d60c1ce51220e1833cdd5b88db9f615720edc762a9b4a6f36b383bd which aligns with the package at https://github.com/orgs/bytecodealliance/packages/container/package/wasm-pkg%2Fwasi%2Fhttp

This is a fairly large PR because this dependency is used everywhere.
cargo component now uses the new wasm-pkg-tools toolchain to load deps,
which means that both OCI and Warg are supported. This tries to stay as
close to the original code style as possible, but I did have to make
large changes to how dependencies were being resolved to account for the
new library. There are a couple major breaking changes to be aware of and
one question to answer. This PR is already quite large, so I figured it
would be better to probably merge this and then deal with follow ups to
any of the outstanding questions below:

- Bindings are now generated every time. Where the actual dependency is
  being loaded from is now abstracted away by the client, so we can't
  check if those files have changed on disk. I personally don't think this
  is the worst thing, but if people really want that functionality, we
  can add support for that into the package tools.
- Offline deps currently does not work, but that is something that will
  be added into wasm-pkg-client instead of doing checks within cargo
  component itself.
- Currently the registries specified within Cargo.toml are not used in
  favor of using the config file for wasm-pkg-tools. I am not sure if we
  wanted to use that to override the wasm-pkg-tools config or just drop
  it entirely. I would like to address this one before merge.
- Until wasm-pkg-client has support for publishing, I have left support
  in to use warg directly. Once we have that added to wasm-pkg-tools, we
  can change the behavior here

Signed-off-by: Taylor Thomas <[email protected]>
@calvinrp calvinrp self-requested a review August 7, 2024 20:31
Copy link
Collaborator

@calvinrp calvinrp left a comment

Choose a reason for hiding this comment

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

This is part of a big refactor to leverage wasm-pkg-tools to support both OCI and Warg registry protocols. Expect that main branch needs to be a bit of staging ground while follow on PRs land. So we will hold on doing the next cargo-component release until these changes are sorted.

This PR is intentionally pinned to a git rev, but will be updated in future PRs.

@calvinrp calvinrp merged commit 936a47f into bytecodealliance:main Aug 7, 2024
6 checks passed
mergify bot referenced this pull request in andrzejressel/pulumi-wasm Sep 6, 2024
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [cargo-component](https://redirect.github.com/bytecodealliance/cargo-component) | minor | `0.15.0` -> `0.16.0` |

---

### Release Notes

<details>
<summary>bytecodealliance/cargo-component (cargo-component)</summary>

### [`v0.16.0`](https://redirect.github.com/bytecodealliance/cargo-component/releases/tag/v0.16.0)

[Compare Source](https://redirect.github.com/bytecodealliance/cargo-component/compare/v0.15.0...v0.16.0)

#### What's Changed

-   feat(\*)!: Uses wasm-pkg-client for loading dependencies by [@&#8203;thomastaylor312](https://redirect.github.com/thomastaylor312) in [https://github.com/bytecodealliance/cargo-component/pull/322](https://redirect.github.com/bytecodealliance/cargo-component/pull/322)
-   feat(\*)!: Adds back support for the offline flag by [@&#8203;thomastaylor312](https://redirect.github.com/thomastaylor312) in [https://github.com/bytecodealliance/cargo-component/pull/328](https://redirect.github.com/bytecodealliance/cargo-component/pull/328)
-   Don't fail parsing on `@feature` and `@since` gates. by [@&#8203;dmvk](https://redirect.github.com/dmvk) in [https://github.com/bytecodealliance/cargo-component/pull/329](https://redirect.github.com/bytecodealliance/cargo-component/pull/329)
-   ref(\*)!: Updates all code to use the new publish functionality in wkg by [@&#8203;thomastaylor312](https://redirect.github.com/thomastaylor312) in [https://github.com/bytecodealliance/cargo-component/pull/331](https://redirect.github.com/bytecodealliance/cargo-component/pull/331)
-   updated wasm-pkg-client dep version 0.5.0 by [@&#8203;calvinrp](https://redirect.github.com/calvinrp) in [https://github.com/bytecodealliance/cargo-component/pull/334](https://redirect.github.com/bytecodealliance/cargo-component/pull/334)

#### New Contributors

-   [@&#8203;dmvk](https://redirect.github.com/dmvk) made their first contribution in [https://github.com/bytecodealliance/cargo-component/pull/329](https://redirect.github.com/bytecodealliance/cargo-component/pull/329)

**Full Changelog**: bytecodealliance/cargo-component@v0.15.0...v0.16.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] If you want to rebase/retry this PR, check this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/andrzejressel/pulumi-wasm).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants