[libpcap] Enable compilation of libpcap port on x86-windows and x64-windows #10731

traversaro · 2020-04-07T17:17:21Z

This PR enables support for compiling the libpcap port on x86-windows and x64-windows, removing the need to use the outdated and unmaintained fork winpcap (see #10730).

As winpcap and libpcap install the same headers, this two port have been marked as not not compatible, and cannot be installed together.

What does your PR fix?

This PR provides an alternative solution to the issue #10730 .

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

The PR adds support for building the port libpcap for ports x86-windows and x64-windows. I did not updated the CI baseline as this port conflicts with another port (winpcap) so I am not sure how this should be handled.

Does your PR follow the maintainer guide?

Yes.

Neumann-A · 2020-04-07T22:57:43Z

ports/libpcap/portfile.cmake

+# pcap dows not install pkg-config files in Windows
+if(VCPKG_CMAKE_SYSTEM_NAME STREQUAL "")
+    file(READ ${CURRENT_PACKAGES_DIR}/lib/pkgconfig/libpcap.pc LIBPCAP_PC)
+    string(REGEX REPLACE \($|\n\)prefix=[^\n]+ \\1prefix=\"${CURRENT_INSTALLED_DIR}\" LIBPCAP_PC_FIXED "${LIBPCAP_PC}")


as discussed in #9861 (comment) the prefix should be removed or commented out from the *.pc file.

Probably better to patch out the if(NOT MSCV) which stops the pc file being generated.

as discussed in #9861 (comment) the prefix should be removed or commented out from the *.pc file.

Sorry, I am looking into #9861 (comment), but I am a bit confused about it. There is any specific reason why the prefix variable should be removed, instead of being defined in function of the ${pcfiledir} to obtained relocatable .pc files?

Related links:

https://web.archive.org/web/20190927173057/https://wp.colliertech.org/cj/?p=164

https://bugs.freedesktop.org/show_bug.cgi?id=62018

https://manpages.debian.org/stretch/pkg-config/pkg-config.1.en.html

Use pcfiledir for prefix in pkgconfig file google/googletest#1905

That looks even better. Wasn't aware that something like ${pcfiledir} exists (probably to focused search in setting the prefix instead of searching for relocatable pkg-config).
Wondering why this is not the default in automake builds.

Neumann-A · 2020-04-07T22:59:26Z

ports/libpcap/portfile.cmake

+
+# On Windows, remove any executable leftover in bin
+if(VCPKG_TARGET_IS_WINDOWS)
+    file(GLOB EXE ${CURRENT_PACKAGES_DIR}/bin/*.exe)


what kind of executables are these? Maybe move them to tools/${PORT} instead. Sometimes these executables are needed by other packages.

The only tool created is rpcapd . This was done mainly for consistency with the existing libpcap on Linux (see https://github.com/microsoft/vcpkg/blob/41a00d56f02dc38f98994ddf50d111b8edc42471/ports/winpcap/portfile.cmake#L105) and on the winpcap port, that both does not install the rpcapd tool. Adding installation for the rpcapd tool may be interesting, but I think it is best to limit the changes on a single PR . In any case, I think that indeed disabling the ENABLE_REMOTE option also on Windows is the correct way to proceed.

Neumann-A · 2020-04-07T23:00:45Z

ports/libpcap/portfile.cmake

+if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Windows")
+    vcpkg_find_acquire_program(BISON)
+    get_filename_component(BISON_PATH ${BISON} DIRECTORY)
+    vcpkg_add_to_path(${BISON_PATH})
+    vcpkg_find_acquire_program(FLEX)
+    get_filename_component(FLEX_PATH ${FLEX} DIRECTORY)
+    vcpkg_add_to_path(${FLEX_PATH})
+endif()


The if is not needed due to:

"libpcap currently requires the following libraries from the system package manager: flex libbison-dev These can be installed on Ubuntu systems via sudo apt install flex libbison-dev"

I am not sure why the fact that on Ubuntu these packages are installed via apt, makes the download of this tools in Windows not needed.

vcpkg_find_acquire_program is only doing a find_program on linux for flex and bison. If it is not found it says to install via apt.

Ah, so you mean that the check with if is not necessary, not that the vcpkg_find_acquire_program calls are not necessary, I got that right?

Yes, remove the if keep the calls to vcpkg_find_acquire_program

Neumann-A · 2020-04-07T23:02:06Z

ports/winpcap/portfile.cmake

 include(vcpkg_common_functions)

+if (EXISTS "${CURRENT_INSTALLED_DIR}/share/libpcap")
+    message(FATAL_ERROR "FATAL ERROR: libpcap and winpcap are incompatible.")
+endif()
+
 set(WINPCAP_VERSION 4_1_3)

 vcpkg_download_distfile(ARCHIVE


Is there a need to keep this in vcpkg if libpcap is superior?

To be honest, I have no idea about deprecation works at the vcpkg level, if you want to warn the users in some way of you simply remove the port.

PhoebeHui

@traversaro, thanks for the PR!

PhoebeHui · 2020-04-08T05:50:14Z

ports/libpcap/portfile.cmake

+    vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY)
+endif()
+
+if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Windows")


Could you use 'if(VCPKG_TARGET_IS_WINDOWS)' instead?

See https://github.com/microsoft/vcpkg/pull/10731/files#r405309522 .

PhoebeHui · 2020-04-08T05:51:30Z

ports/libpcap/portfile.cmake

+    message(FATAL_ERROR "FATAL ERROR: winpcap and libpcap are incompatible.")
+endif()
+
+if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Linux")


Could you use 'if(VCPKG_TARGET_IS_LINUX)' instead?

I think that checking the host system name is correct in this case, as flex and bison are tools that are necessary on the host machine, not on the target. If someone for example uses a community triplet for cross-compilation of Windows binary on Ubuntu, this message should still be printed even if the target system is Windows.

As @traversaro says, flex and bison are build-time tools that generate code. The text below should be changed to reflect that, e.g. by saying "tools" instead of "libraries". Additionally, "libbison-dev" should be plain old "bison", at least on Debian/Ubuntu.

There should probably be a corresponding 'if(VCPKG_TARGET_IS_WINDOWS)' block as well that mentions WinFlexBison, which is installable via Chocolatey.

There should probably be a corresponding 'if(VCPKG_TARGET_IS_WINDOWS)' block as well that mentions WinFlexBison, which is installable via Chocolatey.

I don't think that is necessary, as bison is automatically downloaded (at least on Windows) by vcpkg_find_acquire_program(BISON) .

Maybe we need VCPKG_HOST_IS_LINUX etc.

ports/libpcap/portfile.cmake

PhoebeHui · 2020-04-08T05:55:06Z

ports/winpcap/portfile.cmake

@@ -1,5 +1,9 @@
 include(vcpkg_common_functions)


Could you help remove 'include(vcpkg_common_functions)? this is deprecated.

Could you also bump the Version in CONTROL file?

Fixed in 6139be5 .

Neumann-A · 2020-04-08T06:41:24Z

While looking thorugh the CMakeLists.txt I found that there are possible extra dependencies:

on Windows: https://nmap.org/npcap/
(Npcap is the Nmap Project's packet sniffing (and sending) library for Windows. It is based on the discontinued WinPcap library, but with improved speed, portability, security, and efficiency)

general: OpenSSL

both could lead to future CI failures due to missing libraries in dependent ports.

traversaro · 2020-04-08T07:37:00Z

While looking thorugh the CMakeLists.txt I found that there are possible extra dependencies:

on Windows: https://nmap.org/npcap/
(Npcap is the Nmap Project's packet sniffing (and sending) library for Windows. It is based on the discontinued WinPcap library, but with improved speed, portability, security, and efficiency)

general: OpenSSL

both could lead to future CI failures due to missing libraries in dependent ports.

Good point. I think the safer route for now is just to patch the CMake to avoid the automatic enabling of this dependencies, what do you think?

Neumann-A · 2020-04-08T08:01:42Z

Good point. I think the safer route for now is just to patch the CMake to avoid the automatic enabling of this dependencies, what do you think?

If you are already patching them instead of removing the code add a option(WITH_OPENSSL "Build with OpenSSL" OFF) and change the if in the CMakeLists.txt and move the find_package call into said if. Is probably the same amount of work as removing but makes it easier to integrate it as a feature later.

traversaro · 2020-04-08T08:10:12Z

Good point. I think the safer route for now is just to patch the CMake to avoid the automatic enabling of this dependencies, what do you think?

If you are already patching them instead of removing the code add a option(WITH_OPENSSL "Build with OpenSSL" OFF) and change the if in the CMakeLists.txt and move the find_package call into said if. Is probably the same amount of work as removing but makes it easier to integrate it as a feature later.

Yes, that make sense.

traversaro · 2020-04-08T13:54:31Z

Good point. I think the safer route for now is just to patch the CMake to avoid the automatic enabling of this dependencies, what do you think?

If you are already patching them instead of removing the code add a option(WITH_OPENSSL "Build with OpenSSL" OFF) and change the if in the CMakeLists.txt and move the find_package call into said if. Is probably the same amount of work as removing but makes it easier to integrate it as a feature later.

It turns out that at least in 1.9.1 there is no OpenSSL dependency, so I just added the option for the npcap dependency for now.

traversaro · 2020-04-08T13:54:42Z

I think I addressed all the reviews.

Neumann-A · 2020-04-08T14:08:07Z

ports/libpcap/portfile.cmake

 file(WRITE ${CURRENT_PACKAGES_DIR}/lib/pkgconfig/libpcap.pc "${LIBPCAP_PC_FIXED}")

 file(READ ${CURRENT_PACKAGES_DIR}/debug/lib/pkgconfig/libpcap.pc DEBUG_LIBPCAP_PC)
-string(REGEX REPLACE \($|\n\)prefix=[^\n]+ \\1prefix=\"${CURRENT_INSTALLED_DIR}/debug\" DEBUG_LIBPCAP_PC_FIXED "${DEBUG_LIBPCAP_PC}")
-file(WRITE ${CURRENT_PACKAGES_DIR}/debug/lib/pkgconfig/libpcap.pc "${DEBUG_LIBPCAP_PC_FIXED}")
+string(REGEX REPLACE \($|\n\)prefix=[^\n]+ \\1prefix=\"\${pcfiledir}/../..\" DEBUG_LIBPCAP_PC_FIXED "${DEBUG_LIBPCAP_PC}")


missing correction of the include path

I saw that #9426 is already open on that, but if you prefer I can handle that as well here.

Fixed by 611f558 .

Neumann-A · 2020-04-08T14:14:42Z

ports/libpcap/portfile.cmake

        -DDISABLE_BLUETOOTH=ON
        -DDISABLE_DBUS=ON
        -DDISABLE_RDMA=ON
+        -DDISABLE_DAG=ON
+        -DDISABLE_SEPTEL=ON
+        -DDISABLE_SNF=ON
+        -DDISABLE_TC=ON
+        -DDISABLE_PACKET=ON
+        -DENABLE_REMOTE=OFF


Reason to disable so many features?
This directly influences the capabilities of the library since most of those influence PROJECT_SOURCE_LIST_C which is used to build the library.

Except for ENABLE_REMOTE (that was already disabled on Linux, and is disabled also on Windows for consistency) all the rest are not features, but are optional dependency that if found are automatically enabled unless we disabled them explicitly. If the port was build on a clean environment, all this options were effectively disabled, and we need to disable them to ensure that the build is reproducible in all possible environments.

Neumann-A · 2020-04-08T14:17:36Z

It turns out that at least in 1.9.1 there is no OpenSSL dependency, so I just added the option for the npcap dependency for now.

???

#
# OpenSSL/libressl.
#
find_package(OpenSSL)
if(OPENSSL_FOUND)
  #
  # We have OpenSSL.
  #
  include_directories(SYSTEM ${OPENSSL_INCLUDE_DIR})
  set(PCAP_LINK_LIBRARIES ${PCAP_LINK_LIBRARIES} ${OPENSSL_LIBRARIES})
  set(HAVE_OPENSSL YES)
endif(OPENSSL_FOUND)

there are also a lot of #ifdef HAVE_OPENSSL in the code.

Ok with -DENABLE_REMOTE=OFF you are basically removing the OpenSSL dependency -> but why are so many things now disabled

traversaro · 2020-04-08T14:23:32Z

???

#
# OpenSSL/libressl.
#
find_package(OpenSSL)
if(OPENSSL_FOUND)
  #
  # We have OpenSSL.
  #
  include_directories(SYSTEM ${OPENSSL_INCLUDE_DIR})
  set(PCAP_LINK_LIBRARIES ${PCAP_LINK_LIBRARIES} ${OPENSSL_LIBRARIES})
  set(HAVE_OPENSSL YES)
endif(OPENSSL_FOUND)

I may be missing something, but this snippet of code is not part of the 1.9.1 release of libpcap.

Neumann-A · 2020-04-08T14:25:56Z

I may be missing something, but this snippet of code is not part of the 1.9.1 release of libpcap.

ah okay downloaded the wrong commit. Is added later to the CMakeLists.txt

traversaro · 2020-04-16T09:17:59Z

Just to clarify, this PR should be ready for review.

guyharris · 2020-04-16T22:43:17Z

Note that, without Npcap, libpcap-on-Windows will not have capabilities equivalent to libpcap-on-Linux or libpcap-on-macOS or libpcap-on-any-other-UN*X, because it won't be able to capture network traffic. Current UN*Xes all have built-in network traffic capture mechanisms, which libpcap uses; Windows may have a mechanism that can be used for that purpose, but, if so, no code has been written to use it, so, instead, WinPcap and Npcap provide an NDIS driver to provide such a mechanism and a DLL to access the driver, which pcap-npf.c uses.

guyharris · 2020-04-16T22:46:15Z

It turns out that at least in 1.9.1 there is no OpenSSL dependency, so I just added the option for the npcap dependency for now.

1.9.1 doesn't support remote traffic capture over TLS; that's present only in the libpcap master branch.

TLS support isn't used, even if the master branch, if remote capture support isn't configured in. Libpcap defaults to disabling it on UN*Xes, as it increases the attack surface (malicious rpcapd), but defaults to enabling it on Windows, for backwards compatibility with WinPcap (which is where the remote capture stuff first appeared). I've audited the code somewhat and fixed issues, and some others have audited it and found additional now-fixed issues, so we may enable it by default at some point in the future.

traversaro · 2020-04-16T22:46:34Z

Note that, without Npcap, libpcap-on-Windows will not have capabilities equivalent to libpcap-on-Linux or libpcap-on-macOS or libpcap-on-any-other-UNX, because it won't be able to capture network traffic. Current UNXes all have built-in network traffic capture mechanisms, which libpcap uses; Windows may have a mechanism that can be used for that purpose, but, if so, no code has been written to use it, so, instead, WinPcap and Npcap provide an NDIS driver to provide such a mechanism and a DLL to access the driver, which pcap-npf.c uses.

Thanks for the additional info! In my application I use libpcap to load pcap files offline, so I am not concerned by this, but this is exactly the reason why I left the option to still install winpcap .

guyharris · 2020-04-16T22:56:26Z

Note that, without Npcap, libpcap-on-Windows will not have capabilities equivalent to libpcap-on-Linux or libpcap-on-macOS or libpcap-on-any-other-UN_X, because it won't be able to capture network traffic. Current UN_Xes all have built-in network traffic capture mechanisms, which libpcap uses; Windows may have a mechanism that can be used for that purpose, but, if so, no code has been written to use it, so, instead, WinPcap and Npcap provide an NDIS driver to provide such a mechanism and a DLL to access the driver, which pcap-npf.c uses.

Thanks for the additional info! In my application I use libpcap to load pcap files offline, so I am not concerned by this, but this is exactly the reason why I left the option to still install winpcap .

If there's a WinPcap package, perhaps there should also be an Npcap package.

traversaro · 2020-04-16T23:05:57Z

If there's a WinPcap package, perhaps there should also be an Npcap package.

I opened a separate issue for tracking if someone is intereted in working on an npcap port: #10873 .

azure-pipelines · 2020-05-22T10:30:42Z

Azure Pipelines successfully started running 1 pipeline(s).

PhoebeHui · 2020-05-22T11:02:54Z

libcrafter:x86-windows and libcrafter:x64-windows failed with following failures:

warning: gettext-0.19.8.1-1 is up to date -- skipping
error: cannot remove /usr/bin/msys-iconv-2.dll (Permission denied)
warning: warning given when extracting /usr/bin/msys-iconv-2.dll (Could not unlink)

@BillyONeal, could you help take a look?

JackBoosY

Please add the following codes to VCPKG_PATH/scripts/ci.baseline.txt:

libcrafter:x86-windows=fail
libcrafter:x64-windows=fail

JackBoosY · 2020-05-28T07:54:14Z

The libcrafter issuse will solved in another PR.

traversaro · 2020-05-28T08:04:08Z

Please add the following codes to VCPKG_PATH/scripts/ci.baseline.txt:
libcrafter:x86-windows=fail
libcrafter:x64-windows=fail

Done in 7a4aead .

dan-shaw · 2020-06-11T08:07:28Z

@traversaro I just merged the other PR #9426 as suggested. Could you resolve the conflicts?

traversaro · 2020-06-11T14:20:48Z

@traversaro I just merged the other PR #9426 as suggested. Could you resolve the conflicts?

Done.

As winpcap and libpcap install the same headers, this two port have been marked as not not compatible, and cannot be installed together.

dan-shaw · 2020-06-12T06:13:34Z

Thanks for the PR!

…indows (microsoft#10731) * Enable compilation of libpcap port on x86-windows and x64-windows As winpcap and libpcap install the same headers, this two port have been marked as not not compatible, and cannot be installed together. * Update ci.baseline.txt * Add libcrafter failing ports to ci.baseline.txt

traversaro mentioned this pull request Apr 7, 2020

Compiling against winpcap on a recent Visual Studio fails #10730

Closed

Neumann-A reviewed Apr 7, 2020

View reviewed changes

JackBoosY requested a review from PhoebeHui April 8, 2020 00:25

JackBoosY assigned PhoebeHui Apr 8, 2020

PhoebeHui requested changes Apr 8, 2020

View reviewed changes

PhoebeHui added the waiting for response label Apr 8, 2020

PhoebeHui changed the title ~~Enable compilation of libpcap port on x86-windows and x64-windows~~ [libpcap] Enable compilation of libpcap port on x86-windows and x64-windows Apr 8, 2020

Neumann-A mentioned this pull request Apr 8, 2020

[scripts] add new function vcpkg_fixup_pkgconfig #9861

Merged

Neumann-A reviewed Apr 8, 2020

View reviewed changes

traversaro mentioned this pull request Apr 16, 2020

[New Port Request] npcap #10873

Closed

traversaro mentioned this pull request Apr 22, 2020

Some ROS2 packages are not relocatable ros2/ros2#606

Closed

JackBoosY suggested changes May 28, 2020

View reviewed changes

JackBoosY added requires:author-response and removed requires:discussion labels May 28, 2020

JackBoosY requested a review from PhoebeHui May 28, 2020 09:54

JackBoosY approved these changes May 28, 2020

View reviewed changes

JackBoosY added info:reviewed Pull Request changes follow basic guidelines requires:discussion and removed requires:author-response labels May 28, 2020

traversaro force-pushed the enable-libpcap-win branch from 7a4aead to f7699f0 Compare June 11, 2020 14:20

traversaro force-pushed the enable-libpcap-win branch from f7699f0 to 8484d4e Compare June 11, 2020 14:37

traversaro added 3 commits June 11, 2020 16:50

Enable compilation of libpcap port on x86-windows and x64-windows

f8a498b

As winpcap and libpcap install the same headers, this two port have been marked as not not compatible, and cannot be installed together.

Update ci.baseline.txt

6bd3f11

Add libcrafter failing ports to ci.baseline.txt

1baffb5

traversaro force-pushed the enable-libpcap-win branch from 8484d4e to 1baffb5 Compare June 11, 2020 14:51

JackBoosY requested a review from dan-shaw June 12, 2020 02:35

PhoebeHui approved these changes Jun 12, 2020

View reviewed changes

dan-shaw merged commit 310f4df into microsoft:master Jun 12, 2020

traversaro mentioned this pull request Jun 12, 2020

Update vcpkg dependencies iit-danieli-joint-lab/idjl-software-dependencies-vcpkg#14

Merged

traversaro deleted the enable-libpcap-win branch June 12, 2020 06:47

[libpcap] Enable compilation of libpcap port on x86-windows and x64-windows #10731

[libpcap] Enable compilation of libpcap port on x86-windows and x64-windows #10731

Conversation

traversaro commented Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhoebeHui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geraldcombs Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neumann-A commented Apr 8, 2020

traversaro commented Apr 8, 2020

Neumann-A commented Apr 8, 2020

traversaro commented Apr 8, 2020

traversaro commented Apr 8, 2020

traversaro commented Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neumann-A commented Apr 8, 2020

traversaro commented Apr 8, 2020

Neumann-A commented Apr 8, 2020

traversaro commented Apr 16, 2020

guyharris commented Apr 16, 2020

guyharris commented Apr 16, 2020

traversaro commented Apr 16, 2020

guyharris commented Apr 16, 2020

traversaro commented Apr 16, 2020

azure-pipelines bot commented May 22, 2020

PhoebeHui commented May 22, 2020

JackBoosY left a comment

Choose a reason for hiding this comment

JackBoosY commented May 28, 2020

traversaro commented May 28, 2020

dan-shaw commented Jun 11, 2020

traversaro commented Jun 11, 2020

dan-shaw commented Jun 12, 2020

traversaro commented Apr 7, 2020 •

edited

Loading

geraldcombs Apr 16, 2020 •

edited

Loading

traversaro commented Apr 8, 2020 •

edited

Loading