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.

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

cmake: change import library name on non Visual Studio platform #6225

Closed
wants to merge 1 commit into from
Closed

cmake: change import library name on non Visual Studio platform #6225

wants to merge 1 commit into from

Conversation

@vtorri
Copy link
Contributor

@vtorri vtorri commented Nov 19, 2020

This change is motivated by the usage of pkg-config on MSYS2.
Indeed, when 'pkg-config --libs libcurl' is used, -lcurl is
passed to ld. The documentation of ld on Windows :

https://sourceware.org/binutils/docs/ld/WIN32.html

lists, in the 'direct linking to a dll' section, the pattern
of the searched import library, and libcurl_imp.lib is not there.

@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Nov 20, 2020

  1. This change may break all the methods of finding curl that rely on the old name
  2. It introduces yet another name that has to be checked. I'd say if we want to change the name, let's change it for all the way.

@vtorri
Copy link
Contributor Author

@vtorri vtorri commented Nov 20, 2020

@jzakrzewski so only .dll.a ?

@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Nov 20, 2020

@vtorri yes, if we want the change at all. I'm afraid, however, that I'm not qualified to say if the rename should be applied at all. For that I'd rather hear what others think.

@vtorri
Copy link
Contributor Author

@vtorri vtorri commented Nov 20, 2020

ok, i'll wait for comments, then

@jay
Copy link
Member

@jay jay commented Nov 21, 2020

Almost all the libraries for Visual Studio use .lib, and we should keep doing that. At the time possibly nobody thought about using cmake with other compilers in Windows. MinGW does use .a extension so it seems reasonable to use it there. Perhaps we should just let the linker pick, I'm not sure why we're not doing that...

@vtorri
Copy link
Contributor Author

@vtorri vtorri commented Nov 22, 2020

@jay i've tried to remove that part and compile with MSYS2 + mingw-w64

git diff :

diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt
index def8a0d93..0420e6fe1 100644
--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -115,18 +115,6 @@ if(CURL_HAS_LTO)
     INTERPROCEDURAL_OPTIMIZATION_RELWITHDEBINFO TRUE)
 endif()

-if(WIN32)
-  if(BUILD_SHARED_LIBS)
-    if(MSVC)
-      # Add "_imp" as a suffix before the extension to avoid conflicting with the statically linked "libcurl.lib"
-      set_target_properties(${LIB_NAME} PROPERTIES IMPORT_SUFFIX "_imp.lib")
-    else()
-      # With MSYS(2), use classic ".dll.a" suffix
-      set_target_properties(${LIB_NAME} PROPERTIES IMPORT_SUFFIX ".dll.a")
-    endif()
-  endif()
-endif()
-
 target_include_directories(${LIB_NAME} INTERFACE
   $<INSTALL_INTERFACE:include>
   $<BUILD_INTERFACE:${CURL_SOURCE_DIR}/include>)

script to compile

#! /bin/sh

rm -rf builddir && mkdir builddir && cd builddir

export PKG_CONFIG_DIR=
export PKG_CONFIG_LIBDIR=/opt/curl/lib/pkgconfig
export PKG_CONFIG_SYSROOOT_DIR=/opt/curl

cmake \
    -DCMAKE_TOOLCHAIN_FILE=../cross_toolchain.txt \
    -DCMAKE_INSTALL_PREFIX=/opt/curl \
    -DCMAKE_VERBOSE_MAKEFILE:BOOL=TRUE \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_C_FLAGS="-O2 -pipe -D__USE_MINGW_ANSI_STDIO=0" \
    -DCMAKE_EXE_LINKER_FLAGS="-s" \
    -DCMAKE_SHARED_LINKER_FLAGS="-s" \
    -DBUILD_CURL_EXE=OFF \
    -DENABLE_INET_PTON=OFF \
    -DCURL_TARGET_WINDOWS_VERSION=0x0601 \
    -DUSE_NGHTTP2=ON \
    -DCURL_ZSTD=ON \
    -DCMAKE_USE_SCHANNEL=ON \
    -G "Unix Makefiles" \
    ..

make -j install

cross compilation file (otherwise cmake considers it's a UNIX system) :

set(CMAKE_SYSTEM_NAME Windows)
set(CMAKE_SYSTEM_PROCESSOR AMD64)
set(CMAKE_C_COMPILER x86_64-w64-mingw32-gcc)
set(CMAKE_CXX_COMPILER x86_64-w64-mingw32-g++)
set(CMAKE_RC_COMPILER x86_64-w64-mingw32-windres)

installation output

Install the project...
E:/Documents/programmes_x64/msys2/mingw64/bin/cmake.exe -P cmake_install.cmake
-- Install configuration: "Release"
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/lib/libcurl.dll.a
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/bin/libcurl.dll
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/bin/curl-config
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/lib/pkgconfig/libcurl.pc
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/include/curl
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/include/curl/curl.h
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/include/curl/curlver.h
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/include/curl/easy.h
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/include/curl/mprintf.h
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/include/curl/multi.h
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/include/curl/options.h
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/include/curl/stdcheaders.h
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/include/curl/system.h
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/include/curl/typecheck-gcc.h
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/include/curl/urlapi.h
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/lib/cmake/CURL/CURLTargets.cmake
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/lib/cmake/CURL/CURLTargets-release.cmake
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/lib/cmake/CURL/CURLConfigVersion.cmake
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/lib/cmake/CURL/CURLConfig.cmake

@vtorri
Copy link
Contributor Author

@vtorri vtorri commented Nov 22, 2020

also, a bit later in this CMakeLists.txt i see

install(TARGETS ${LIB_NAME}
  EXPORT ${TARGETS_EXPORT_NAME}
  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
  RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
)

again, cmake is tweaked a bit. Is it really necessary ?

@jay jay added the cmake label Nov 22, 2020
@jay
Copy link
Member

@jay jay commented Nov 22, 2020

I don't know. We will need cmake people (contributors familiar with cmake) to chime in. Another possibility is in Visual Studio since you can have a static lib and an implicit lib maybe there is a naming conflict, unlike mingw. So libcurl.lib (static) and libcurl_imp.lib (dll)

@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Nov 23, 2020

My uderstanding is that the name conflict between the static and import library (thank you MS for that) is why we rename the thing. Most libraries I know actually just use different name for the static library so there's no conflict and all the names are simply generated by the build system. Here the sole fact that we have "curl" and "libcurl" targets is problematic.

@vtorri
Copy link
Contributor Author

@vtorri vtorri commented Nov 23, 2020

afaics, you can't build both static and library with the project generated by cmake. toplevel cmake, at line 1472 and later shows this. So i don't think the name is a problem

to paraphrase Pedro Alves, one of the dev of gcc on Windows : dll are good on Windows (he implies no static lib on Windows)

@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Nov 23, 2020

That's correct. Many projects try work around this limitation by providing two CMake targets - one for static and one for dynamic library. But I think curl is rather concerned about installation - with the default naming scheme on Windows it's impossible to to install static and dynamic versions because the import library will conflict with the static one.

@jay
Copy link
Member

@jay jay commented Jan 26, 2021

@jzakrzewski what do you think we should do here, is this PR necessary?

@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Jan 26, 2021

Strictly necessary - probably not. It's, however, good to do things in "more standard" way. This renaming is only necessary because curl went the route of keeping static library name the same as dynamic. Renaming anything at this point is a trade-off between breaking something and enabling something else.

From CMake code perspective the change is OK. I don't touch Windows unless I absolutely have to, so I cannot say if this change brings more benefits or sorrow.

@jay
Copy link
Member

@jay jay commented Jan 26, 2021

Ok thanks for your input, I've tagged it next-feature-window.

@jay
Copy link
Member

@jay jay commented Jan 26, 2021

-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/lib/libcurl.dll.a
-- Installing: E:/Documents/programmes_x64/msys2/opt/curl/bin/libcurl.dll

@vtorri to follow up on this, what you are saying is .dll.a is applied without your change, is that correct? Then maybe it should be just if(MSVC) to address only that.

@vtorri
Copy link
Contributor Author

@vtorri vtorri commented Jan 27, 2021

@jay

without my patch, libcurl_imp.lib is always installed (VS or msys2)
with my patch, libcurl_imp.lib is installed with VS and libcurl.dll.a is installed otherwise

what is important, for me, is that with msys2, or when cross-sompiling on linux for windows, we can use pkg-config --cflags --libs libcurl

also note that with msys2 and the autotools, libcurl.dll.a is installed :

libtool: install: /usr/bin/install -c .libs/libcurl.dll.a E:/Documents/programmes_x64/msys2/opt/ewpi2_64/lib/libcurl.dll.a

@jay
Copy link
Member

@jay jay commented Jan 27, 2021

what i mean is can we remove the .dll.a since according to your earlier post that is added automatically for msys

 if(WIN32)
   if(BUILD_SHARED_LIBS)
-    # Add "_imp" as a suffix before the extension to avoid conflicting with the statically linked "libcurl.lib"
-    set_target_properties(${LIB_NAME} PROPERTIES IMPORT_SUFFIX "_imp.lib")
+    if(MSVC)
+      # Add "_imp" as a suffix before the extension to avoid conflicting with the statically linked "libcurl.lib"
+      set_target_properties(${LIB_NAME} PROPERTIES IMPORT_SUFFIX "_imp.lib")
+    endif()
   endif()
 endif()

@vtorri
Copy link
Contributor Author

@vtorri vtorri commented Jan 27, 2021

@jay i've extracted latest release of curl and i've done your change. Seems good :

-- Installing: E:/Documents/programmes_x64/msys2/opt/curl2/lib/libcurl.dll.a

you can commit your patch to git, i'll close this PR once done

thank you

@vtorri
Copy link
Contributor Author

@vtorri vtorri commented Feb 2, 2021

@jay so ?

@jay
Copy link
Member

@jay jay commented Feb 2, 2021

It's tagged next-feature-window so that means when the feature window opens in a week or so I will add it.

- Use _imp.lib suffix only for Microsoft's compiler (MSVC).

Prior to this change library suffix _imp.lib was used for the import
library on Windows regardless of compiler.

With this change the other compilers should now use their default
suffix which should be .dll.a.

This change is motivated by the usage of pkg-config on MSYS2.
Indeed, when 'pkg-config --libs libcurl' is used, -lcurl is
passed to ld. The documentation of ld on Windows :

https://sourceware.org/binutils/docs/ld/WIN32.html

lists, in the 'direct linking to a dll' section, the pattern
of the searched import library, and libcurl_imp.lib is not there.

Closes #6225
@deepcode-ci-bot
Copy link

@deepcode-ci-bot deepcode-ci-bot bot commented Feb 2, 2021

Congratulations 🎉. DeepCode analyzed your code in 0.245 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@jay jay closed this in d4a3b87 Feb 9, 2021
@jay
Copy link
Member

@jay jay commented Feb 9, 2021

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants