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

Add ci build for WASM target for specific packages #394

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

heeckhau
Copy link
Member

No description provided.

@heeckhau heeckhau linked an issue Dec 14, 2023 that may be closed by this pull request
@heeckhau heeckhau changed the title Add ci build for WASM target for specific packages WIP: Add ci build for WASM target for specific packages Dec 14, 2023
@heeckhau heeckhau changed the title WIP: Add ci build for WASM target for specific packages Add ci build for WASM target for specific packages Dec 15, 2023
@heeckhau heeckhau marked this pull request as ready for review December 16, 2023 22:19
@@ -45,3 +45,6 @@ bincode.workspace = true
[[test]]
name = "api"
required-features = ["fixtures"]

[target.'cfg(target_arch = "wasm32")'.dependencies]
Copy link
Collaborator

@mhchia mhchia Dec 18, 2023

Choose a reason for hiding this comment

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

Should ring = { version = "0.17", features = ["wasm32_unknown_unknown_js"] } also be required here since tlsn-tls-core is a dependency and it depends on ring?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mhchia I am not sure. I have not fully analysed the dependencies.
It builds without specifying it here, so that is why I didn't add it here.

Copy link
Member

@th4s th4s Dec 18, 2023

Choose a reason for hiding this comment

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

I think tls-core already pulls in ring 0.17 because it has ring.workspace = true. So no need to make it a dependency of tlsn-core?

Copy link
Collaborator

@mhchia mhchia Dec 18, 2023

Choose a reason for hiding this comment

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

@heeckhau I see 🙏
@th4s Yeah but in order for tlsn-core (and thus ring 0.17) to build in wasm32, should we specify the feature flag wasm32_unknown_unknown_js for ring 0.17, or do I misunderstand something?

Copy link
Member

@th4s th4s Dec 18, 2023

Choose a reason for hiding this comment

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

Ahh, haven't thought about this. Following this additive-features it would probably work in most cases, since it is never pulled in alone.

But it would probably be better to add the feature to tls-core then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Following this additive-features it would probably work in most cases, since it is never pulled in alone.

I see. Thanks for sharing! If tlsn-core is not used alone then whether adding or not both works for me 🙏

Copy link
Collaborator

@mhchia mhchia left a comment

Choose a reason for hiding this comment

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

🙏

@heeckhau heeckhau merged commit 2d205f6 into dev Dec 18, 2023
15 checks passed
@heeckhau heeckhau deleted the 297-add-wasm-build-to-ci branch December 18, 2023 14:30
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.

Add WASM build to CI
3 participants