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: Visual Studio 2022 #9370

Merged
merged 22 commits into from
Aug 30, 2021
Merged

Feature: Visual Studio 2022 #9370

merged 22 commits into from
Aug 30, 2021

Conversation

SSE4
Copy link
Contributor

@SSE4 SSE4 commented Aug 3, 2021

closes: #9363
closes: #9362
closes: #9300
closes: #9461

/cc @thorbenk

compiler version is 16.30 (_MSC_VER=1930)
toolset is v143

tested on different packages

  • cmake: zlib/1.2.11 - works (after installing CMake 3.21)
  • autotools: libiconv/1.16 - works
  • msbuild: lcms/2.10 - works
  • nmake: libjpeg/9d - works
  • meson: fribidi/1.0.10 - works
  • custom: boost/1.76.0 - works

Changelog: Feature: Visual Studio 2022
Docs: omit

  • 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.

Copy link
Contributor

@lasote lasote left a comment

Choose a reason for hiding this comment

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

Looking good! :)

@SSE4 SSE4 marked this pull request as ready for review August 4, 2021 12:59
version: ["19.0",
"19.1", "19.10", "19.11", "19.12", "19.13", "19.14", "19.15", "19.16",
"19.2", "19.20", "19.21", "19.22", "19.23", "19.24", "19.25", "19.26", "19.27", "19.28",
"19.3", "19.30"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@memsharded there is some important thing, which concerns me a lot. it's not directly related to VS2022, but to msvc compiler versioning scheme in general.
I just realized it than I've seen we maybe need to add 19.29 to the list.
check out https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=vs-2019
TLDR: _MSC_VER 19.28 maps to the two versions (16.8 and 16.9), and 19.30 maps to the two versions as well (16.10 and 16.11)
our current versioning scheme won't be able to express that.
maybe we need to create a new issue and discuss if we want to change? since msvc is experimental, it's not late.

Copy link
Contributor

Choose a reason for hiding this comment

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

But we are ok with msvc.version right? I mean if 19.28 maps both to 16.8 and 16.9 means that both versions "use the same compiler version", so the model for msvc is good. The reverse mapping can choose the version we want. How do you think this could affect? @SSE4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it may hit some users who use LTTG and binaries aren't compatible for 16.8 vs 16.9
perhaps, they will need to add more sub-settings, like 19.28.29500?
more info is by links from #3573 (comment):
https://developercommunity.visualstudio.com/t/the-169-cc-compiler-still-uses-the-same-version-nu/1335194#T-N1337120
https://developercommunity.visualstudio.com/t/how-to-select-vs-168-compiler-with-vcvars-after-up/1359197?from=email

it's irrelevant for vs2022 anyway, but maybe we can discuss if it's an issue to be addressed in conan.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I see. What a mess...

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'd say we should stop adding features and support for new (experimental, not released yet) compilers in the legacy toolchains. I think it is time to add this only to the new toolchains, and to start encouraging even more their use, otherwise we will have a problem.

Wdyt @lasote?


flag = {"14": v14, "17": v17, "20": v20}.get(str(cppstd), None)
flag = {"14": v14, "17": v17, "20": v20, "23": v23}.get(str(cppstd), None)
Copy link
Member

Choose a reason for hiding this comment

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

These changes are missing in conan.tools._compilers, please make sure they are done there too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@SSE4
Copy link
Contributor Author

SSE4 commented Aug 9, 2021

I'd say we should stop adding features and support for new (experimental, not released yet) compilers in the legacy toolchains.

what exactly does it mean? no VS2022 support for the old CMake build helper? sounds like a terrible idea for me

otherwise we will have a problem.

can you explain, what kind of problem? I don't think there is a risk to break something

conans/client/conf/__init__.py Outdated Show resolved Hide resolved
@memsharded
Copy link
Member

what exactly does it mean? no VS2022 support for the old CMake build helper? sounds like a terrible idea for me

can you explain, what kind of problem? I don't think there is a risk to break something

We will start migrating to the new toolchains in ConanCenter very soon. We will change the docs to start using the new toolchains too. Conan 2.0 will not support the legacy integrations anymore. Yet, you are adding a feature that is only supported in the legacy generators, and you forgot to add support and testing for the new ones (the missing cppstd flag). This is the problem. From now on, all the features, changes, improvements should go to the new toolchain first, and only if it makes sense and we decide to implement in the legacy ones, we will do it, but only after the new ones have been done.

@SSE4 SSE4 closed this Aug 11, 2021
@arttet arttet mentioned this pull request Aug 22, 2021
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.

There was no need to close this, it could have added tests for the modern integrations, then it would be fine to keep the legacy changes, as they were already done. The key part of my review is that:

  • The feature must be supported by the new integrations: CMakeToolchain/CMakeDeps/MSBuildToolchain, etc
  • The feature must be tested for the new integrations.

Once these are met, I wouldn't oppose to fix it in the legacy integrations, specially if the job has been already done.

Would you please re-open or open a new PR for this? Thanks!

@tapia
Copy link
Contributor

tapia commented Aug 25, 2021

@SSE4 I'm taking the liberty to reopen this. The way I see this, @memsharded isn't rejecting the work done on this PR, he is just asking to add support for the new integrations, which are going to be the priority from now on.

Adding support to the legacy generators without adding the new ones at the same time will only delay the adoption of the new ones, and that's not what we're aiming for.

@tapia tapia reopened this Aug 25, 2021
conans/client/conf/__init__.py Outdated Show resolved Hide resolved
@SSE4
Copy link
Contributor Author

SSE4 commented Aug 26, 2021

The feature must be tested for the new integrations.

CI has no VS2022 installed. I also don't have any recipe using new integrations to test.
there are some tests in conan\conans\test\functional\toolchains\microsoft, do you want me to reconfigure them to VS2022 or make a copies of them for VS2022 and mark as skipped (as they won't pass in CI)?

also, there are some unit tests, e.g. conan\conans\test\unittests\tools\microsoft\test_msbuild.py
it uses GCC to test MSBuild integration (does it make any sense ever?):

def test_msbuild_cpu_count():
    c = ConfDefinition()
    c.loads(textwrap.dedent("""\
        tools.microsoft.msbuild:max_cpu_count=23
        tools.build:processes=10
    """))

    settings = MockSettings({"build_type": "Release",
                             "compiler": "gcc",
                             "compiler.version": "7",
                             "os": "Linux",
                             "arch": "x86_64"})
    conanfile = ConanFileMock()
    conanfile.settings = settings
    conanfile.conf = c.get_conanfile_conf(None)

    msbuild = MSBuild(conanfile)
    cmd = msbuild.command('project.sln')

    assert '/m:23' in cmd

should I update them for VS2022?

@memsharded
Copy link
Member

No, please do not modify or change existing tests, they should keep working, we are not removing support for older visual studio versions.

We have merged new infrastructure in conftest.py please have a look. You can tag @pytest.mark.tool_visual_studio(version="17"), then disable that specific version in conftest.py and enable in your conftest_user.py.

A different thing is when we install 16 or 17 in CI, it becomes the default and some tests might fail, but that is a different issue.

@SSE4
Copy link
Contributor Author

SSE4 commented Aug 26, 2021

No, please do not modify or change existing tests, they should keep working, we are not removing support for older visual studio versions.

so, make a copies and mark with @pytest.mark.tool_visual_studio(version="17"), right?

@SSE4
Copy link
Contributor Author

SSE4 commented Aug 29, 2021

FYI some mappings are redundant, and some are duplicated across multiple places in the codebase:

cppstd:

  • conan/tools/_compilers.py
  • conans/client/build/cppstd_flags.py

vs version -> ide year:

  • conan/tools/cmake/toolchain.py
  • conans/client/build/cmake_flags.py
  • conans/client/tools/win.py

msvc version -> vs version:

  • conan/tools/microsoft/toolchain.py
  • conan/tools/microsoft/visual.py
  • conans/model/info.py

msvc version -> toolset:

  • conan/tools/microsoft/toolchain.py
  • conans/client/tools/win.py

vs version -> toolset:

  • conan/tools/microsoft/toolchain.py

toolset -> vcvars version:

  • conan/tools/microsoft/visual.py

toolset -> vs version:

  • conans/client/tools/win.py

Signed-off-by: SSE4 <[email protected]>
@memsharded
Copy link
Member

memsharded commented Aug 29, 2021

FYI some mappings are redundant, and some are duplicated across multiple places in the codebase:

Yes, we know, we need them to be duplicated, or almost duplicated, as minor changes are being introduced in the new integrations and the legacy integrations are being removed in develop2 branch, will disappear in 2.0

@@ -396,6 +398,12 @@
"""


vs_versions = [{"vs_version": "15", "msvc_version": "19.1", "ide_year": "2017", "toolset": "v141"}]
Copy link
Member

Choose a reason for hiding this comment

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

The functional tests for XXXDeps do not need to be repeated for every compiler version, they are practically independent of the VS version, and they are very slow. The way is to parameterize the test with the default VS version, and run only once. If anything some fast unit-test, that doesn't build anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they seem to be dependent on VS version to some degree: e.g. they pass -s compiler.version at very least, also, it seems like they invoke the actual build with the actual compiler.
(I was expecting generator tests to just generate some files and validate their contents, but these tests seems to be going to verify the actual use case of consuming generating output and trying to use it with the real solution).
more, the toolchain right now tries to evaluate vcvars (_write_conanvcvars), so it wants a valid mapping of toolset -> vcvars version, and it actually even tries to compute vs_installation_path (from vcvars_path) - yes, debugger hits that from the tests.
so practically speaking, these tests actually depend on the different VS versions.
and yes, they are very slow, but CI skips vs2022 tests anyway, so right now it's not an issue IMO.

def test_toolchain_win(self, compiler, version, runtime, cppstd):
def test_toolchain_win_vs2017(self, compiler, version, runtime, cppstd):
if self.vs_version != "15":
pytest.skip("test for Visual Studio 2017")
Copy link
Member

Choose a reason for hiding this comment

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

Better put the skip as @pytest.mark, even if the @parameterized_class(vs_versions) is removed and a bit of duplication of the parameterized happens for the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to make it work different ways, starting from copying the whole file, realizing it will be a nightmare to support, then tried some thing with parameterized. the main problem here I can't write:

@parameterized_class(vs_versions)
@pytest.mark.tool_visual_studio(version=vs_version)
class MSBuildGeneratorTest(unittest.TestCase):

or

    @parameterized.expand(self.vs_combo)
    def test_toolchain_win(self, compiler, version, runtime, cppstd):

in other words, I don't see how can I access self or cls from the decorator. it seems like decorator should be evaluated on the earlier state, thus it cannot reference class internals. I don't want to dig into this right now too much, it's already taking time, but current state works and provides tests for both vs2017 and vs2022 (as it was requested), also makes it relatively easy to add new version on demand.
so, @pytest.mark (or other decorator) cannot reference the version it needs to skip from the class, so it's much easier for now to skip inside the function without fighting with python internals.

@SSE4
Copy link
Contributor Author

SSE4 commented Aug 30, 2021

Yes, we know, we need them to be duplicated, or almost duplicated, as minor changes are being introduced in the new integrations and the legacy integrations are being removed in develop2 branch, will disappear in 2.0

okay, right now it makes maintenance much harder than necessary - like updating ~10 files instead of just one, but we don't expect any new VS release besides 2022 any time soon (hopefully), so practically it's pain already suffered for now :)
I would love to move all mappings to the single file somewhere under conan/tools/microsoft, but it's definitely out of scope right now.

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

Successfully merging this pull request may close these issues.

[feature] MSVC C++ 14.29 [bug] cmake generator does not support Visual Studio 17 2022 Support for VS 2022
4 participants