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

[ixwebsocket] update to 9.6.2 #11030

Merged
merged 12 commits into from May 19, 2020
Merged

[ixwebsocket] update to 9.6.2 #11030

merged 12 commits into from May 19, 2020

Conversation

bsergean
Copy link
Contributor

Update the library to 9.5.0.

Notable changes:

  • Windows default to OpenSSL, this simplify the Portfile.
  • On Windows, with TLS on, load the os/system certificates by default for verifying peers.
  • On OpenSSL and mbedtls TLS backend, add ability to specify a ca-cert from memory, instead of using a file.

NancyLi1013
NancyLi1013 reviewed Apr 26, 2020
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.

vcpkg_test_cmake() is deprecated.
Could you please remove it?

@NancyLi1013 NancyLi1013 added requires:testing Needs tests added before merging and removed waiting for response labels Apr 26, 2020
@NancyLi1013
Copy link
Contributor

Needs to test the features.

NancyLi1013
NancyLi1013 reviewed Apr 26, 2020
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.

I noticed there is only one options -DUSE_TLS=1 that is related with these features.
Besides this, I didn't see any operations to handle these features.
Could you please help make a simple clarification about how these features work?

@bsergean
Copy link
Contributor Author

Hi @NancyLi1013,

I might not use the proper vcpkg terminology.

I used curl as an example, as it's a bit similar to my app.

  • I want TLS to always be activated when building this library, which is why the CMake has USE_TLS=1.
  • On macOS, the core tls library is part of the OS and called secure-transport. I don't need any dependent package to build it.
  • on Windows and Linux, I want the tls library to be OpenSSL, which is why I'm setting up this feature, as non optional.
# SSL backends
Feature: openssl
Build-Depends: openssl
Description: SSL support (OpenSSL)

@NancyLi1013
Copy link
Contributor

HI @bsergean
Thanks for your above info.
Besides this, we still need to judge in which condition that we can use these features in portfile.cmake.

For example, if we want to build with openssl or sectransp feature, how these features work?
Do these options USE_OPEN_SSL USE_SECURE_TRANSPORT USE_MBED_TLS not need to be set in portfile.cmake?

For feature mbedtls, I noticed you removed it from feature list. Could you help make a simple description?

@NancyLi1013 NancyLi1013 removed the requires:testing Needs tests added before merging label Apr 27, 2020
@bsergean
Copy link
Contributor Author 

$ ./vcpkg install ixwebsocket        
Computing installation plan...
The following packages will be built and installed:
    ixwebsocket[core]:x64-osx
Warning: abi keys are missing values:
    zlib

Starting package 1/1: ixwebsocket:x64-osx
Building package ixwebsocket[core]:x64-osx...
-- Downloading https://github.com/machinezone/IXWebSocket/archive/v9.5.2.tar.gz...
-- Extracting source /Users/bsergeant/src/foss/github/vcpkg/downloads/machinezone-IXWebSocket-v9.5.2.tar.gz
-- Using source at /Users/bsergeant/src/foss/github/vcpkg/buildtrees/ixwebsocket/src/v9.5.2-efe54999d7
-- Configuring x64-osx-dbg
-- Configuring x64-osx-rel
-- Building x64-osx-dbg
-- Building x64-osx-rel
-- Installing: /Users/bsergeant/src/foss/github/vcpkg/packages/ixwebsocket_x64-osx/share/ixwebsocket/copyright
-- Performing post-build validation
-- Performing post-build validation done
Building package ixwebsocket[core]:x64-osx... done
Installing package ixwebsocket[core]:x64-osx...
Installing package ixwebsocket[core]:x64-osx... done
Elapsed time for package ixwebsocket:x64-osx: 19.44 s

Total elapsed time: 19.44 s

Tried to build locally (on macOS) and it worked fine.

exclude uwp from the supported platforms / cannot see in logfiles why…
7694da4 
… it is not building + I do not have access to this platform for testing
@bsergean bsergean mentioned this pull request Apr 29, 2020
Closed
@NancyLi1013
Copy link
Contributor

Why do you remove all features?

@bsergean
Copy link
Contributor Author

bsergean commented Apr 30, 2020 via email

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Apr 30, 2020
@strega-nil
Copy link
Contributor

@bsergean this seems to remove support for UWP; that's, imo, a non-starter. Can you add back in the dependency on mbedtls for UWP, and depend on opensll for !uwp & !osx?

@strega-nil strega-nil added waiting for response and removed info:reviewed Pull Request changes follow basic guidelines labels May 1, 2020
@bsergean
Copy link
Contributor Author

bsergean commented May 1, 2020

I'd prefer making uwp works with OpenSSL, as it's easier for me to test and support. I'll see what I can do. Switching windows to openssl addresses a very common bug.

I have tried enabling CI for uwp in here.
https://github.com/machinezone/IXWebSocket/tree/feature/windows_uwp

The build fails with:

2020-04-28T00:14:35.5677795Z D:\a\IXWebSocket\IXWebSocket\ixwebsocket\IXDNSLookup.cpp(44,54): error C2679: binary '=': no operator found which takes a right-hand operand of type 'WCHAR *' (or there is no acceptable conversion) [D:\a\IXWebSocket\IXWebSocket\build\ixwebsocket.vcxproj]

in this code, which works will all platform (and used to work ?). Not sure what's going on.

        struct addrinfo* res;
        int getaddrinfo_result = getaddrinfo(hostname.c_str(), sport.c_str(), &hints, &res);
        if (getaddrinfo_result)
        {
            errMsg = gai_strerror(getaddrinfo_result);
            res = nullptr;
        }
        return res;

@bsergean
Copy link
Contributor Author

bsergean commented May 1, 2020

gai_strerror seems to return a wchar_t

I configure with:

-DCMAKE_SYSTEM_NAME=WindowsStore -DCMAKE_SYSTEM_VERSION="10.0"

If you have any clue on what's going on @strega-nil I'm all ears :)

@bsergean
Copy link
Contributor Author

bsergean commented May 1, 2020

I tried to find a MACRO that tells me if I'm building on uwp but I couldn't find it. I could convert the wstring to a string for uwp.

@strega-nil
Copy link
Contributor

@bsergean it looks like _UNICODE is getting defined on uwp, but not regular windows; might mean you need to explicitly undefine _UNICODE with /U_UNICODE

bsergean added a commit to machinezone/IXWebSocket that referenced this pull request May 18, 2020
@bsergean
(cmake) make install cmake files optional to not conflict with vcpkg
54d4d81 
See microsoft/vcpkg#11030
@bsergean
Copy link
Contributor Author

Cool those logs helped.

I made one last change and everything seems to pass but the osx test, which looks like a flaky intermittent azure pipeline problem (failure to git clone). Next run should be good.

remote: Compressing objects: 100% (15/15), done.        
remote: Total 65 (delta 33), reused 37 (delta 29), pack-reused 21        
From https://github.com/microsoft/vcpkg
 * [new ref]             refs/pull/11030/merge -> pull/11030/merge
git checkout --progress --force refs/remotes/pull/11030/merge
Previous HEAD position was 35a71e008 Merge 6248f7cc6dd2a25fe8b8f31d8f8ef1c76eec5ed9 into 8a583e80da3b72141105da9003175679af2fcb92
fatal: update_ref failed for ref 'HEAD': cannot lock ref 'HEAD': Unable to create '/Volumes/data/work/1/s/.git/HEAD.lock': File exists.

Another git process seems to be running in this repository, e.g.
an editor opened by 'git commit'. Please make sure all processes
are terminated then try again. If it still fails, a git process
may have crashed in this repository earlier:
remove the file manually to continue.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013
Copy link
Contributor

It seems good now except for osx pipeline that is not related with this PR.
We will solve it later.

@bsergean
Copy link
Contributor Author

It looks like CI is happy again indeed, 'All checks have passed' says the robot.

@NancyLi1013 NancyLi1013 added the requires:testing Needs tests added before merging label May 19, 2020
@NancyLi1013
Copy link
Contributor

Yes, all checks have passed now.
I am going to test these features and will update the results once completed.

@bsergean
Copy link
Contributor Author

bsergean commented May 19, 2020 via email

@bsergean bsergean changed the title [ixwebsocket] update to 9.5.0 [ixwebsocket] update to 9.6.2 May 19, 2020
@bsergean
Copy link
Contributor Author

The check that fails looks like a ci problem, unrelated to the change, but I could be wrong -> https://dev.azure.com/vcpkg/public/_build/results?buildId=37180&view=results

It failed right away after 4s trying to cleanup a scratch folder.

@bsergean bsergean mentioned this pull request May 19, 2020
Closed
@NancyLi1013
Copy link
Contributor

All feature (except for sectransp) have passed with the following triplets:

  • x86-windows
  • x64-windows
  • x64-windows-static

Note: Feature sectransp only supports osx platform.

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:testing Needs tests added before merging labels May 19, 2020
@strega-nil
Copy link
Contributor

Alright, cool, thanks @bsergean :) This looks great

@strega-nil strega-nil merged commit 9e03a3a into microsoft:master May 19, 2020
@bsergean
Copy link
Contributor Author

Thanks !

Took me a while but I think I understand ports better now.

@bsergean bsergean deleted the feature/ixwebsocket_9.5.0 branch May 19, 2020 21:44
@ludekvodicka
Copy link
Contributor

Hello, Benjamin (@bsergean bsergean),
I tried to update IXWebsocket port to 11.0.4 version but I'm not sure how to solve this vcpkg error:

The /lib/cmake folder should be merged with /debug/lib/cmake and moved to /share/ixwebsocket/cmake.
Please use the helper function `vcpkg_fixup_cmake_targets()`
The following cmake files were found outside /share/ixwebsocket. Please place cmake files in /share/ixwebsocket.

    Q:/ExternalLibraries/vcpkg32/packages/ixwebsocket_x86-windows/lib/cmake/ixwebsocket/ixwebsocket-config-release.cmake
    Q:/ExternalLibraries/vcpkg32/packages/ixwebsocket_x86-windows/lib/cmake/ixwebsocket/ixwebsocket-config.cmake
    Q:/ExternalLibraries/vcpkg32/packages/ixwebsocket_x86-windows/debug/lib/cmake/ixwebsocket/ixwebsocket-config-debug.cmake
    Q:/ExternalLibraries/vcpkg32/packages/ixwebsocket_x86-windows/debug/lib/cmake/ixwebsocket/ixwebsocket-config.cmake

The /debug/lib/cmake folder should be merged with /lib/cmake into /share/ixwebsocket
Found 3 error(s). Please correct the portfile:
    Q:\ExternalLibraries\vcpkg32\ports\ixwebsocket\portfile.cmake
-- Performing post-build validation done
Error: Building package ixwebsocket:x86-windows failed with: POST_BUILD_CHECKS_FAILED

I only changed REF and SHA hash in portfile.cmake to following:

vcpkg_from_github(
    OUT_SOURCE_PATH SOURCE_PATH
    REPO machinezone/IXWebSocket
    REF v11.0.4
    SHA512 
	fb24a628600cf28acdcaed5d2268f6a6e36baa1cc31f54287d91fb979fe375b20931fa9346153eaaf5a5d17fc6d87f06ca03ce12e401b83095c16919d35454ce
)

but there are probably larger differences between 9.x and 11.x versions

@bsergean
Copy link
Contributor Author

bsergean commented Dec 30, 2020 via email

@bsergean
Copy link
Contributor Author

bsergean commented Dec 30, 2020 via email

@bsergean
Copy link
Contributor Author

bsergean commented Dec 30, 2020 via email

@ludekvodicka
Copy link
Contributor

ludekvodicka commented Dec 30, 2020 •
edited

Is there any official way on how to ask maintainers to check it?

Your library is great and I'm currently building it myself. But I'm trying to migrate all our compilation scripts to vcpkg but I have only limited knowledge of cmake and this is unfortunately also beyond my knowledge.

PS: I can prepare new tag and hashs, it's not a problem.

@ludekvodicka
Copy link
Contributor

You were right, adding these two lines fixed the issue

file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/debug/lib/cmake)
file(REMOVE_RECURSE ${CURRENT_PACKAGES_DIR}/lib/cmake)

Now the build proceeds correctly. I'm going to test it and if everything will be ok, I will create PR.

@NancyLi1013
Copy link
Contributor

@ludekvodicka

You can usevcpkg_fixup_cmake_targets(CONFIG_PATH "lib/cmake/ixwebsocket")to solve this issue.

@ludekvodicka
Copy link
Contributor

@NancyLi1013 Thanks! Victor Romero already helped me in another thread.

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

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

Assignees

@NancyLi1013 NancyLi1013

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.

None yet
4 participants
@bsergean @NancyLi1013 @strega-nil @ludekvodicka