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

[zlib] zlib/1.2.12: Compilation with Clang on Windows produces the wrong library name (z.lib instead of zlib.lib) #13474

Closed
Adnn opened this issue Oct 14, 2022 · 18 comments · Fixed by #13597
Labels
bug Something isn't working

Comments

@Adnn
Copy link
Contributor

Adnn commented Oct 14, 2022

Description

To elaborate on the title:

  • When building zlib on Windows with MSVC, the produced library is named zlib.lib, as intended by all generators which try to locate a file with basename zlib.

  • If zlib is built with Conan 15 on Windows, the produced library is names z.lib instead, which breaks downstreams because they expect zlib basename.

Package and Environment Details

  • Package Name/Version: libpng/1.6.38
  • Operating System+version: Windows 10
  • Compiler+version: Clang 15 (scoop install llvm)
  • Conan version: conan 1.53.0

Conan profile

[settings]
arch=x86_64
arch_build=x86_64
build_type=Debug
compiler=clang
compiler.cppstd=20
compiler.version=15
os=Windows
os_build=Windows
[options]
[build_requires]
*: cmake/3.24.1
[env]
CONAN_CMAKE_GENERATOR=Unix Makefiles
[conf]
tools.cmake.cmaketoolchain:generator=Unix Makefiles

Steps to reproduce

Build zlib:

conan install -pr:h clang15-win-dev -pr:b build/Windows zlib/1.2.12@

Then locate the build package in the filesystem, and take a look under lib/ folder. It will contain z.lib for Clang builds, and zlib.lib for MSVC builds.

Logs

Click to expand log

It is more of filename issue than a log error per se.

@Adnn Adnn added the bug Something isn't working label Oct 14, 2022
@Malacath-92
Copy link

This breaks every library that depends on zlib (so you can imagine that is a lot) for me when I compile with clang + Ninja on Windows. I'm not quite sure if this is an issue on the conan recipe or the zlib cmake setup.

@HappySeaFox
Copy link
Contributor

Interesting. Just wondering. It seems that "UNIX" is somehow defined for this use-case? https://github.com/madler/zlib/blob/04f42ceca40f73e2978b50e93806c2a18c1281fc/CMakeLists.txt#L168

@Adnn
Copy link
Contributor Author

Adnn commented Oct 18, 2022

Interesting catch @HappySeaFox

From the documentation of UNIX variable, maybe CMake considers (at least the scoop's version of) Clang to be UNIX-like.

@Malacath-92
Copy link

If I build zlib myself using cmake .. -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ I get a zlib.lib output, so something is presumably interfering while building through the conan recipe as I am passing the same compiler via the env to conan (i.e. in CC and CXX)

@Malacath-92
Copy link

I am not used to looking at conan recipes, but guess this line just determines what file gets packaged, it doesn't affect the output. Is that correct?
https://github.com/conan-io/conan-center-index/blob/master/recipes/zlib/all/conanfile.py#L106-L109

@Malacath-92
Copy link

Malacath-92 commented Oct 18, 2022

On a closer look I believe the error must lie here where the wild assumption is made that any non-MSVC build must behave like a UNIX build: https://github.com/conan-io/conan-center-index/blob/master/recipes/zlib/all/patches/1.2.x/0001-fix-cmake.patch#L66-L71
This matches up with your observation @Adnn.

@SpaceIm pinging you as that change came from your end. What was the rationale behind the change and are there no regression tests or the like? I would gladly fix it myself, but I would appreciate some guidance (specifically for adding tests) because I have extremely limited time right now. It seems especially crucial to me that a library as ubiquitous as zlib has tests that verify it works on a broad set of compilers and platforms.

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 18, 2022

CCI and conan client don't properly support clang on Windows currently. Many recipes are broken for this compiler/os, and it's very hard to know which flavor of windows clang has been used based on conan profile. There were recent improvements in conan client, but I don't know how to handle this in a recipe.

The main issue we have usually is that MSVC variable in CMake may be or may not be True for clang/windows (it's true for all cl like compilers), but is_msvc conan helper doesn't match this condition for clang/windows, so a recipe may go into the wrong branch for clangcl.

see conan-io/conan#10955

/cc @memsharded @SSE4 for advices, because currently clangcl handling is complete mess in conan-center and we don't know how to properly detect it nor what a canonical profile for each flavor should look like.

Anyway, clangcl is not tested in c3i (like MinGW, Intel compiler, cross-build to Android, iOS, etc), so you can't expect premium support, anybody can break this settings in a PR.

@Malacath-92
Copy link

Malacath-92 commented Oct 18, 2022

This approach of not supporting clang on Windows is quite disappointing. I'd much rather be told by my tool that something isn't supported and error out rather than having random breakages in arbitrary packages. That aside I get that working with clang on Windows is a tad more annoying and tedious than MSVC but I don't quite understand the criteria for not testing/supporting it. I don't have numbers to back this up so if you like you can disregard this statement, but I wager that clang on Windows is used plenty enough to be a valuable inclusion to the conan teams thoughts and tests.

Edit: I should also not that the OP and I are not complaining about clang-cl not working on Windows but plain clang not working. The previous comment made it sound like we were complaining about clang-cl. Not that it matters much I reckon, but I still wanted to clarify.

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 18, 2022

Edit: I should also not that the OP and I are not complaining about clang-cl not working on Windows but plain clang not working. The previous comment made it sound like we were complaining about clang-cl. Not that it matters much I reckon, but I still wanted to clarify.

Yes, zlib recipe works for clang-cl (and before this special management it was broken for clang-cl but fine for other flavors) but not for other flavors of clang on Windows, because conan client didn't provide a predictable way to know this flavor from profile. Now it might be better since conan-io/conan#11492, but it requires conan 1.53.0 and it's not even deployed in c3i.

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 18, 2022

If zlib is built with Conan 15 on Windows, the produced library is names z.lib instead, which breaks downstreams because they expect zlib basename.

What do you mean? There is no standard name on Windows (maybe just for MinGW where it has always been libz.a/libz.dll.a/zlib1.dll in msys2/pacman, so we can consider these names as standard names): madler/zlib#652, so downstream shouldn't expect any specific name but rely on pkgconf or FindZLIB.cmake or a plenty of different flavors. For example if you take a look at FindZLIB.cmake, they try many different names in order to locate zlib binary.

If you mean that package_info() is broken for clang on Windows (if not clang-cl), yes it's true (self.cpp_info.libs may be different than lib name installed under lib folder).

If I build zlib myself using cmake .. -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ I get a zlib.lib output, so something is presumably interfering while building through the conan recipe as I am passing the same compiler via the env to conan (i.e. in CC and CXX)

Yes, but zlib recipe has a CMakeLists patch to change this name depending on the compiler, because it produces libzlib.dll.a or libzlibstatic.a for MinGW for example, and they are not the canonical names for MinGW. zlib build systems (CMake, autotools, NMake, MSBuild) are not consistent, that's the problem. It's explained in madler/zlib#652

@SSE4
Copy link
Contributor

SSE4 commented Oct 18, 2022

yes, you basically shouldn't care if it's named z.lib, libz.lib, zlib.lib, libzlib.lib, zlibstatic.lib, libzlibstatic.lib, libzlibstaticd.lib, zlib.dll.lib, zlibstat.lib, zlibwapi.lib, zlibd1.lib or whatever crazy name it has for the certain given configuration/profile. the name of the library file on disk could be arbitrary - it's an implementation detail.
it's recommended to use higher-level abstraction, such as pkg-config or cmake find module / cmake config file, instead of hard-coding library name into your makefile.
and fortunately, conan also abstracts all these details for you, so you don't need to worry about naming. in the end, you only need zlib symbols (like _deflate function) to be resolved on linking stage during the build of your application.

@Adnn
Copy link
Contributor Author

Adnn commented Oct 18, 2022

To confirm, I am indeed also using non -cl clang,

it's recommended to use higher-level abstraction, such as pkg-config or cmake find module / cmake config file, instead of hard-coding library name into your makefile.

@SSE4 We are using Conan's CMakeDepth generator, and this produces files expecting zlib.lib name. I think that @SpaceIm stated that those are indeed broken:

If you mean that package_info() is broken for clang on Windows (if not clang-cl), yes it's true.

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 18, 2022

I've opened conan-io/conan#12336. I believe that we can't do anything robust currently without such method.

@Malacath-92
Copy link

@SSE4 I might also not have been clear, but my issue is with other recipes that fail linking against zlib during build because they expect zlib.lib. User code is not having any issues. I will check which package(s) is having issues and whether it can be resolved on that end for me, but I still assume the intention here was to build the same library name on Windows period, which is what the other package (s) expect.

@SSE4
Copy link
Contributor

SSE4 commented Oct 19, 2022

@Malacath-92 that sounds like discrepancy between the reality (how file could be named on disk) and conan's package_info in zlib recipe.
package_info might be (in some general case) disconnected from the build method, as it could be highly customized (e.g. CMake generator might affect on file name, or environment variable like CC/CXX, or usage of MSBuild vs CMake vs Autotools as specified in madler/zlib#652).
in general, any makefile (or build script) is turing complete, and can do crazy things where output file name depends on some arbitrary thing. that's not always easy to emulate and replicate correctly in package_info method - complexity is growing very fast, and you can easily hit package_info method of thousands lines long, still not fully correct.
better solution could be some flawless interoperability with build systems, like conan using CMake File API to query actual target and file names directly from the CMake (or other build system). that way, it's not needed to replicate and duplicate logic - you just once query the filename of the library, but logic to choose and compute crazy filename is executed only once and only in the right place.

@Adnn
Copy link
Contributor Author

Adnn commented Oct 19, 2022

better solution could be some flawless interoperability with build systems, like conan using CMake File API to query actual target and file names directly from the CMake (or other build system). that way, it's not needed to replicate and duplicate logic.

A bit "higher level" than this specific issue, but I love this idea. Having package_info automatically and correctly populated while keeping it DRY with CMake scripts, that would be some major step forward IMHO.

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 19, 2022

Here is a proposal: #13597

@Neustradamus
Copy link

Good job @SpaceIm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
6 participants