Skip to content

Conversation

qis
Copy link
Contributor

@qis qis commented Oct 4, 2019

This is a new port for BoringSSL. As explained in #8084 (comment), there are several issues I see with it:

  1. It does not support UWP due to usage of desktop-only APIs like RtlGenRandom.
  2. It totally ignores CMAKE_C_FLAGS and CMAKE_CXX_FLAGS variables set by a MSVC toolchains.
  3. When CMakeLists.txt is patched to honor flags set by a toolchain, /EH flags break compilation.
  4. It uses its own gtest for tests which has the same /EH flags issues.
  5. It uses Go for codegen.

It may be useful to people targeting Windows Desktop, Linux and possibly OSX who stick with the supported build system options.

It may be a good idea to patch out the tests as a workaround for the poorly designed CMakeLists.txt, or make tests optional.

@qis
Copy link
Contributor Author

qis commented Oct 4, 2019

I have no idea how to deal with those specific failed builds (except UWP - that's expected).

@cbezault
Copy link
Contributor

cbezault commented Oct 7, 2019

We need to install go on the ci machines. Or find_acquire needs to know how to get go on macOS and Linux.

@Rastaban

@vicroms
Copy link
Member

vicroms commented Oct 7, 2019

/azp run

@qis
Copy link
Contributor Author

qis commented Nov 15, 2019

@vicroms I don't mean to be rude, but it's unclear to me if there are any actions on my part that must be taken. If there is no interest in boringssl, it's perfectly fine to close this PR. :)

@vicroms
Copy link
Member

vicroms commented Nov 28, 2019

/azp run

@LilyWangL
Copy link
Contributor

/azp run

@qis qis requested a review from LilyWangL February 8, 2020 14:47
@LilyWangL
Copy link
Contributor

@qis Currently, this port build failed on arm with the error as below:

[7/431] C:\VS2017\Stable\VC\Tools\MSVC\14.22.27905\bin\Hostx64\arm64\cl.exe   /TP -DBORINGSSL_DISPATCH_TEST -DBORINGSSL_IMPLEMENTATION -DBORINGSSL_SHARED_LIBRARY -DNOMINMAX -DWIN32_LEAN_AND_MEAN -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_STL_EXTRA_DISABLED_WARNINGS="4774 4987" -IC:\vsts\_work\4\s\buildtrees\boringssl\src\4d1af57504-1834815d49\third_party\googletest\include -IC:\vsts\_work\4\s\buildtrees\boringssl\src\4d1af57504-1834815d49\crypto\..\include -utf-8 -Wall -WX  -wd4061 -wd4100 -wd4127 -wd4200 -wd4204 -wd4221 -wd4242 -wd4244 -wd4267 -wd4371 -wd4388 -wd4296 -wd4350 -wd4365 -wd4389 -wd4464 -wd4510 -wd4512 -wd4514 -wd4548 -wd4610 -wd4623 -wd4625 -wd4626 -wd4628 -wd4668 -wd4706 -wd4710 -wd4711 -wd4800 -wd4820 -wd5026 -wd5027 -wd5045  -w44265 /D_DEBUG /MDd /Z7 /Ob0 /Od /RTC1 /showIncludes /Focrypto\test\CMakeFiles\test_support_lib.dir\file_test.cc.obj /Fdcrypto\test\CMakeFiles\test_support_lib.dir\test_support_lib.pdb /FS -c C:\vsts\_work\4\s\buildtrees\boringssl\src\4d1af57504-1834815d49\crypto\test\file_test.cc
FAILED: crypto/test/CMakeFiles/test_support_lib.dir/file_test.cc.obj 
C:\VS2017\Stable\VC\Tools\MSVC\14.22.27905\bin\Hostx64\arm64\cl.exe   /TP -DBORINGSSL_DISPATCH_TEST -DBORINGSSL_IMPLEMENTATION -DBORINGSSL_SHARED_LIBRARY -DNOMINMAX -DWIN32_LEAN_AND_MEAN -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_STL_EXTRA_DISABLED_WARNINGS="4774 4987" -IC:\vsts\_work\4\s\buildtrees\boringssl\src\4d1af57504-1834815d49\third_party\googletest\include -IC:\vsts\_work\4\s\buildtrees\boringssl\src\4d1af57504-1834815d49\crypto\..\include -utf-8 -Wall -WX  -wd4061 -wd4100 -wd4127 -wd4200 -wd4204 -wd4221 -wd4242 -wd4244 -wd4267 -wd4371 -wd4388 -wd4296 -wd4350 -wd4365 -wd4389 -wd4464 -wd4510 -wd4512 -wd4514 -wd4548 -wd4610 -wd4623 -wd4625 -wd4626 -wd4628 -wd4668 -wd4706 -wd4710 -wd4711 -wd4800 -wd4820 -wd5026 -wd5027 -wd5045  -w44265 /D_DEBUG /MDd /Z7 /Ob0 /Od /RTC1 /showIncludes /Focrypto\test\CMakeFiles\test_support_lib.dir\file_test.cc.obj /Fdcrypto\test\CMakeFiles\test_support_lib.dir\test_support_lib.pdb /FS -c C:\vsts\_work\4\s\buildtrees\boringssl\src\4d1af57504-1834815d49\crypto\test\file_test.cc
Microsoft (R) C/C++ Optimizing Compiler Version 19.22.27905 for ARM64
Copyright (C) Microsoft Corporation.  All rights reserved.

C:\vsts\_work\4\s\buildtrees\boringssl\src\4d1af57504-1834815d49\include\openssl/base.h(122): fatal error C1189: #error:  "Unknown target CPU"

Does this port support arm triplet?

@qis
Copy link
Contributor Author

qis commented Feb 10, 2020

@LilyWangL Strange, I specifically added boringssl:arm-uwp=fail and boringssl:x64-uwp=fail to scripts/ci.baseline.txt.

Not sure about ARM support on Windows. It does support ARM on Android.

@dan-shaw
Copy link
Contributor

/azp run

@dan-shaw
Copy link
Contributor

Since BoringSSL is a fork of OpenSSL, we might get ODR violations, so we might have to add skip to all the triplets in the baseline.

@qis
Copy link
Contributor Author

qis commented Feb 11, 2020

@dan-shaw Good call. I've changed it to use the same lines as libressl in scripts/ci.baseline.txt.

@LilyWangL LilyWangL added info:reviewed Pull Request changes follow basic guidelines and removed waiting for response labels Feb 11, 2020
@NancyLi1013 NancyLi1013 added waiting for response and removed info:reviewed Pull Request changes follow basic guidelines labels Feb 11, 2020
@qis
Copy link
Contributor Author

qis commented Feb 11, 2020

@NancyLi1013 Is there a tutorial on how to write ports?

I like and will do the changes, but since the usual approach is to grep existing ports for what you need, it results in sub-optimal PRs.

In the current vcpkg source tree, there are:

  • 1014 instances of include(vcpkg_common_functions)
  • 158 instances of VCPKG_CMAKE_SYSTEM_NAME STREQUAL .*WindowsStore
  • 34 instances of set(ENV{PATH}
  • 227 instances of IN_LIST FEATURES

This is not meant as criticism. I understand how things work and evolve. It's just a suggestion.

@NancyLi1013
Copy link
Contributor

Hi @qis
Thanks for your suggestion and detailed instances.
As these functions are added or removed not so long. Some ports haven't been changed in a short time. So there are some functions still kept as before.
We hope to update these functions or variables as the new ones when update or fix these ports.
It is also our suggestion not requirement.

We will try to update them later.
As for tutorial, you can refer to this doc currently. we also need to update and supplement new changes.

Thanks for your understand and support again.

@qis
Copy link
Contributor Author

qis commented Feb 11, 2020

@NancyLi1013 Thanks, that's a good article, but one thing is missing.

After applying the requested changes, I found out that vcpkg_check_features does not set actual CMake variables in the current directory scope, so I cannot do if(INSTALL_TOOLS) without looking for -DINSTALL_TOOLS=ON in the list (seems too hacky).

I solved the problem by installing executables directly into tools/boringssl in the CMakeLists.txt patch and checking if that directory exists before calling vcpkg_copy_tool_dependencies.

Is this the correct? What about ports, where you cannot easily change the executable install directory? Is the following solution better?

file(GLOB BSSL ${CURRENT_PACKAGES_DIR}/bin/bssl ${CURRENT_PACKAGES_DIR}/bin/bssl.exe)
if(BSSL)
  file(MAKE_DIRECTORY ${CURRENT_PACKAGES_DIR}/tools/boringssl)
  foreach(exe ${BSSL})
    # move bin/bssl to tools/boringssl/bssl
  endforeach()
endif()

P.S.: I tested it with x64-windows, x64-windows-static and x64-linux. Could somebody check if it actually works on OSX (since the CI won't do that now).

@dan-shaw
Copy link
Contributor

@qis I believe you can do if("tools" IN_LIST FEATURES) to check if it has been set. Also feel free to enable CI builds for MacOS is you need them. We just need to make sure it is changed to skip before merging.

@NancyLi1013
Copy link
Contributor

Hi @qis
Sorry for the late reply.
Could you please address the changes that @dan-shaw suggested above?

@qis
Copy link
Contributor Author

qis commented Mar 5, 2020

@NancyLi1013 But that's already covered by the CMakeLists.txt patch… My question was purely hypothetical in case I need it in another port. Do you want me to move that functionality to the portfile.cmake file?

@NancyLi1013
Copy link
Contributor

@qis
Sorry for the late reply again.
It seems that there is no need to update now.
Thanks for your patience.

Need to test the feature now.

@NancyLi1013
Copy link
Contributor

The feature has been tested passed with the following triplets:

  • x86-windows
  • x64-windows
  • x64-linux

@qis
Thanks for this PR.
It looks good to me now.

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Mar 20, 2020
@qis
Copy link
Contributor Author

qis commented Apr 9, 2020

Since I finally came around setting up my repaired MacBook Pro (Mid 2012), here is an update:

  • upstream commit 590265773@2020-04-07

Tested locally on Windows 10 with VS2019 16.5.3:

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

Tested locally on Ubuntu 18.04.4 LTS with GCC 7.5.0 and GCC 9.2.1:

  • x64-linux

Tested locally on OSX 10.15.3 and Apple Clang 11.0.3:

  • x64-osx

I will push a restored scripts/ci.baseline.txt once the CI finishes.

@qis
Copy link
Contributor Author

qis commented Apr 9, 2020

Do these Linux CI and OSX CI logs mean that the CI is broken? The Windows CI logs look good.

The scripts/ci.baseline.txt file is now restored.

@qis
Copy link
Contributor Author

qis commented Apr 9, 2020

Do I still need to change anything, or is everything in order?

@NancyLi1013
Copy link
Contributor

@qis
Thanks for your update.
There is nothing else need to do now. Let's wait for the PR to be merged.

@vicroms vicroms merged commit 1e19af0 into microsoft:master Apr 15, 2020
Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Apr 15, 2020
@qis qis deleted the dev/qis/boringssl branch April 15, 2020 06:48
@luncliff luncliff mentioned this pull request Jun 16, 2021
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants