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

Replace curl package into ureq #3511

Merged
merged 4 commits into from Jul 10, 2023
Merged

Replace curl package into ureq #3511

merged 4 commits into from Jul 10, 2023

Conversation

HoKim98
Copy link
Contributor

@HoKim98 HoKim98 commented Jul 7, 2023

When installing wasm-bindgen-cli in Alpine Linux, it's forced to install openssl-sys package.

But I found that the openssl dependency is only subject to curl package, which is used for headless operations.

So I think it's ok to adopt rustls instead, not installing the native openssl binaries by option.

And I replaced the old curl package into ureq, using rustls by default.

Therefore, this PR is a rustls support for wasm-bindgen-cli.

Copy link
Collaborator

@hamza1311 hamza1311 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 a good solution in the meantime but, ultimately we should just replace curl with ureq.

I'm afraid that removing the features in the future could end being a breaking change

@daxpedda
Copy link
Collaborator

daxpedda commented Jul 9, 2023

@kerryeon would you be willing to rip out cURL entirely and just switch to ureq?
I would prefer that we fix this now instead of introducing a bunch of crate features that we are going to deprecate anyway.

A note in the changelog would also be appreciated.

@HoKim98
Copy link
Contributor Author

HoKim98 commented Jul 10, 2023

Ok, let me dig in this migration job and notify it.

@HoKim98 HoKim98 changed the title Add rustls-tls support for wasm-bindgen-cli Replace curl package into ureq Jul 10, 2023
@HoKim98
Copy link
Contributor Author

HoKim98 commented Jul 10, 2023

@hamza1311 @daxpedda Hello, please review when you have time :)

- Add rustls-tls support for wasm-bindgen-cli
Copy link
Collaborator

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'll merge this once the CI is finished

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Thank you, a couple of nits here and there.

crates/cli/Cargo.toml Outdated Show resolved Hide resolved
crates/cli/Cargo.toml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: daxpedda <daxpedda@gmail.com>
HoKim98 added a commit to ulagbulag/wasm-bindgen that referenced this pull request Jul 10, 2023
HoKim98 added a commit to ulagbulag/wasm-bindgen that referenced this pull request Jul 10, 2023
HoKim98 added a commit to ulagbulag/wasm-bindgen that referenced this pull request Jul 10, 2023
Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Small nits.

crates/cli/Cargo.toml Outdated Show resolved Hide resolved
crates/cli/Cargo.toml Outdated Show resolved Hide resolved
crates/cli/Cargo.toml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: daxpedda <daxpedda@gmail.com>
@HoKim98
Copy link
Contributor Author

HoKim98 commented Jul 10, 2023

Confirmed!

Copy link
Collaborator

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Thank you!

@daxpedda daxpedda merged commit 0853bb7 into rustwasm:main Jul 10, 2023
25 checks passed
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.

None yet

3 participants