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

[usockets] WolfSSL feature #32162

Merged
merged 5 commits into from
Jun 22, 2023
Merged

[usockets] WolfSSL feature #32162

merged 5 commits into from
Jun 22, 2023

Conversation

pierr3
Copy link
Contributor

@pierr3 pierr3 commented Jun 21, 2023

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

This PR adds WolfSSL support as a feature for uSockets.

@pierr3 pierr3 changed the title WolfSSL usockets feature [usockets] WolfSSL feature Jun 21, 2023
@vicroms vicroms merged commit 20b059f into microsoft:master Jun 22, 2023
15 checks passed
@dg0yt
Copy link
Contributor

dg0yt commented Jun 22, 2023

Reviews, any?
There is already an ssl feature based on openssl. Is the wolfssl feature additive, or does it implement an alternative?
If it is an alternative, it must be removed immediately.

@pierr3
Copy link
Contributor Author

pierr3 commented Jun 22, 2023

It is one of the SSL implementations in uSockets, OpenSSL is not required to obtain that and SSL features can be obtained through OpenSS, WolfSSL or BoringSSL. Reasons not to use OpenSSL include reducing compile time/overheads. See uSockets

@dg0yt
Copy link
Contributor

dg0yt commented Jun 22, 2023

@pierr3 You don't have to explain why you want to choose alternatives. The vcpkg maintainers must ensure that features are used correctly.

Feature ssl names the behaviour it adds to the package. It chose to use openssl as a backend. How to correctly choose wolfssl? usockets[ssl,wolfssl] doesn't omit the openssl dependency. usockets[wolfssl] doesn't correctly includ the ssl feature.

Really, this PR was merged without sufficient review. I don't know if usockets allows to choose backends at runtime. But the feature interface is invalid now IMO.


list(APPEND CMAKE_REQUIRED_INCLUDES ${OPENSSL_INCLUDE_DIR})
else()
set(NOT_USE_WOLFSSL "-DLIBUS_NO_SSL")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is set even if openssl is enabled. Does it meant that usockets[ssl] is broken now?

Copy link
Contributor

Choose a reason for hiding this comment

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

usockets[ssl] is broken now.
@vicroms @BillyONeal Please revert, and return to proper reviews.

Copy link
Member

@vicroms vicroms Jun 22, 2023

Choose a reason for hiding this comment

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

My bad, this is a case of an alternative as a feature that should have been rejected per policy.

@pierr3 if you need to override the ssl selection please use an overlay port

vicroms added a commit that referenced this pull request Jun 22, 2023
vicroms added a commit that referenced this pull request Jun 22, 2023
@pierr3
Copy link
Contributor Author

pierr3 commented Jun 22, 2023

Apologies for the confusion, thank you for the explanation, will review the features docs deeper next time.

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