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.

Sign up for GitHub

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

Jump to bottom

Use rustls-tls instead of OpenSSL #62

Merged
merged 5 commits into from
May 5, 2024
Merged

Use rustls-tls instead of OpenSSL #62

merged 5 commits into from
May 5, 2024

Conversation

DervexDev
Copy link
Contributor

Closes #61

@DervexDev DervexDev mentioned this pull request May 2, 2024
Closed
@LPGhatguy
Copy link
Owner

Huh. I wonder why the 1.58 test suite has started failing, I noticed that on main too.

@LPGhatguy
Copy link
Owner

I bumped the MSRV on the main branch to 1.60.0. If you rebase or pull main, it'll rerun the test suite and we'll see.

@DervexDev
Copy link
Contributor Author

Done!

@DervexDev
Copy link
Contributor Author

Oh it failed again, looks like reqwest v0.11.27 requires rustc 1.63.0

@LPGhatguy
Copy link
Owner

Feel free to bump the MSRV in the workflow file until it does work 😅

@DervexDev
Copy link
Contributor Author

Succeeded with 1.65.0

@paradoxuum paradoxuum mentioned this pull request May 3, 2024
Merged
@LPGhatguy
Copy link
Owner

It looks like openssl is still ending up in the tree if you search in Cargo.lock. I think we need to set default-features = false as well in order to not use native TLS (and thus OpenSSL) on Linux.

You can compare this PR to jackTabsCode/asphalt#37 to see the difference.

@DervexDev
Copy link
Contributor Author

It is actually not necessary to make Linux builds succeed but that's definitely better as we have less dependencies.

@LPGhatguy
Copy link
Owner

LPGhatguy commented May 5, 2024 •
edited
Loading

Right, not necessary to make the builds succeed, but it is necessary to ensure the binaries can run on machines that don't have a compatible version of OpenSSL installed. Otherwise, users would download and run the binary on those machines and find that they fail with a load-time error.

LPGhatguy
LPGhatguy approved these changes May 5, 2024
View reviewed changes
Copy link
Owner

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

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

LGTM! Merging when CI finishes.

@LPGhatguy LPGhatguy merged commit cc0ed2b into LPGhatguy:main May 5, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LPGhatguy LPGhatguy LPGhatguy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run Build (x86_64-unknown-linux-gnu) job
2 participants
@DervexDev @LPGhatguy