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

[teemo] Update version to v2.2. #18551

Merged
merged 6 commits into from Jul 23, 2021
Merged

[teemo] Update version to v2.2. #18551

merged 6 commits into from Jul 23, 2021

Conversation

winsoft666
Copy link
Contributor

Describe the pull request

  • What does your PR fix?

    Fixes Build-Depends library.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    <all / linux, windows, ...>, <Yes/No>

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    <Yes / I am still working on this PR>

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@NancyLi1013 NancyLi1013 added the category:port-update The issue is with a library, which is requesting update new revision label Jun 21, 2021
NancyLi1013
NancyLi1013 reviewed Jun 21, 2021
View reviewed changes
Copy link
Contributor

@NancyLi1013 NancyLi1013 left a comment

Choose a reason for hiding this comment

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

Could you please run vcpkg x-add-version teemo to update version files and commit the changes?

ports/teemo/CONTROL Outdated Show resolved Hide resolved
@NancyLi1013 NancyLi1013 changed the title [teemo] Update version to v2.3. [teemo] Update version to v2.2. Jun 21, 2021
@winsoft666
Copy link
Contributor Author

I have modified version to v2.2, please recheck again.

@NancyLi1013
Copy link
Contributor

Could you please run vcpkg x-add-version teemo to update version files and commit the changes?

Please update version files via the above command.

@winsoft666
Copy link
Contributor Author

Sorry, It's my mistake.
I have updated again.

ras0219-msft
ras0219-msft reviewed Jun 21, 2021
View reviewed changes
Copy link
Contributor

@ras0219-msft ras0219-msft 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 PR!

ports/teemo/CONTROL Outdated
Version: 2.1
Build-Depends: curl[non-http]
Version: 2.2
Build-Depends: curl[non-http,openssl]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this require curl[openssl]?

  1. We default to using the Win32 APIs on Windows, not openssl. At a minimum, this needs to be qualified to non-windows platforms.
  2. My understanding is that this is a behavior, not an API. teemo shouldn't be able to notice whether curl is built with or without SSL support and it's reasonable that a user might want to use teemo on top of curl without openssl.

If you want users of teemo to be able to request specifically OpenSSL support, then perhaps instead teemo should have an openssl feature that depends on curl's openssl feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

teemo is cross-platfrom, not noly support windows.
If you want libcurl support https, openssl is not optional.
Now https is very popular.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our default build of libcurl does support https on windows -- but via the Win32 APIs, not openssl.

Could you instead try using:

Suggested change
Build-Depends: curl[non-http,openssl]
Build-Depends: curl[core], openssl

and let us know specifically what does not work for you? If a user's project wants to support non-http protocols, the user can easily ask for that on the command line via vcpkg install teemo curl[non-http].

Copy link
Contributor

Choose a reason for hiding this comment

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

Port curl has the feature 'ssl' which chooses an implementation (another feature) depending on the platform. So there is no need to hardcode 'openssl'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dg0yt @ras0219-msft
I think both of yours opinion are right, I have updated to:
Build-Depends: curl[core]

@NancyLi1013 NancyLi1013 added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. requires:author-response and removed requires:author-response labels Jul 1, 2021
@NancyLi1013
Copy link
Contributor

@winsoft666
Could you please runvcpkg x-add-version --overwrite-version teemo to solve the failures on x86-windows?

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jul 20, 2021
@vicroms vicroms merged commit 4e2653e into microsoft:master Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dg0yt dg0yt dg0yt left review comments

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

@ras0219-msft ras0219-msft Awaiting requested review from ras0219-msft

Assignees

@NancyLi1013 NancyLi1013

Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@winsoft666 @NancyLi1013 @ras0219-msft @dg0yt @vicroms