diff mbox series

[Openvpn-devel,v3,3/4] Check if the -wrap argument is actually supported by the platform's ld

Message ID 20230712095529.570306-1-arne@rfc2549.org
State Accepted
Headers
Series None |

Commit Message

Arne Schwabe July 12, 2023, 9:55 a.m. UTC 
  This avoids build errors on macOS. Also the test_tls_crypt command works
just fine on FreeBSD with its linkers, so do not make that test Linux only.

Patch v2: allow running with old cmake version (cmake 3 on RHEL7 with EPEL
          is only 3.17)
Patch v3: add OPTIONAL keyword to Incldue required by some cmake versions

Change-Id: Id26676bdc576c7d3d6726afa43fe6c7a397c579b
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 CMakeLists.txt | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Frank Lichtenheld July 12, 2023, 11:10 a.m. UTC | #1 
On Wed, Jul 12, 2023 at 11:55:29AM +0200, Arne Schwabe wrote:
> This avoids build errors on macOS. Also the test_tls_crypt command works
> just fine on FreeBSD with its linkers, so do not make that test Linux only.
> 
> Patch v2: allow running with old cmake version (cmake 3 on RHEL7 with EPEL
>           is only 3.17)
> Patch v3: add OPTIONAL keyword to Incldue required by some cmake versions

Yeah, I also have no idea why you didn't need that for your CMake. Anyway,
it is correct according to the documentation. And it is definitely required
on Ubuntu 20.04.

Tested on Ubuntu 20.04 (cmake 3.16.3), Ubuntu 22.04 (cmake 3.22.1), and
checked build results for GHA.

Acked-by: Frank Lichtenheld <frank@lichtenheld.com>

> Change-Id: Id26676bdc576c7d3d6726afa43fe6c7a397c579b
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  CMakeLists.txt | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
Frank Lichtenheld July 12, 2023, 11:19 a.m. UTC | #2 
On Wed, Jul 12, 2023 at 01:10:53PM +0200, Frank Lichtenheld wrote:
> On Wed, Jul 12, 2023 at 11:55:29AM +0200, Arne Schwabe wrote:
> > This avoids build errors on macOS. Also the test_tls_crypt command works
> > just fine on FreeBSD with its linkers, so do not make that test Linux only.
> > 
> > Patch v2: allow running with old cmake version (cmake 3 on RHEL7 with EPEL
> >           is only 3.17)
> > Patch v3: add OPTIONAL keyword to Incldue required by some cmake versions
> 
> Yeah, I also have no idea why you didn't need that for your CMake. Anyway,
> it is correct according to the documentation. And it is definitely required
> on Ubuntu 20.04.
> 
> Tested on Ubuntu 20.04 (cmake 3.16.3), Ubuntu 22.04 (cmake 3.22.1), and
> checked build results for GHA.
> 
> Acked-by: Frank Lichtenheld <frank@lichtenheld.com>

Note: please merge after
"Mock openvpn_exece on win32 also for test_tls_crypt" to avoid
broken MinGW builds.

Regards,
Gert Doering July 17, 2023, 6:44 p.m. UTC | #3 
Tested with GHA mingw/msvc builds, and the resulting unit test runs - all
looks green.

(As a side note, we *build* test_tls_crypt on Windows now, but the GHA
"mingw unittests" runs do not seem to run it yet -> patch to build.yaml
missing?)

Your patch has been applied to the master branch.

commit 4ef76f0ee4e122dcd616e1b1e2d652562ab10756
Author: Arne Schwabe
Date:   Wed Jul 12 11:55:29 2023 +0200

     Check if the -wrap argument is actually supported by the platform's ld

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Message-Id: <20230712095529.570306-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26850.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2d0cd5dd0..7dae6655d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -16,6 +16,7 @@  find_package(PkgConfig REQUIRED)
 include(CheckSymbolExists)
 include(CheckIncludeFiles)
 include(CheckCCompilerFlag)
+include(CheckLinkerFlag OPTIONAL)
 include(CheckTypeSize)
 include(CheckStructHasMember)
 include(CTest)
@@ -564,18 +565,24 @@  if (BUILD_TESTING)
             )
     endif ()
 
-    if (NOT MSVC)
-        # MSVC does not support --wrap
+    # MSVC and Apple's LLVM ld do not support --wrap
+    # This test requires cmake >= 3.18, so check if check_linker_flag is
+    # available
+    if (COMMAND check_linker_flag)
+        check_linker_flag(C -Wl,--wrap=parse_line LD_SUPPORTS_WRAP)
+    endif()
+
+    if (${LD_SUPPORTS_WRAP})
         list(APPEND unit_tests
             "test_argv"
+            "test_tls_crypt"
             )
     endif ()
 
-    # These tests work on only on Linux since they depend on special linker features
+    # These tests work on only on Linux since they depend on special Linux features
     if (${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
         list(APPEND unit_tests
             "test_networking"
-            "test_tls_crypt"
             )
     endif ()