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

fix: add missing OpenSSL dependency for building wws #77

Merged
merged 10 commits into from
Jan 27, 2023

Conversation

Angelmmiguel
Copy link
Contributor

@Angelmmiguel Angelmmiguel commented Jan 26, 2023

I tried to fix this error by installing the required dependencies following the recommendation in the openssl-sys crate. However, this only works for the gnu tooling. Since we're using musl for compiling static binaries, we cannot rely on this solution.

I tested the usage of rusttls instead of openssl. I couldn't make it work in Windows ARM64, as one of the dependencies (ring) is not compiling on that environment with the latest version. The team did an amazing job of supporting this platform and it's already fixed in upstream (See this PR). However, this version is not published yet. It seems they're planning to release the 0.16.21 it soon. Until this version is published, unfortunately we won't be able to swich to rusttls.

In the end, I used the vendored feature from the openssl crate in both musl target. This feature compiles the OpenSSL library from source. I tested and it's properly working.

You can find here a successful run, as I removed the PR trigger in the latest commit: https://github.com/vmware-labs/wasm-workers-server/actions/runs/4016745514

It closes #76

@Angelmmiguel Angelmmiguel added the 🐛 bug Something isn't working label Jan 26, 2023
@Angelmmiguel Angelmmiguel added this to the v1.0.0 milestone Jan 26, 2023
@Angelmmiguel Angelmmiguel self-assigned this Jan 26, 2023
@Angelmmiguel Angelmmiguel requested a review from a team January 26, 2023 16:03
@Angelmmiguel Angelmmiguel marked this pull request as ready for review January 26, 2023 16:03
Copy link
Contributor

@assambar assambar left a comment

Choose a reason for hiding this comment

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

Great research and findings! Thanks for the detailed explanation!

Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

Thanks for the great research @Angelmmiguel! LGTM

@Angelmmiguel Angelmmiguel merged commit 9f49736 into main Jan 27, 2023
@Angelmmiguel Angelmmiguel deleted the 76-fix-build-process branch May 8, 2023 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working cla-not-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error building the binaries after adding reqwest
4 participants