feat: Detect MSVC settings in Conan 2.0 way

[hwhsu1231]

trafficstars

According to the documentation, msvc will deprecate Visual Studio in Conan 2.X. Therefore, I improve conan_cmake_autodetect() function. And I already tests it locally. The following is the Test Log:

F:\GitRepo\cmake-conan>pytest tests.py
================================= test session starts =================================
platform win32 -- Python 3.10.7, pytest-7.1.3, pluggy-1.0.0
rootdir: F:\GitRepo\cmake-conan
collected 37 items

tests.py .................s...................                                   [100%]

====================== 36 passed, 1 skipped in 309.32s (0:05:09) ======================

[hwhsu1231]

The only remained settings that _conan_detect_compiler() haven't detected for msvc compiler is compiler.update. Still thinking about how to detect it. Any idea?

But, it seems fine without specifying compiler.update for now.

[hwhsu1231]

According to this issue from Kitware/CMake, it is recommended to use CMAKE_<LANG>_COMPILER_ARCHITECTURE_ID instead of MSVC_[C|CXX]_ARCHITECTURE_ID.

[hwhsu1231]

I tried to follow the Conventional Commits specification. So I modified some commit messages.

[hwhsu1231]

@czoido

How is this PR going? I hope this PR can be merged to develop branch as soon as possible since the conan_cmake_detect_msvc_runtime() function is required for detecting MSVC-based Clang compilers as well.

• compiler.runtime
• compiler.runtime_type

[hwhsu1231]

@czoido

I suggest that we rename those internal functions which are used in _conan_detect_compiler macro with conan_detect_ prefix. That is:

• conan_detect_unix_libcxx (from conan_cmake_detect_unix_libcxx)
• conan_detect_msvc_version (from _get_msvc_ide_version)
• conan_detect_msvc_runtime (from conan_cmake_detect_vs_runtime)

Later when implementing an internal function to detect compiler.runtime_version setting of Clang compilers, I will name it:

• conan_detect_clang_runtime_version

What do you think?

[hwhsu1231]

This PR should fix: https://github.com/conan-io/cmake-conan/issues/421

[hwhsu1231]

@czoido

When will this PR be merged? What else do I need to improve?

Should I need to rename the internal functions myself, or will Conan Team do so?

• conan_detect_unix_libcxx (from conan_cmake_detect_unix_libcxx)
• conan_detect_msvc_version (from _get_msvc_ide_version)
• conan_detect_msvc_runtime (from conan_cmake_detect_vs_runtime)

[hwhsu1231]

I've rebased this branch from the latest develop branch.

[hwhsu1231]

@czoido

Should I squash those commits into about 1~3 commits?

[czoido]

@czoido

Should I squash those commits into about 1~3 commits?

Hi @hwhsu1231,

No need to do anything on your side, we merge the PR's with squash.

Regarding the PR, we can't merge this as it is, because it will detect msvc instead of Visual Studio and will break users that use generators that don't support msvc. Maybe we can add an extra argument to conan_cmake_autodetect and make this opt-in. Something like:

conan_cmake_autodetect(CONAN_V2 ON)

[hwhsu1231]

@czoido

I think maybe it is time for using conan_version() to differentiate the implementation between Conan 1.0 and Conan 2.0 since Visual Studio will be deprecated in Conan 2.0 but still required in Conan 1.0 for now. For example, using conan_version() in _conan_detect_compiler() macro like the following. What do you think?

elseif ("${CMAKE_${LANGUAGE}_COMPILER_ID}" STREQUAL "MSVC"
        OR ("${CMAKE_${LANGUAGE}_COMPILER_ID}" STREQUAL "Clang" 
            AND "${CMAKE_${LANGUAGE}_COMPILER_FRONTEND_VARIANT}" STREQUAL "MSVC" 
            AND "${CMAKE_${LANGUAGE}_SIMULATE_ID}" STREQUAL "MSVC"))

    conan_version(CONAN_VERSION)
    if (${CONAN_VERSION} VERSION_LESS "2.0.0" AND NOT CONAN_V2)
        # Conan 1.0 without CONAN_V2 enabled. 
        # using 'Visual Studio' compiler settings.
    else ()
        # Conan 2.0 or Conan 1.0 with CONAN_V2 enabled.
        # using 'msvc' compiler settings.
    endif ()
endif ()

If so, I think we should add back the following internal functions for using Visual Studio compiler settings:

• _get_msvc_ide_version
• conan_cmake_detect_vs_runtime

[czoido]

Hi @hwhsu1231, The problem I see with that is that users won't be able to test with msvc using Conan 1.X, I think selecting the behaviour of the function between old and new would probably is a better option.

[hwhsu1231]

@czoido

How about the following implementations?

• In conan_cmake_autodetect():

  Click to expand

  function(conan_cmake_autodetect detected_settings)
      # Detect whether CONAN_V2 is specified with ON:
      # - If CONAN_V2 is specified with ON, then MODE_CONAN_V2 will be ON.
      # - Otherwise:
      #   - If current Conan version is 1.0, then set MODE_CONAN_V2 to OFF.
      #   - If current Conan version is 2.0, then set MODE_CONAN_V2 TO ON.
      set(autodetectOneValueArgs CONAN_V2)
      cmake_parse_arguments(MODE "" "${autodetectOneValueArgs}" "" ${ARGV})
      if (NOT MODE_CONAN_V2)
          conan_version(CONAN_VERSION)
          if (CONAN_VERSION VERSION_LESS "2.0.0")
              set(MODE_CONAN_V2 OFF)
          else ()
              set(MODE_CONAN_V2 ON)
          endif ()
      endif ()
      _conan_detect_build_type(${ARGV})
      _conan_check_system_name()
      _conan_check_language()
      _conan_detect_compiler(${ARGV})
      _collect_settings(collected_settings)
      set(${detected_settings} ${collected_settings} PARENT_SCOPE)
  endfunction()
  

• In MSVC condition statement of _conan_detect_compiler():

  Click to expand

  elseif ("${CMAKE_${LANGUAGE}_COMPILER_ID}" STREQUAL "MSVC"
          OR ("${CMAKE_${LANGUAGE}_COMPILER_ID}" STREQUAL "Clang" 
              AND "${CMAKE_${LANGUAGE}_COMPILER_FRONTEND_VARIANT}" STREQUAL "MSVC" 
              AND "${CMAKE_${LANGUAGE}_SIMULATE_ID}" STREQUAL "MSVC"))
  
      if (NOT MODE_CONAN_V2)
          ###########################################
          # Detect 'Visual Studio' compiler settings.
          ###########################################
      else ()
          ###########################################
          # Detect 'msvc' compiler settings.
          ###########################################
      endif ()
  endif ()
  

Therefore, users can acquire:

• Visual Studio compiler settings by default when using Conan 1.0.
• msvc compiler settings with CONAN_V2 ON when using Conan 1.0.
• msvc compiler settings by default when using Conan 2.0.

[hwhsu1231]

@czoido

I made the above-mentioned implementations. How about now? Is it able to be merged?

[hwhsu1231]

@czoido

I've rebased this branch from the latest develop branch.

[hwhsu1231]

@czoido

About renaming internal functions used inside _conan_detect_compiler macro, I was thinking about removing cmake string and adding _ prefix from them. That is:

• conan_cmake_detect_vs_version -> _conan_detect_vs_version
• conan_cmake_detect_vs_runtime -> _conan_detect_vs_runtime
• conan_cmake_detect_msvc_version -> _conan_detect_msvc_version
• conan_cmake_detect_msvc_runtime -> _conan_detect_msvc_runtime
• conan_cmake_detect_unix_libcxx -> _conan_detect_unix_libcxx

What do you think?

[hwhsu1231]

@czoido

I've rebased this branch from the latest develop branch.

[hwhsu1231]

@czoido - Execuse me. What else problems does this PR have?

[hwhsu1231]

@czoido @memsharded

It's about one and a half month since I submit this PR. I want to keep improving this repo. Hope Conan Team can merge this PR as soon as possible.

Thanks.

[prince-chrismc]

As per the readme, this project is a secondary to the main client and I am sure after the 2.0 release there will be more attention to these great contributions.

Thanks for understanding :heart:

[hwhsu1231]

@prince-chrismc

I understand that Conan Team is paying more attention on 2.0 release. However, since this PR is passed with the current CI/CD testing, I wonder what else does this PR need to be improved? If not, why not merge this PR? If some errors do happen thereafter, then we just submit another PR to fix it. Right?

[memsharded]

Hi @hwhsu1231

I understand that Conan Team is paying more attention on 2.0 release. However, since this PR is passed with the current CI/CD testing, I wonder what else does this PR need to be improved? If not, why not merge this PR?

Thanks very much for your contribution, but this is not how it works, not every green PR is merged, not even our own PRs, many of them also stay there long time, getting reviews, discussed, or just stuck until it becomes a higher priority. Things need to be reviewed, to see if they align with the design, with the maintainers view on the tool, consider possible side effects, future maintenance, etc, etc. Also this cmake-conan is not that thoroughly tested, so it better not rely only on CI but on reviews too. It is also true that large PRs also take more time to review.

If some errors do happen thereafter, then we just submit another PR to fix it. Right?

Some things might have a high risk of breaking, yes, things can be fixed later, but if possible it is better not to break in the first place.

[hwhsu1231]

I pushed a new commit, which is aim to fix: #457

[hwhsu1231]

I summarized the commits in this PR so far, so that Conan Team can squash them into a single commit if it's accepted.

feat: Detect MSVC settings in Conan 2.0 way (#454)

In conan.cmake:

* Rename internal functions/macros:

  * conan_cmake_detect_unix_libcxx() -> _conan_detect_unix_libcxx()
  * _get_msvc_ide_version()          -> _conan_detect_vs_version()
  * conan_cmake_detect_vs_runtime()  -> _conan_detect_vs_runtime()

* Add new internal functions/macros:

  * Add _conan_detect_msvc_runtime() function
  * Add _conan_detect_msvc_version() function
  * Add _conan_detect_arch() macro

* Modify conan_cmake_autodetect():

  * Apply _conan_detect_arch()
  * Remove detecting arch for MSVC compiler
  * Add optional CONAN_V2 arguments

* Apply _conan_detect_arch() in conan_cmake_settings()
* Add compiler.update and compiler.runtime_type in _collect_settings()

In tests.py:

* Add compiler. prefix in test_settings()
* Add = after {} in test_settings_removed_from_autodetect()

Closes: #421, #457

[hwhsu1231]

@czoido @prince-chrismc @memsharded

Dear Conan Team,

How is the progress of this PR? It's almost about 3 months since this PR was labled PR: Under review. If there are some other situations required to consider in this PR, please let me know. Maybe I can handle them myself. I just hope this project can be improved.

Sincerely.

[prince-chrismc]

As you are aware, this auxiliary project is on hold until after Conan 2.0 is out. I am sure your contributions will be reviewed in the near future :)

[hwhsu1231]

@memsharded @jcar87

Excuse me.

I saw that a new branch develop2 was created. Does that mean this Change/PR should be merged to that new branch?

If so, should I clsoe this PR and re-submit a new one to the develop2 branch?

[memsharded]

If so, should I clsoe this PR and re-submit a new one to the develop2 branch?

No, the develop2 is the one used for the experimentation of the new approaches for 2.0 for the team, please do not send PRs to it.

[hwhsu1231]

@memsharded

Will Conan Team keep maintaining the develop branch?

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}