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: support compiling to webassembly #2254

Merged
merged 48 commits into from
Mar 29, 2023

Conversation

eduardomourar
Copy link
Contributor

@eduardomourar eduardomourar commented Jan 26, 2023

Motivation and Context

#2087

In order to compile the AWS SDK to WebAssembly, it requires to patch the ring dependency and disable most default features.

Description

This will introduce some additional improvements and allow the AWS SDK to be compiled to WebAssembly without any workaround.

Testing

A basic integration test has been added that targets wasm32-wasi. Keep in mind that we are mocking the response until outbound HTTP request is added to the WASI standard.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Collaborator

@jdisanti jdisanti 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 awesome! Thank you for the contribution ❤️

My biggest comment is around disabling SSO. I think it will be better to swap out ring instead. Let me know what you think.

aws/rust-runtime/aws-config/Cargo.toml Outdated Show resolved Hide resolved
aws/rust-runtime/aws-endpoint/Cargo.toml Outdated Show resolved Hide resolved
@@ -15,4 +15,5 @@ members = [
"s3control",
"sts",
"transcribestreaming",
"webassembly",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have some prerequisite work to do before this will run in CI. I'll see if I can get around to that today.

Copy link
Contributor Author

@eduardomourar eduardomourar Jan 26, 2023

Choose a reason for hiding this comment

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

Our best option is to install cargo-wasi then run cargo wasi test -- --nocapture

Copy link
Collaborator

Choose a reason for hiding this comment

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

That aspect of it didn't even cross my mind yet 😅

I did the prerequisite work to actually include the tests in the first place in #2255.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the tokio crate, they have a nice a setup for testing against the different webassembly targets: https://github.com/tokio-rs/tokio/blob/06f1a601bb05b1aba9f95020a7fa7572899c588f/.github/workflows/ci.yml#L529-L590

aws/sdk/integration-tests/webassembly/tests/test.rs Outdated Show resolved Hide resolved
rust-runtime/aws-smithy-client/Cargo.toml Outdated Show resolved Hide resolved
rust-runtime/aws-smithy-client/Cargo.toml Show resolved Hide resolved
rust-runtime/aws-smithy-http/Cargo.toml Show resolved Hide resolved
Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

Some minor things.

aws/rust-runtime/aws-config/src/lib.rs Outdated Show resolved Hide resolved
aws/rust-runtime/aws-inlineable/Cargo.toml Outdated Show resolved Hide resolved
aws/rust-runtime/aws-types/Cargo.toml Outdated Show resolved Hide resolved
@jdisanti
Copy link
Collaborator

For CI, there are a few different problems.

  1. The Kotlin unit tests aren't compiling:
e: /home/build/workspace/smithy-rs/aws/sdk-codegen/src/test/kotlin/software/amazon/smithy/rustsdk/EndpointsCredentialsTest.kt: (101, 79): Unresolved reference: runtimeConfig
e: /home/build/workspace/smithy-rs/aws/sdk-codegen/src/test/kotlin/software/amazon/smithy/rustsdk/EndpointsCredentialsTest.kt: (103, 57): Function invocation 'awsCredentialTypesTestUtil(...)' expected
e: /home/build/workspace/smithy-rs/aws/sdk-codegen/src/test/kotlin/software/amazon/smithy/rustsdk/EndpointsCredentialsTest.kt: (103, 57): No value passed for parameter 'runtimeConfig'
e: /home/build/workspace/smithy-rs/aws/sdk-codegen/src/test/kotlin/software/amazon/smithy/rustsdk/EndpointsCredentialsTest.kt: (125, 79): Unresolved reference: runtimeConfig
e: /home/build/workspace/smithy-rs/aws/sdk-codegen/src/test/kotlin/software/amazon/smithy/rustsdk/EndpointsCredentialsTest.kt: (127, 57): Function invocation 'awsCredentialTypesTestUtil(...)' expected
e: /home/build/workspace/smithy-rs/aws/sdk-codegen/src/test/kotlin/software/amazon/smithy/rustsdk/EndpointsCredentialsTest.kt: (127, 57): No value passed for parameter 'runtimeConfig'
  1. The aws-config crate's tests fail when doing cargo test --all-features.
  2. Looks like aws/sdk/integration-tests/webassembly/tests/test.rs needs rustfmt run on it.
  3. Something weird going on with the unused dependencies check on aws-config:
unused dependencies:
`aws-config v0.54.1 (/home/build/workspace/aws-sdk-smoketest/sdk/aws-config)`
└─── dependencies
     └─── "aws-sdk-sso"
Note: These dependencies might be used by other targets.
      To find dependencies that are not used by any target, enable `--all-targets`.
Note: They might be false-positive.
      For example, `cargo-udeps` cannot detect usage of crates that are only used in doc-tests.
      To ignore some dependencies, write `package.metadata.cargo-udeps.ignore` in Cargo.toml.

@jdisanti
Copy link
Collaborator

Note to work on aws-config locally, you may need to run ./gradlew aws:sdk:assemble so that the STS/SSO crates it depends on actually exist.

@eduardomourar
Copy link
Contributor Author

@jdisanti and @rcoh , could we get this approved and merged, please?

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

This looks great! I'm going to work on merging it today and wiring it up to CI.

@jdisanti
Copy link
Collaborator

I added a compilation check for wasm32-unknown-unknown and wasm32-wasi for aws-config to CI. I'm not confident we won't break things until we can run tests, but the tests don't currently compile. I've filed #2499 to track fixing that.

I'm willing to merge this once the CI portion passes to at least keep your progress, but there is a chance we may break things in future releases without this visibility in CI.

Thank you for the work here!

@jdisanti
Copy link
Collaborator

Looks like it passed CI. Adding it to the merge queue.

@jdisanti jdisanti enabled auto-merge March 24, 2023 23:29
@jdisanti jdisanti added this pull request to the merge queue Mar 25, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 25, 2023
@rcoh rcoh enabled auto-merge March 29, 2023 18:56
@rcoh rcoh added this pull request to the merge queue Mar 29, 2023
Merged via the queue into smithy-lang:main with commit 65058d9 Mar 29, 2023
@eduardomourar eduardomourar deleted the feat/support-wasm branch March 29, 2023 20:35
unexge pushed a commit that referenced this pull request Apr 24, 2023
* feat(aws-config): define default feature for sso

* chore(aws): disable unused features

* chore(rust-runtime): disable unused features

* chore: add missing tokio fs feature

* chore: disable spawn blocking on wasm

* chore: fix sso feature

* chore: allow unused variables when not sso

* chore: remove unnecessary dependency filter for wasm

* chore: add integration test for wasm

* chore: ensure test-util feature is applied to dev dependencies

* chore: disable retry config

Co-authored-by: John DiSanti <[email protected]>

* chore: simplify features as suggested

* chore: no default features for aws types

* chore: rename credentials-sso feature

* Revert "chore: simplify features as suggested"

This reverts commit d8fcf49.

* chore: set right name in default features

* chore: create smithy client test util in runtime types

* Update aws/rust-runtime/aws-types/Cargo.toml

Co-authored-by: John DiSanti <[email protected]>

* Update aws/rust-runtime/aws-inlineable/Cargo.toml

Co-authored-by: John DiSanti <[email protected]>

* Update aws/rust-runtime/aws-config/src/profile/credentials/exec.rs

Co-authored-by: John DiSanti <[email protected]>

* Update aws/rust-runtime/aws-config/src/profile/credentials/exec.rs

Co-authored-by: John DiSanti <[email protected]>

* Update aws/rust-runtime/aws-config/src/profile/credentials/exec.rs

Co-authored-by: John DiSanti <[email protected]>

* Update aws/rust-runtime/aws-config/src/lib.rs

Co-authored-by: John DiSanti <[email protected]>

* chore: use hardcoded credentials feature

* chore: make wasm example for s3 instead

* chore: fix formatting

* chore: fix kotlin formatting

* chore: fix kotlin unit tests

* chore: use timeout config from smithy types

* chore: move tests into main file

* chore: add vscode setttings to target wasi by default

* chore: fix test-util feature for smithy client

* chore: separate adapter into own module

* chore: fix test with no default features

* Fix typo

* Update changelog

* Check compilation of `aws-config` against `wasm32` in CI

* Fix Dockerfile issue and use correct cargo-wasi command

* Small Dockerfile fix

* Add missing `tar` binary to Docker image

---------

Co-authored-by: Eduardo Rodrigues <[email protected]>
Co-authored-by: John DiSanti <[email protected]>
Co-authored-by: John DiSanti <[email protected]>
Co-authored-by: Russell Cohen <[email protected]>
rcoh added a commit that referenced this pull request Apr 24, 2023
* feat(aws-config): define default feature for sso

* chore(aws): disable unused features

* chore(rust-runtime): disable unused features

* chore: add missing tokio fs feature

* chore: disable spawn blocking on wasm

* chore: fix sso feature

* chore: allow unused variables when not sso

* chore: remove unnecessary dependency filter for wasm

* chore: add integration test for wasm

* chore: ensure test-util feature is applied to dev dependencies

* chore: disable retry config

Co-authored-by: John DiSanti <[email protected]>

* chore: simplify features as suggested

* chore: no default features for aws types

* chore: rename credentials-sso feature

* Revert "chore: simplify features as suggested"

This reverts commit d8fcf49.

* chore: set right name in default features

* chore: create smithy client test util in runtime types

* Update aws/rust-runtime/aws-types/Cargo.toml

Co-authored-by: John DiSanti <[email protected]>

* Update aws/rust-runtime/aws-inlineable/Cargo.toml

Co-authored-by: John DiSanti <[email protected]>

* Update aws/rust-runtime/aws-config/src/profile/credentials/exec.rs

Co-authored-by: John DiSanti <[email protected]>

* Update aws/rust-runtime/aws-config/src/profile/credentials/exec.rs

Co-authored-by: John DiSanti <[email protected]>

* Update aws/rust-runtime/aws-config/src/profile/credentials/exec.rs

Co-authored-by: John DiSanti <[email protected]>

* Update aws/rust-runtime/aws-config/src/lib.rs

Co-authored-by: John DiSanti <[email protected]>

* chore: use hardcoded credentials feature

* chore: make wasm example for s3 instead

* chore: fix formatting

* chore: fix kotlin formatting

* chore: fix kotlin unit tests

* chore: use timeout config from smithy types

* chore: move tests into main file

* chore: add vscode setttings to target wasi by default

* chore: fix test-util feature for smithy client

* chore: separate adapter into own module

* chore: fix test with no default features

* Fix typo

* Update changelog

* Check compilation of `aws-config` against `wasm32` in CI

* Fix Dockerfile issue and use correct cargo-wasi command

* Small Dockerfile fix

* Add missing `tar` binary to Docker image

---------

Co-authored-by: Eduardo Rodrigues <[email protected]>
Co-authored-by: John DiSanti <[email protected]>
Co-authored-by: John DiSanti <[email protected]>
Co-authored-by: Russell Cohen <[email protected]>
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