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

[libzip] update to rel-1-6-1 #10784

Merged
merged 5 commits into from
Apr 24, 2020
Merged

[libzip] update to rel-1-6-1 #10784

merged 5 commits into from
Apr 24, 2020

Conversation

mitzal
Copy link
Contributor

@mitzal mitzal commented Apr 11, 2020

Libzip updated to rel-1-6-1
[ #10778 ]

@msftclas
Copy link

msftclas commented Apr 11, 2020 •
edited
Loading

CLA assistant check
All CLA requirements met.

@mitzal mitzal changed the title Libzip updated to rel-1-6-1 [libzip] update to rel-1-6-1 Apr 12, 2020
LilyWangL
LilyWangL suggested changes Apr 13, 2020
View reviewed changes
ports/libzip/portfile.cmake Outdated Show resolved Hide resolved
@LilyWangL LilyWangL added info:reviewed Pull Request changes follow basic guidelines and removed needs-feature-review labels Apr 13, 2020
@janisozaur
[libzip] Add windows_crypto feature
def96f4 
The new BCrypt/CNG-based crypto uses BCryptDeriveKeyPBKDF2 which is only
available since WinNT 6.1. It is important to me as a consumer of libzip
through vcpkg to be able to turn this off.
janisozaur
janisozaur suggested changes Apr 14, 2020
View reviewed changes
Copy link
Contributor

@janisozaur janisozaur left a comment

Choose a reason for hiding this comment

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

Please merge mitzal#1

@janisozaur janisozaur mentioned this pull request Apr 14, 2020
Closed
@mitzal
Merge pull request #1 from janisozaur/libzip-windows-crypto
db7e8a9 
[libzip] Add windows_crypto feature
@mitzal mitzal requested a review from LilyWangL April 14, 2020 22:43
@ras0219-msft
Copy link
Contributor

Looking at the upstream code[1], it seems like only one of the crypto backends will be used. Therefore, it seems to me that we should restructure the features as:

Default-Features: default-aes
...
Feature: default-aes
Build-Depends: libzip[core,openssl] (!windows), libzip[core,wincrypto] (windows)

Feature: openssl
Build-Depends: openssl

Feature: wincrypto

We also need to explicitly disable the other backends (mbedTLS, gnutls, and commoncrypto) to ensure they aren't accidentally linked. These could be optionally made into features in the future if desired.

[1] https://github.com/nih-at/libzip/blob/8bd4f66d906026ab85bd2d05e47c6be0e144135f/CMakeLists.txt#L259

@LilyWangL LilyWangL added waiting for response and removed info:reviewed Pull Request changes follow basic guidelines labels Apr 17, 2020
@mitzal
Copy link
Contributor Author

mitzal commented Apr 17, 2020

Looking at the upstream code[1], it seems like only one of the crypto backends will be used. Therefore, it seems to me that we should restructure the features as:

Default-Features: default-aes
...
Feature: default-aes
Build-Depends: libzip[core,openssl] (!windows), libzip[core,wincrypto] (windows)

Feature: openssl
Build-Depends: openssl

Feature: wincrypto

We also need to explicitly disable the other backends (mbedTLS, gnutls, and commoncrypto) to ensure they aren't accidentally linked. These could be optionally made into features in the future if desired.

[1] https://github.com/nih-at/libzip/blob/8bd4f66d906026ab85bd2d05e47c6be0e144135f/CMakeLists.txt#L259

Features have been restructured and some additional options have been added.

@mitzal mitzal force-pushed the master branch 3 times, most recently from 8726774 to 3dd3822 Compare April 17, 2020 14:47
@janisozaur
Copy link
Contributor

https://dev.azure.com/vcpkg/public/_build/results?buildId=34181&view=logs&j=275f1d19-1bd8-5591-b06b-07d489ea915a&t=a4822bc1-f3a9-5984-c941-6c1b7aa2ab81&l=18

|libzip |Fail|core|Test passes in baseline but fails in current run. If expected update ci.baseline.txt with 'libzip:x64-linux=fail'

@LilyWangL LilyWangL added info:reviewed Pull Request changes follow basic guidelines and removed waiting for response labels Apr 24, 2020
@ras0219-msft ras0219-msft merged commit 242897b into microsoft:master Apr 24, 2020
@ras0219-msft
Copy link
Contributor

LGTM, thanks for the fixes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janisozaur janisozaur janisozaur requested changes

@LilyWangL LilyWangL Awaiting requested review from LilyWangL

Assignees

@LilyWangL LilyWangL

Labels
info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mitzal @msftclas @ras0219-msft @janisozaur @LilyWangL