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

tokio-native-tls: remove unused dependencies #133

Merged
merged 1 commit into from
Mar 30, 2023
Merged

tokio-native-tls: remove unused dependencies #133

merged 1 commit into from
Mar 30, 2023

Conversation

djc
Copy link
Contributor

@djc djc commented Mar 13, 2023

No description provided.

@djc djc mentioned this pull request Mar 16, 2023
Closed
cpu
cpu approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Would be nice to get this change merged if someone with review perms has a free minute. I wasn't able to build tip of master without it and it looks like others are hitting it too (#135).

@djc djc requested review from hawkw, LucioFranco and quininer March 22, 2023 16:46
@cpu cpu mentioned this pull request Mar 22, 2023
Merged
@djc
Copy link
Contributor Author

djc commented Mar 29, 2023

@quininer @hawkw @LucioFranco ping? This seems pretty uncontroversial, would like to get this in to pass CI so we can get tokio-rustls out with the new rustls version.

quininer
quininer approved these changes Mar 30, 2023
View reviewed changes
Copy link
Member

@quininer quininer left a comment

Choose a reason for hiding this comment

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

LGTM, but I don't know enough about native-tls .

robjtede
robjtede suggested changes Mar 30, 2023
View reviewed changes
Copy link

@robjtede robjtede left a comment •
edited
Loading

Choose a reason for hiding this comment

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

From what I can tell (testing on macOS), this dependency spec doesn't do anything.

Grepping for security_framework shows nothing in this repo and security-framework-sys v2.8.0 + security-framework v2.8.2 get selected anyway via native-tls.

This line dates back to the initial commit, but it's not clear that it served any purpose even then.

My suggestion is to delete this section instead of updating the dep spec.

@djc djc force-pushed the security-framework branch from e131ef5 to 7c40a0f Compare March 30, 2023 07:13
@djc djc changed the title Upgrade security-framework to version 2 tokio-native-tls: remove unused dependencies Mar 30, 2023
@djc
Copy link
Contributor Author

djc commented Mar 30, 2023

@robjtede huh, you're right. I was able to remove a whole bunch of dependencies, let's see what CI thinks.

@quininer quininer merged commit 7dfc981 into master Mar 30, 2023
@quininer
Copy link
Member

It didn't seem risky, so I merged it.

@cpu
Copy link
Contributor

cpu commented Mar 30, 2023

Thanks for tidying this up everyone.

@taiki-e taiki-e deleted the security-framework branch January 6, 2024 09:36
@perheed-adv perheed-adv mentioned this pull request Mar 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quininer quininer quininer approved these changes

@robjtede robjtede robjtede approved these changes

@cpu cpu cpu approved these changes

@hawkw hawkw Awaiting requested review from hawkw

@LucioFranco LucioFranco Awaiting requested review from LucioFranco

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

Successfully merging this pull request may close these issues.
4 participants
@djc @quininer @cpu @robjtede