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

[feature] Add is_cl_like() or improve is_msvc() method to perfectly mimic behavior of MSVC variable in CMake #12336

Open
1 task done
SpaceIm opened this issue Oct 18, 2022 · 15 comments

Comments

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 18, 2022

Currently conan provides is_msvc() method which returns True if compiler=Visual Studio or compiler=msvc in settings. In conan-center recipes, this method is widely used for different logics, and one of them is for some branching in package() and package_info() depending on MSVC variable in upstream CMakeLists (conditional lib name, renaming of files, injection of definitions etc).
But MSVC variable in CMake is not only True for Visual Studio, but also for any cl like compiler like clang-cl: https://cmake.org/cmake/help/latest/variable/MSVC.html
The problem is that depending on clang flavor on Windows, it may or may not be a cl like compiler for CMake. It leads to wrong assumption in several recipes, for example zlib where cpp_info.libs may be wrong depending on clang flavor: conan-io/conan-center-index#13474

For example a CMakeLists may have this:

add_library(mylib ...)
if(MSVC AND NOT BUILD_SHARED_LIBS)
    set_target_properties(mylib PROPERTIES OUTPUT_NAME "mylib-static")
endif()

so that in conan recipe we usually write:

    package_info(self):
        suffix = "-static" if is_msvc(self) and not self.options.shared else ""
        self.cpp_info.libs = [f"mylib{suffix}"]

It's very common, and it's broken for clang-cl.

So some folks using clang-cl were disappointed and began to put this kind of snippet in CCI recipes (and this time it's broken for non cl-like clang flavors on windows, so other folks complain that their clang flavor on windows doesn't work anymore):

    @property
    def _is_clang_cl(self):
        return self.settings.os == "Windows" and self.settings.compiler == "clang"

    package_info(self):
        suffix = "-static" if (is_msvc(self) or self._is_clang_cl) and not self.options.shared else ""
        self.cpp_info.libs = [f"mylib{suffix}"]

So it would be nice if conan could provide a method to perfectly mimic https://cmake.org/cmake/help/latest/variable/MSVC.html logic. It may be another method than is_msvc, or an additional argument to is_msvc in order to switch to CMake MSVC behavior.

@memsharded
Copy link
Member

The thing is that MSVC in CMakeLists.txt belongs to build time, while package_info() must work irrespective of the build. Then it is not possible to know the CMake generator that was used at package_info(), because also the binary would be the same irrespective of the generator. That means that the library name is not well defined for all cases, it seems there is nothing Conan can do to implement a correct is_cl_like() helper for all cases.

I'd say that this needs to be fixed at the package() level? Why not just renaming the lib (can also be done at build time, or even patch the build scripts)?

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Oct 19, 2022

Then it is not possible to know the CMake generator that was used at package_info()

MSVC value doesn't depend on generator, only compiler. It's deterministic at package_info() time as soon as compiler settings allow to know whether it's a cl like compiler.

I'd say that this needs to be fixed at the package() level? Why not just renaming the lib (can also be done at build time, or even patch the build scripts)?

In conan-center we honor lib file names of upstream. And moreover we can't patch upstream CMakeLists forever, libs maintainers won't accept a patch like that in their source code which would introduce a breaking change to lib name (due to a conan limitation).

@memsharded
Copy link
Member

MSVC value doesn't depend on generator, only compiler. It's deterministic at package_info() time as soon as compiler settings allow to know whether it's a cl like compiler.

No, it is not. The clang compiler, in Windows, building MSVC compatible binaries linking with the MSVC runtime and building with Ninja or MinGW Makefiles, does not have the MSVC variable set in CMakeLists.txt.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Oct 19, 2022

MSVC value doesn't depend on generator, only compiler. It's deterministic at package_info() time as soon as compiler settings allow to know whether it's a cl like compiler.

No, it is not. The clang compiler, in Windows, building MSVC compatible binaries linking with the MSVC runtime and building with Ninja or MinGW Makefiles, does not have the MSVC variable set in CMakeLists.txt.

You mean that the same compiler can work with Visual Studio generatorI, and it would set MSVC to TRUE? It's a complete mess, looks like a CMake bug.

@memsharded
Copy link
Member

Yes, if using Visual Studio generator, it uses the internal bundled ClangCL, but it is the same compiler as external LLVM/Clang, and they are binary compatible, so for Conan settings, they are the same, and independent from the actual CMake generator used.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Oct 19, 2022

I see. Well that's a big problem, because such complexity to properly handle libraries having MSVC branching (it's very common) can't be delegated to conan-center recipes (I mean like patching CMake files, renaming libs etc), it's too much work & it's fragile, just to handle a specific compiler.

@memsharded
Copy link
Member

I think that a rename in the package() method should work relatively well and without major issues.

Or why not collect_libs()? I really don't like it, but it is massively used also in ConanCenter, and that avoids completely deducing different names for libraries?

@SSE4
Copy link
Contributor

SSE4 commented Oct 19, 2022

conan build should be deterministic and predictable, it means:

  • library names produced by conan should fully match library names produced by the upstream. they shouldn't be renamed.
  • library names should be the same regardless of build parameters, such as CMake generator or compiler executable (such clang vs clang-cl)

collect_libs makes build unpredicated, on one machine it could produce one library name, on another machine it could produce a different one (e.g. if someone specific conf for CONAN_CMAKE_GENERATOR in his profile). it makes troubes and headache to troubleshoot that.

rename makes conan build diverge from the upstream, breaking user's expectations and changing behavior. it also requires people to change their makefiles (e.g. replace -lz via lzlib or whatever), means it's too intrusive.

@memsharded
Copy link
Member

collect_libs makes build unpredicated, on one machine it could produce one library name, on another machine it could produce a different one (e.g. if someone specific conf for CONAN_CMAKE_GENERATOR in his profile). it makes troubes and headache to troubleshoot that.

Should we ban collect_libs from ConanCenter, is that what you mean? Because it is used in >150 recipes.

In any case collect_libs collect the names created by the build system. This is nothing related to Conan. If the build system creates different names under different circumstances without being well-defined, the alternatives are:

  • Patch the build system to get the same name
  • Rename the files in package() method
  • Collect the generated library names, with collect_libs or some similar strategy.

rename makes conan build diverge from the upstream, breaking user's expectations and changing behavior. it also requires people to change their makefiles (e.g. replace -lz via lzlib or whatever), means it's too intrusive.

I don't care about divergence from the upstream if things are broken. The priority should be things should work, then match to upstream, not the opposite. If people are consuming Conan packages, it is also not expected that they will have -lzlib hardcoded in their build scripts. There are Conan generators to abstract away the dependencies information, it is impossible to match every upstream expectation and not requiring any change, ever in any consumer build script. And recall that we are talking about some CMake build scripts that will output a different library binary name depending on a very specific configuration of the build, even if they output clang binary compatible binaries. Do we really think that the name of Zlib library should be different if built with LLVM/Clang and Ninja generator and Visual Studio generator, if the compiler is always CLang?

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Oct 19, 2022

collect_libs makes build unpredicated, on one machine it could produce one library name, on another machine it could produce a different one (e.g. if someone specific conf for CONAN_CMAKE_GENERATOR in his profile). it makes troubes and headache to troubleshoot that.

Should we ban collect_libs from ConanCenter, is that what you mean? Because it is used in >150 recipes.

Not ban, but avoid as much as possible. We use collect_libs either for historical reasons, or because original recipe author was too lazy to find true lib name, or because it's not predictable based on settings/options (generator dependent for example, which is ugly I agree).

collect_libs is not used when there are components.

In any case collect_libs collect the names created by the build system. This is nothing related to Conan. If the build system creates different names under different circumstances without being well-defined, the alternatives are:

  • Patch the build system to get the same name
  • Rename the files in package() method
  • Collect the generated library names, with collect_libs or some similar strategy.

rename makes conan build diverge from the upstream, breaking user's expectations and changing behavior. it also requires people to change their makefiles (e.g. replace -lz via lzlib or whatever), means it's too intrusive.

I don't care about divergence from the upstream if things are broken. The priority should be things should work, then match to upstream, not the opposite. If people are consuming Conan packages, it is also not expected that they will have -lzlib hardcoded in their build scripts. There are Conan generators to abstract away the dependencies information, it is impossible to match every upstream expectation and not requiring any change, ever in any consumer build script. And recall that we are talking about some CMake build scripts that will output a different library binary name depending on a very specific configuration of the build, even if they output clang binary compatible binaries. Do we really think that the name of Zlib library should be different if built with LLVM/Clang and Ninja generator and Visual Studio generator, if the compiler is always CLang?

You don't take into account that downstream users are not always conan users, but also recipes based on build systems which are not aware of conan paradigm. We have to avoid patches as much as possible, because it's a maintenance burden.

@SSE4
Copy link
Contributor

SSE4 commented Oct 24, 2022

Should we ban collect_libs from ConanCenter, is that what you mean? Because it is used in >150 recipes.

I personally never liked and recommended this one since its inception, I wish it could be banned - it's completely non-transparent what does it capture (it may capture different things for different users) and causes issues (like, it may capture more libraries than needed - some libraries are not needed to be linked by default; or libraries with same name in different directories, or whatever). realistically speaking, bad might be too sound, but it might be flagged as a general warning and slowly deleted in new PRs.

Patch the build system to get the same name

IMO patch should always be a last measure - all these patches need to be maintained (=rebased for new releases), and they make behavior of conan build diverge from the upstream (which is a source of confusion by itself)

Rename the files in package() method

you may rename, but it might be too fragile (especially, for things like #pragma comment(lib) or dlopen in source code, whenever library name is significant) and cause even more bugs. but, I agree, sometimes it's unavoidable, e.g. if produced name are incorrect and cannot be used (like, generating .a for MSVC builds is in general a misbehavior). the same as with patch method, it causes conan build diverge from upstream.

Collect the generated library names, with collect_libs or some similar strategy.

about collect_libs - already answered.
but an actual and right strategy is to try to integrate conan closer with build systems such as CMake, and define some interoperability layer, e.g. with CMake File API, or similar approach for other build system. for many libraries, package_info already could be hundreds of lines long, and populated with a lot of complicated conditions to describe correct library names for all the possible options and settings combinations. very fragile, very hard to maintain. IMO working together with build system developers for better C++ ecosystem is the right way to go.

I don't care about divergence from the upstream if things are broken.

I don't like wording I don't care, but here it's a solid border to define what is broken. if upstream decided to do something, we follow, as they might have their own good reasons to do it (by default, we assume good will and no mistake, in other words, praesumptio innocentiae). if it's really a broken thing, we first report upstream and ask to correct. as a last stand, if upstream is not responsive, e.g. abandoned or closed project, we may try to fix ourselves, but that's an already a custom build, which is different from vanilla by its definition (= custom cci.date release).

The priority should be things should work, then match to upstream, not the opposite.

yes, working builds, but different from upstream, are better than no builds at all, or non-working builds. but here it's not the situation of non-working build, it's just a sophisticated upstream logic to select a library name.

If people are consuming Conan packages, it is also not expected that they will have -lzlib hardcoded in their build scripts.

they already used it even before conan, and will continue to use. conan might be used by some projects, but they still have a requirement to be able to be built without conan, with old-fashioned methods (like, with dependencies installed from apt-get or whatever).

Do we really think that the name of Zlib library should be different if built with LLVM/Clang and Ninja generator and Visual Studio generator, if the compiler is always CLang?

IMO, it's not our business to decide. upstream chooses to produce library names for their own logic and their own use-cases, and we should follow. for instance, in some build systems (e.g. b2) you can build x86 and x64 together in one shot, and ask b2 to encode architecture into the library name as a tag, so libraries for the same architecture may perfectly co-live in the same folder. in case of zlib, underlying compiler might not be the only important thing between these two builds, what if they activate different compiler flags or preprocessor definitions in these two builds? they might not be 100% compatible.
if we still think different library name per generator is abuse or misuse, let's report it upstream and see their motivation. if they don't answer, we can try to fix ourselves, but again, people using -lzlib, #pragma comment lib, dlopen or similar interfaces might be affected by that patch (despite the fact we all dislike hard-coding library names anywhere, and recommending higher level abstractions, like pkg-config or cmake find package, it's still widely encountered situation).

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Dec 7, 2022

In the same vein, a helper is_mingw() would be very useful, I still don't know how to detect from a properly written profile if compiler comes from MinGW or is a cl like compiler, so we can't predict what to write in package_info() when there is a logic like if(MINGW) in a CMakeLists.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Dec 8, 2022

I read https://blog.conan.io/2022/10/13/Different-flavors-Clang-compiler-Windows.html, and from my understanding current settings in conan are not sufficient to disambiguate each clang flavor in a recipe. Different clang flavors share the same settings, so recipes can't know from settings if compiler is clang-cl or llvm-clang.
Without proper fields in profile to disambiguate, and helpers in conan, there is no hope for robust recipes support of these different clang windows flavors.

Helpers I would advice:

  • is_mingw (gcc & clang from MinGW, so basically if compiler is gcc, host os is windows, and compiler.libcxx exists)
  • is_cl_like (msvc, clang-cl, intel-cc)
  • is_llvm_clang (llvm-clang) ?

Improve msvc_runtime_flags & is_msvc_static_runtime to support compiler with a runtime attribute.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Jan 25, 2023

@memsharded there are really too many issues in conan-center related to compilation with different windows clang flavors. It's really important that conan profiles can explicitly express which flavor a user is using, so that recipes can rely on this information to go into correct recipe branch when it's relevant.
And it's not just about lib name depending on MSVC variable in CMake we could handle with collect_libs() (I don't even take into account recipes with many components where collect_libs() is a no go). There are several other build logic which can change depending on clang flavor:

@sykhro
Copy link

sykhro commented Mar 27, 2023

CMake has been supporting this with CMAKE_<LANG>_SIMULATE_ID ever since 3.0 for Intel ICL pretending to be MSVC. Since v3.14 CMAKE_<LANG>_COMPILER_FRONTEND_VARIANT is also supported.

What do you recommend end users do about this problem at the moment?

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

No branches or pull requests

4 participants