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 settings for clang-cl (clang on Windows) #5705

Merged
merged 4 commits into from
Jul 28, 2020

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Sep 4, 2019

Changelog: Feature: Add settings for clang-cl (clang on Windows).
Docs: conan-io/docs#1784

related:
#1839
#6789
https://github.com/conan-community/community/issues/242
https://github.com/conan-community/community/issues/236
bincrafters/community#680

as clang on Windows is getting more popular (Clang is now used to build Chrome for Windows, Firefox switching to clang-cl for Windows builds), this configuration needs to have a first-class support in conan. this demand is also shown by growing number of issues related to Clang on Windows in conan-community/bincrafters.

currently, there are several possibilities to use Clang on Windows:

  • Clang/C2 toolset, which isn't something we consider, as The Clang/C2 experiment for C++ is ending
  • install LLVM binaries from the llvm.org (section Pre-Built Binaries, Windows)
  • install LLVM via pacman in MSYS2 (pacman -S mingw64/mingw-w64-x86_64-clang)

NOTE: users may currently use llmv* based toolsets for Visual Studio compiler, e.g. if they install corresponding extension or use preview version, but it has some disadvantages, as it works only for Visual Studio (MSBuild) projects, while the majority of packages use plain makefiles, autotools, CMake or other build systems like b2 which have no concept of MSVC toolsets.

to add more complexity, Clang on Windows may operates in two modes (--driver-mode):

  • cl - in this mode Clang tries to be compatible with Microsoft Visual Studio (MSVC) and accepts MSVC-style command line arguments, such as /Zi, /MT, etc. used by default ibyclang-cl.exe.
  • g++ - classic mode, where clang tries to accept GCC-style arguments, such as -m32, -g and so on. in other words, same behaviors, as observer on Linux and Mac. used by default by clang.exe

we clearly have to distinguish mentioned two modes, so:

  • conan doesn't try to add unsupported compiler flags, and we avoid warnings like clang-cl: warning: argument unused during compilation: '-O3' [-Wunused-command-line-argument]
  • recipe authors may distinguish easily between these two modes

besides these two modes, I also propose to add compiler.runtime for Clang, as it uses the same compiler runtime options (/MT, /MD, /MDd, /MTd) as MSVC

finally, I propose to add None to compiler.libcxx for the Clang on Windows, so we don't use -stdlib option, as it's not supported and redundant in the mentioned case (Clang uses the same standard library implementation as MSVC does, and doesn't use libc++ or libstdc++). right now conan adds -stdlib=libstdc++ which results in compiler warning: clang-cl: warning: unknown argument ignored in clang-cl: '-stdlib=libstdc++' [-Wunknown-argument]
possible alternative: use msstl to indicate Microsoft STL implementation?

this PR only propose settings changes, it doesn't include:

  • compiler flag management changes
  • compiler detection changes

these things will be added as soon as we agree on settings

annex 1:
typical conan profile for clang-cl currently looks like:

include(default)
[settings]
compiler=clang
compiler.version=8
compiler.libcxx=libstdc++
[options]
[env]
CC=clang-cl  -mrtm
CXX=clang-cl  -mrtm
CFLAGS= -fms-compatibility-version=1800
CXXFLAGS= -fms-compatibility-version=1800
CONAN_CMAKE_GENERATOR=Ninja
PATH=[C:\Program Files\LLVM\bin]
[build_requires]
ninja_installer/1.8.2@bincrafters/stable

or for clang.exe instead of clang-cl.exe:

include(default)
[settings]
compiler=clang
compiler.version=8
compiler.libcxx=libstdc++
[options]
[env]
CC=clang  -mrtm
CXX=clang  -mrtm
CFLAGS= -fms-compatibility-version=1800
CXXFLAGS= -fms-compatibility-version=1800
CONAN_CMAKE_GENERATOR=Ninja
PATH=[C:\Program Files\LLVM\bin]
[build_requires]
ninja_installer/1.8.2@bincrafters/stable

after changes made to the conan, profile will not change significantly, for clang-cl.exe:

include(default)
[settings]
compiler=clang
compiler.version=8
compiler.runtime=MD
compiler.driver=cl
[options]
[env]
CC=clang-cl  -mrtm
CXX=clang-cl  -mrtm
CFLAGS= -fms-compatibility-version=1800
CXXFLAGS= -fms-compatibility-version=1800
CONAN_CMAKE_GENERATOR=Ninja
PATH=[C:\Program Files\LLVM\bin]
[build_requires]
ninja_installer/1.8.2@bincrafters/stable

and for clang.exe:

include(default)
[settings]
compiler=clang
compiler.version=8
compiler.runtime=MD
compiler.driver=g++
[options]
[env]
CC=clang  -mrtm
CXX=clang  -mrtm
CFLAGS= -fms-compatibility-version=1800
CXXFLAGS= -fms-compatibility-version=1800
CONAN_CMAKE_GENERATOR=Ninja
PATH=[C:\Program Files\LLVM\bin]
[build_requires]
ninja_installer/1.8.2@bincrafters/stable
  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@lasote lasote modified the milestones: 1.21, 1.22 Nov 19, 2019
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that the main issue is the driver. It totally seems an argument that should be provided and managed accordingly by the build helpers, depending on the build system, for example. It doesn't really seem that the binary is different if being driven by the cl or the g++ interface. It seems possible to build some third party library package with makefiles, using the g++ interface and other package with cmake, that is using the cl interface. Not only possible, but also quite probable.

I am fine with adding runtime and libcxx with None.

What would be the way to manage it with VS toolset? Just adding the toolset? Will it be binary compatible with cmake clang compiled libraries? If yes, then we should probably take it into account, we can not design it in a way that will block native VS clang support.

conans/client/conf/__init__.py Outdated Show resolved Hide resolved
@memsharded memsharded self-assigned this Jan 8, 2020
@dmn-star
Copy link

@SSE4 Might be of interest to you VS2019 support clang out-of-the-box.
https://devblogs.microsoft.com/cppblog/clang-llvm-support-in-visual-studio/

The “Desktop development with C++” workload with the Visual Studio installer now includes full support for targeting Clang/LLVM based toolsets.

@memsharded
Copy link
Member

Hi @SSE4

There were some comments, questions and concerns above. I feel this is not ready to be in 1.22, so lets move it for 1.23, and lets find some time to keep discussing this.

@memsharded memsharded modified the milestones: 1.22, 1.23 Jan 29, 2020
@SSE4
Copy link
Contributor Author

SSE4 commented Jan 30, 2020

the newly added platform toolset is named LLVM (clang-cl).
the one provided by extension is just llvm.
previously it was LLVM-<vs.version> (e.g. llvm-vs2017).
the problem, if we want to add compatibility quirks (like vs_toolset_compatible() one), we can't deduce Clang version to match just from the settings (toolset name). we need to somehow locate compiler executable (non-trivial in case of VS toolset) and asks it for its version.

side note: Intel compiler adds its own toolsets as well (e.g. in my case it's Intel C++ Compiler 19.0, which is good, as it includes compiler version, so we can at least match it). but we currently don't do any toolset <-> compiler version mapping for Intel, right?

@SSE4
Copy link
Contributor Author

SSE4 commented Apr 8, 2020

NOTE: driver-mode doesn't introduce a new setting on its own, e.g. produced binaries are identical for clang.exe and clang-cl.exe. as well as for clang.exe and clang.exe --driver-mode=cl.
therefore, it doesn't belong to the settings and shouldn't be a part of default settings.yml (thus, I've removed it from this PR).
this should be something that our build helpers should consider when apply various flags (like -m32 and so on), but that's another topic for another PR.

@SSE4
Copy link
Contributor Author

SSE4 commented Apr 8, 2020

the important thing for package_id which matter is so called target.
given the simple source code:

int main()
{
#ifdef __GNUC__
#error "__GNUC__ is defined"
#else
#error "__GNUC__ is not defined"
#endif
    
#ifdef _MSC_VER
#error "_MSC_VER is defined"
#else
#error "_MSC_VER is not defined"
#endif
}

and clang from MSYS:

$ clang --version
clang version 3.8.0 (/repo/sources/clang 1d5b05f1ef9d1b9889ddb5ad946944f224a7ba88) (/repo/sources/llvm 2aebced35905eb3891eba484e4d1902cf7399558)
Target: x86_64-pc-msys
Thread model: posix
InstalledDir: /usr/bin

we get:

$ clang test.cpp
test.cpp:4:2: error: "__GNUC__ is defined"
#error "__GNUC__ is defined"
 ^
test.cpp:12:2: error: "_MSC_VER is not defined"
#error "_MSC_VER is not defined"
 ^
2 errors generated.

and with regular clang:

>"C:\Program Files\LLVM\bin\clang" --version
clang version 8.0.1 (tags/RELEASE_801/final)
Target: x86_64-pc-windows-msvc
Thread model: posix
InstalledDir: C:\Program Files\LLVM\bin

we get:

>"C:\Program Files\LLVM\bin\clang-cl" test.cpp
test.cpp(6,2): error: "__GNUC__ is not defined"
#error "__GNUC__ is not defined"
 ^
test.cpp(10,2): error: "_MSC_VER is defined"
#error "_MSC_VER is defined"
 ^
2 errors generated.

>"C:\Program Files\LLVM\bin\clang"  test.cpp
test.cpp:6:2: error: "__GNUC__ is not defined"
#error "__GNUC__ is not defined"
 ^
test.cpp:10:2: error: "_MSC_VER is defined"
#error "_MSC_VER is defined"
 ^
2 errors generated.

>"C:\Program Files\LLVM\bin\clang" --driver-mode=cl  test.cpp
test.cpp(6,2): error: "__GNUC__ is not defined"
#error "__GNUC__ is not defined"
 ^
test.cpp(10,2): error: "_MSC_VER is defined"
#error "_MSC_VER is defined"
 ^
2 errors generated.

related code is here

pls share what do you think/prefer about modelling target in settings.yml

maybe we can use the approach similar to the Intel (clang with compiler.base=Visual Studio)

@memsharded
Copy link
Member

@SSE4 @jgsogo

What I am seeing here is basically that these clang compilers might be basically incompatible between them. More like they are different compilers? Should we treat them as such?

@memsharded memsharded modified the milestones: 1.25, 1.26 Apr 30, 2020
@memsharded memsharded modified the milestones: 1.26, 1.27 Jun 3, 2020
@memsharded memsharded removed this from the 1.27 milestone Jun 30, 2020
@memsharded memsharded added this to the 1.28 milestone Jun 30, 2020
@SSE4
Copy link
Contributor Author

SSE4 commented Jun 30, 2020

@jgsogo if you can provide your opinion on this would be nice :) I don't think it's different compiler(s), more like sub-setting(s) to be modeled. WDYT?

@jgsogo
Copy link
Contributor

jgsogo commented Jul 1, 2020

I really find myself a bit confused about this. We agree that ABI incompatibilities should be encoded into settings, but here we have different compilers (different executables, maybe requiring an option) that generate identical binaries and I think it is useful to question if they should compute the same package-id unless an explicit sub-setting is given to differentiate between them... is this what you mean, @SSE4 ? ...but I'm afraid there is no way to do it right now in Conan: if a setting takes a different value, it will be encoded in a different package-id.

Maybe the relation 1-1 between a compiler entry in the settings and an executable is no longer true.

I feel like this PR being opened for so long (and the investigation we did about msvc compiler) is a symptom, there is something we are not modeling well enough in the settings.

IMO we need to have a look at the full picture before entering in detail, and probably it is something needed before Conan 2.0.

@SSE4
Copy link
Contributor Author

SSE4 commented Jul 1, 2020

@jgsogo yes, we have different executables (clang-cl.exe and clang.exe**) producing the identical binaries. still, in conan we have to distinguish them as they accept flags in the different format.
we also have clang.exe executables which produce incompatible binaries from the two mentioned previously. and these executables have to be differentiated with some (sub-)settings.

still, maybe we can go incremental and as a first iteration merge only proposed settings changes, which are quite simple, straightforward and safe? then, once you have some brilliant and smart idea on how to model the weirdness mentioned above, we can go with second PR.

@jgsogo
Copy link
Contributor

jgsogo commented Jul 2, 2020

Just as an example, I was struggling with the profile generation in CCI, trying to encode all the combinations in a YML file, trying to figure out how to forbid certain combinations (like here, not all toolchains can be combined with all the compilers)... and finally I came out with a evident solution: you can have repeated entries!

- os: ["Linux"]
  os_build: ["Linux"]
  arch: ["x86_64"]
  arch_build: ["x86_64"]
  compiler:
    - "gcc":
        compiler.version: ["4.9"]
        compiler.libcxx: ["libstdc++"]
        build_type: ["Release", "Debug"]
    - "gcc":
        compiler.version: ["5", "6", "7", "8", "9"]
        compiler.libcxx: ["libstdc++11", "libstdc++"]
        build_type: ["Release", "Debug"]
    - "clang":
        compiler.version: ["3.9", "4.0", "5.0", "6.0", "7.0", "8", "9"]
        compiler.libcxx: ["libstdc++", "libc++"]
        build_type: ["Release", "Debug"]

It will require changing how we read and interpret the settings from the settings.yml file, but it is easy to read and easy to avoid some incompatibilities.


I agree we should add clang to Windows, first step is to add it to the settings, then we need to work on all the helpers and tools to manage those values (if we want to), but these two things are not coupled: how to declare valid settings combinations and how to deal with the profiles once they arrive to the build_helpers/tools.

@memsharded
Copy link
Member

Having a look at this for the N-th time 😅

I like the idea of better representation of incompatibilities in the settings.yml, but I also think it can be a non-obvious implementation, and a bit of a challenge to introduce it without breaking, so probably needs to be something to consider for 2.0.

Regarding the definition of clang, lets move it forward. It will be the responsibility of the users to not select a wrong combination of settings in their profiles. I still wonder if separating a clang-win compiler, would be a better way, but no way to be sure so lets merge.

@memsharded memsharded merged commit 3867871 into conan-io:develop Jul 28, 2020
@memsharded
Copy link
Member

Please @SSE4 follow up with the related issues:

#1839
#6789
conan-community/community#242
conan-community/community#236
bincrafters/community#680

Thanks!

kralicky pushed a commit to kralicky/conan that referenced this pull request Jul 28, 2020
* - add settings for clang-cl (clang on Windows)

Signed-off-by: SSE4 <[email protected]>

* - add new VS toolsets for Clang
kralicky added a commit to kralicky/conan that referenced this pull request Jul 28, 2020
* Feature: add settings for clang-cl (clang on Windows) (conan-io#5705)

* - add settings for clang-cl (clang on Windows)

Signed-off-by: SSE4 <[email protected]>

* - add new VS toolsets for Clang

* fix migration test

* Feature/toolchain without dump (conan-io#7435)

* toolchain without dump()

* working

* fixing tests

Co-authored-by: SSE4 <[email protected]>
Co-authored-by: memsharded <[email protected]>
kralicky added a commit to kralicky/conan that referenced this pull request Aug 4, 2020
* Feature: add settings for clang-cl (clang on Windows) (conan-io#5705)

* - add settings for clang-cl (clang on Windows)

Signed-off-by: SSE4 <[email protected]>

* - add new VS toolsets for Clang

* fix migration test

* Feature/toolchain without dump (conan-io#7435)

* toolchain without dump()

* working

* fixing tests

* relax pluginbase requirements (conan-io#7441)

* New lockfiles iteration. Including version-only lockfiles. (conan-io#7243)

* try to reproduce error in lockfile

* iterate build_order

* package in requires and build_requires

* rename test file

* removing constraint multiple packages

* minor improvements

* first lock-recipe version working

* working

* working

* working 75%, but revisions not managed

* working

* working

* working...

* allow no user/channel match in --build

* build order

* working

* working

* working

* not working...

* working

* tests passing without revisions

* working

* review and patterns for rrevs

* new tests

* some fixes

* more fixes

* skip build-info

* fix revision test msg

* removed global activation of revisions

* multiple br test

* cli changes

* working

* new cli syntax

* fixing tests

* partial lockfiles

* CI partial lock working

* make sure partial lockfiles are used completely and from its root

* version ranges and py_requires working

* partial CI with python_requires

* comment

* tests passing

* working

* working in build_requires

* CI for build-requires test

* recovering conan_build_info tests

* broken py27

* clean-modified, stricter locks

* partial arranged

* minor style fixes

* compacting lockfile search for nodes

* remove unnecessary --build=missing

* processing --update

* testing the --update necessary for revisions non matching

* make the lockfile-out compulsory

* fixed conan_build_info

* forcing --lockfile-out

* workspace change

* Update conans/client/command.py

Co-authored-by: Javier G. Sogo <[email protected]>

* make lock_node.path relative

* new tests and fixes

* remove hardcoded os.environ

* make lockfile-out required

* fix test

* fixing os.path.relpath in Win

* more tests

* fix test

* fix test

* checking --lockfile-out requires --lockfile

* working

* review

* rename get_consumer

* a partial lock can match more than 1 require

* error msg

* working

* check that profile is not passed when using lockfiles

* working

* working

* recover profile_build from lockfile

* add new test for --update

* make sure modified is propagated

* with revisions

* format

* changed build-order order of output

* update cmd line

* review

* Update conans/client/command.py

Co-authored-by: Carlos Zoido <[email protected]>

* Update conans/client/command.py

Co-authored-by: Carlos Zoido <[email protected]>

* Update conans/client/command.py

Co-authored-by: Carlos Zoido <[email protected]>

* forbidding version ranges when using lockfiles, in command line

Co-authored-by: jgsogo <[email protected]>
Co-authored-by: Carlos Zoido <[email protected]>

* Support for components in pkg_config generator (conan-io#7413)

* Support for components in pkg_config generator

* capitalize

* remove code not needed

* use real pkg-config for tests

* try without skip

* [feature] Define file name for CMake find generators (conan-io#7320)

* adds `filenames` to cpp_info

Relates to [this issue](conan-io#7254)

* use `filenames` attribute, not name, as prefix to global vars

* `filenames` should fall back to `names`, not package name

* removes debug statements

* Makes `cmake_find_package_multi` work with `filenames` attribute

* adds a test for the filename attribute

* Update find_package generators to not use `filename` in global var names

* change `name` used in cmake_find_package_test, since sharing them isn't possible now

* Set both `{name}_FOUND` and `{filename}_FOUND` vars so code will work

* reverts `filename` of `name` in some spots (danimtb feedback)

* fixes bug in cmake_fine_package_multi_test

* adds a test for changing filename in cmake_find_package_multi generator

* use filename version of _FOUND in if statements...

This allows CMake exported target namespaces to be shared.

* set upper case version of _FOUND vars

* Add Conan version to HTML output (conan-io#7443)

* conan-io#7365 Add Conan version in search table

Signed-off-by: Uilian Ries <[email protected]>

* conan-io#7365 Add Conan version in info graph

Signed-off-by: Uilian Ries <[email protected]>

* conan-io#7365 Add shebang for python2.7

Signed-off-by: Uilian Ries <[email protected]>

* conan-io#7365 Use JS for current date

Signed-off-by: Uilian Ries <[email protected]>

* conan-io#7365 Do not rename old tests

Signed-off-by: Uilian Ries <[email protected]>

* conan-io#7365 Remove copyright symbol to allow py27

Signed-off-by: Uilian Ries <[email protected]>

* Refactor filenames and simplify tests (conan-io#7447)

* Refactor filenames and simplify tests

* fix

* small fixes

* review fix

* rename variables

* fix linux tests

* version 1.28.0

* version 1.29.0-dev

* fix migration test

* Hotfix/lockfile errors (conan-io#7453)

* improve error msgs

* better error msgs

* minor fixes to error msgs and better checks

* Fixes bad powershell separators on Linux (conan-io#7472)

* minor improvements in components checks (conan-io#7486)

* fix message missing components (conan-io#7483)

* fix message missing components

* changed error message

Co-authored-by: SSE4 <[email protected]>
Co-authored-by: memsharded <[email protected]>
Co-authored-by: David Roman <[email protected]>
Co-authored-by: jgsogo <[email protected]>
Co-authored-by: Carlos Zoido <[email protected]>
Co-authored-by: Daniel <[email protected]>
Co-authored-by: Tim Simpson <[email protected]>
Co-authored-by: Uilian Ries <[email protected]>
Co-authored-by: mjvankampen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants