wxWidgets / wxWidgets

Cross-Platform C++ GUI Library
https://www.wxwidgets.org/
5.77k stars 1.7k forks source link

CYGWIN: DLL files must be installed into bin. #24483

Closed carlo-bramini closed 3 weeks ago

carlo-bramini commented 4 weeks ago

I compiled successfully wxWidgets for CYGWIN with CMake. However, it happened that DLL files are installed into the lib directory instead of bin. Hopefully, the fix is very easy because runtime_dir just needs to be set to bin when CYGWIN is detected, exactly like WIN32. This PR fixes this issue.

vadz commented 4 weeks ago

I'm not actually sure what is the correct installation directory for Cygwin.

Where does configure install the libraries there? I thought it was lib, too.

carlo-bramini commented 3 weeks ago

I'm not actually sure what is the correct installation directory for Cygwin.

Where does configure install the libraries there? I thought it was lib, too.

executables and DLLs are into the bin directory. include files are into the include directory. inport libraries (*.dll.a) and static libraries (*.a) are into lib directory.

This is the same behaviour for both CYGWIN and MinGW.

vadz commented 3 weeks ago

Thanks, if this makes CMake consistent with configure, let's apply it.

@MaartenBent Would you have any objections or suggestions for improvement by chance?

MaartenBent commented 3 weeks ago

I would add brackets so it is clear what is part of AND and OR. Though in this case I don't think it doesn't really matter how it is grouped.

But WIN32_MSVC_NAMING should be 0 when using CYGWIN. So if(WIN32 AND NOT WIN32_MSVC_NAMING) should already succeed without this change... https://github.com/wxWidgets/wxWidgets/blob/ee309e078a2294d59c754b79c0407184e7558738/build/cmake/functions.cmake#L17-L21

vadz commented 3 weeks ago

Hmm, indeed, thanks for noticing this. @carlo-bramini Do you have any idea/could you please debug what's going on here, i.e. what is the value of WIN32_MSVC_NAMING and if it's really 1, how does this happen?

MaartenBent commented 3 weeks ago

Or does CYGWIN not define WIN32?

carlo-bramini commented 3 weeks ago

But WIN32_MSVC_NAMING should be 0 when using CYGWIN. So if(WIN32 AND NOT WIN32_MSVC_NAMING) should already succeed without this change...

https://github.com/wxWidgets/wxWidgets/blob/ee309e078a2294d59c754b79c0407184e7558738/build/cmake/functions.cmake#L17-L21

That's right, following that code into the script, I expect that WIN32_MSVC_NAMING will be zero, but this is not enough because WIN32 is undefined when configuring with CMake for CYGWIN. BTW, is there a particolar reason because it had been written here:

if(WIN32 AND NOT CYGWIN AND NOT MSYS)

instead of:

if (MSVC)

?

Or does CYGWIN not define WIN32?

That's right, when CYGWIN is defined, WIN32 is not.

carlo-bramini commented 3 weeks ago

If there are some doubts, let's take a look to this tiny script:

project(test_cygwin)

if (WIN32)
    message(WARNING "WIN32 defined")
else()
    message(WARNING "WIN32 undefined")
endif()

if (CYGWIN)
    message(WARNING "CYGWIN defined")
else()
    message(WARNING "CYGWIN undefined")
endif()

Running CMake, will print these messages:

-- The C compiler identification is GNU 11.4.0
-- The CXX compiler identification is GNU 11.4.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++.exe - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Warning at CMakeLists.txt:6 (message):
  WIN32 undefined

CMake Warning at CMakeLists.txt:10 (message):
  CYGWIN defined

-- Configuring done (3.8s)
-- Generating done (0.0s)
PBfordev commented 3 weeks ago

BTW, is there a particolar reason because it had been written here: if(WIN32 AND NOT CYGWIN AND NOT MSYS) instead of if(MSVC)

Yes, there is. CMake build on Windows stays compatible library naming-wise with the age-old "traditional" Windows builds using the bundled makefiles for MSVC and MinGW/GCC (and formerly also other compilers such as BCC).

MaartenBent commented 3 weeks ago

Indeed, it is to exclude certain environments/compilers on Windows from using the msvc naming convention. In hindsight we could have also used if(MSVC OR MINGW).

vadz commented 3 weeks ago

Thanks for your answers!

So what's the conclusion, should we just apply this PR or should we somehow simplify/streamline MSVC/MinGW tests in addition/instead of doing this?

MaartenBent commented 3 weeks ago

I think we can change this check to if(MSYS OR CYGWIN) and leave the rest as-is.

carlo-bramini commented 3 weeks ago

I think we can change this check to if(MSYS OR CYGWIN) and leave the rest as-is.

Excuse me, are you talking about modifying this line: https://github.com/wxWidgets/wxWidgets/blob/ee309e078a2294d59c754b79c0407184e7558738/build/cmake/functions.cmake#L456 that is the line modified by this PR, into:

if(MSYS OR CYGWIN)

Actually, this will fix CYGWIN, but MINGW (which has WIN32 defined) also needs to have the DLLs copied into bin directory of its sysroot. I could be wrong, but I suspect that this change will break it.

MaartenBent commented 3 weeks ago

That is the line I was talking about.

For MinGW we try to create the same results as makefile.gcc, which puts the files in /lib/gcc_dll.

vadz commented 3 weeks ago

Thanks, I'll apply

diff --git a/build/cmake/functions.cmake b/build/cmake/functions.cmake
index 699c4873ec..403f84c6bc 100644
--- a/build/cmake/functions.cmake
+++ b/build/cmake/functions.cmake
@@ -453,7 +453,7 @@ macro(wx_add_library name)

         # Setup install
         set(runtime_dir "lib")
-        if(WIN32 AND NOT WIN32_MSVC_NAMING)
+        if(MSYS OR CYGWIN)
             # configure puts the .dll in the bin directory
             set(runtime_dir "bin")
         endif()

soon then — unless somebody tells me not to.

carlo-bramini commented 3 weeks ago

I just tested your change and it works fine on CYGWIN, so it is good for me. Perhaps, while we are on the topic, it would be worth to improve it just a bit with:

if(MSYS OR CYGWIN)
    # configure puts the .dll in the bin directory
    set(runtime_dir "bin")
else()
    set(runtime_dir "lib")
endif()

It doesn't change anything, but it looks a bit easier to read, in my opinion.