[bug] tools.vcvars priority in PATH differers from behavior of vcvarsall

[puetzk]

Environment Details (include every applicable attribute)

• Windows 10
• Visual Studio 2017 15.9.19
• Conan version: 1.22.2
• Python version: 3.7.4

Steps to reproduce (Include if Applicable)

test_package with cmake_minimum_required(VERSION 3.16), which uses conans.CMake and its build() method, using CONAN_CMAKE_GENERATOR=Ninja

Logs (Executed commands with output) (Include/Attach if Applicable)

cmake-modules/0.9.5 (test package): Running build()

---

** Visual Studio 2017 Developer Command Prompt v15.9.19
** Copyright (c) 2017 Microsoft Corporation

---

[vcvarsall.bat] Environment initialized for: 'x64_x86'
CMake Error at CMakeLists.txt:1 (cmake_minimum_required):
CMake 3.16 or higher is required. You are running version
3.12.18081601-MSVC_2

Commit 048da96#diff-029e992e62f056e56357fa0ba4cf4111R220 (conan 1.22.0) changed conans.CMake._run from

conan/conans/client/build/cmake.py

Lines 219 to 220 in 9d990a6

	with tools.vcvars(self._settings, force=True, filter_known_paths=False,
	output=self._conanfile.output):

to

conan/conans/client/build/cmake.py

Lines 220 to 222 in 048da96

	vcvars_dict = tools.vcvars_dict(self._settings, force=True, filter_known_paths=False,
	output=self._conanfile.output)
	with _environment_add(vcvars_dict, post=self._append_vcvars):

This change supports the new append_vcvars mode, but it also subtly altered the behavior of the existing (default) mode. vcvarsall.bat does not give all of its settings equal precedece; it prepends cl and windows SDK tools at the front of %PATH% to ensure they're the ones found, but it appends its CMake/ninja binaries at the end, so they are are used only as a fallback; if you already have a different preferred version of CMake/ninja in PATH, they'll still be found before the binaries bundled with visual c++.

Having vcvars_dict(...,only_diff=True) (the default) determine the additions that vcvars made, and environment_append(post=False) re-insert them all prepended, which loses this sorting into "primary" vs "fallback" that microsoft made, and thus in conan 1.22.0 and up the CMake build helper selects the old Microsoft binary of CMake 3.12.18081601-MSVC_2, whereas 1.21 and older used the CMake binary in my %PATH% (which is 3.16). This seems counter to the intention of #6000, which was to give the vcvars outputs less precedence, and altered the default in a way that breaks any recipe that needs a new CMake but still supports older MSVC. I suspect this did not show up on conan-center-index builds because conan-package-tools also does the vcvars context manager, still using tools.vcvars(), and so in those cases the call in the CMake build helper finds things already set: https://github.com/conan-io/conan-package-tools/blob/94c083f211430f09eeab7e6cb2da052dc4d79be8/cpt/runner.py#L76-L85

I think the easiest fix would be to just have this code use vcvars_dict(..., only_diff = self._append_vcvars)`, so that it only separates a diff and re-inserts the PATH entries if it's intending to push them to the end, and otherwise preserves order Microsoft chose.

[memsharded]

Hi @puetzk

This is certainly unexpected, that change should be a pure refactor if the self._append_vcvars=False

I am trying to reproduce, and so far the behavior seems exactly the same in 1.22 and 1.21.

    def vcvars_precedence_build_env_test(self):
        # https://github.com/conan-io/conan/issues/6577
        settings = Settings.loads(default_settings_yml)
        settings.os = "Windows"
        settings.compiler = "Visual Studio"
        settings.compiler.version = "15"
        settings.arch = "x86"
        settings.arch_build = "x86_64"

        import os
        with environment_append({"PATH": ["MyPathToMyCMake"]}):
            path0 = os.getenv("PATH")
            with tools.vcvars(settings, force=True, filter_known_paths=False, output=self.output):
                path1 = os.getenv("PATH")
            # Set the env with a PATH containing the vcvars paths
            vcvars = tools.vcvars_dict(settings, force=True, filter_known_paths=False,
                                       output=self.output)
            with environment_append(vcvars):
                path2 = os.getenv("PATH")

        self.assertNotEqual(path0, path1)
        self.assertEqual(path1, path2)
        position_cmake_vs = path1.find(r"CommonExtensions\Microsoft\CMake\CMake\bin")
        position_my_cmake = path1.find("MyPathToMyCMake")
        self.assertGreater(position_my_cmake, position_cmake_vs)

In both versions, and no matter if using the refactored _environment_add(), they both pass with the same result.

To be honest, I am confused, it seems that vcvars has been always pre-prending its values, and thus always prioritizing its CMake and Ninja binaries. Actually, the purpose of #6000 was precisely to change that. I would like to understand this better soon, if it is a regression, I would like to put it into next 1.22.3 release. I keep investigating, but any help here would be very useful.

[memsharded]

Btw, following the steps above, it always fail, in Conan 1.22., 1.21, 1.20:

[vcvarsall.bat] Environment initialized for: 'x64'
CMake Error at CMakeLists.txt:1 (cmake_minimum_required):
  CMake 3.14 or higher is required.  You are running version
  3.12.18081601-MSVC_2

[memsharded]

And, using Conan 1.22 and opting into the new behavior:

def build(self):
        cmake = CMake(self, generator="Ninja", append_vcvars=True)
        # Current dir is "test_package/build/<build_id>" and CMakeLists.txt is
        # in "test_package"
        cmake.configure()
        cmake.build()

Then it works, and is able to build succesfully (adding CC=cl.exe and CXX=cl.exe)

[puetzk]

I'm definitely seeing the behavior of vcvarsall.bat splitting its additions between append and prepend:

> where cmake
C:\Program Files\CMake\bin\cmake.exe

> "C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x86
**********************************************************************
** Visual Studio 2017 Developer Command Prompt v15.9.19
** Copyright (c) 2017 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'x86'

> where cmake
C:\Program Files\CMake\bin\cmake.exe
C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake.exe

> cmd /v:on /c echo !PATH:%__VSCMD_PREINIT_PATH%=__VSCMD_PREINIT_PATH
C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\bin\HostX86\x86;C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\VC\VCPackages;C:\Program Files (x86)\Microsoft SDKs\TypeScript\3.1;C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\TestWindow;C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\TeamFoundation\Team Explorer;C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\bin\Roslyn;C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Team Tools\Performance Tools;C:\Program Files (x86)\Microsoft Visual Studio\Shared\Common\VSPerfCollectionTools\;C:\Program Files (x86)\Microsoft SDKs\Windows\v10.0A\bin\NETFX 4.6.1 Tools\;C:\Program Files (x86)\HTML Help Workshop;C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\FSharp\;C:\Program Files (x86)\Windows Kits\10\bin\10.0.18362.0\x86;C:\Program Files (x86)\Windows Kits\10\bin\x86;C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\\MSBuild\15.0\bin;C:\Windows\Microsoft.NET\Framework\v4.0.30319;C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\;C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\Tools\;__VSCMD_PREINIT_PATH;C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin;C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\CMake\Ninja

You can see that some of the additions made by vcvarsall.bat are inserted ahead of __VSCMD_PREINIT_PATH, but CMake/Ninja are appended after, and that the MSVC-provided CMake is behind the one I had in PATH to start with

Can you reproduce that much, or do we somehow have visual studio installations that behave differently, even leaving conan completely out of it?

I'm setting up a venv now to make certain I can reproduce this as a 1.21->1.22 regression, with only the conan version varying (it broke on me after installing updates, and I thought I had fingered the right commit, but pip did update a few other packages too...

[puetzk]

Hmm, you're right. A venv with conan==1.21 behaves the same way (the cmake helper gets everything from vcvars prepended, so it fails too.

I still find the fact that with tools.vcvars() does not produce the same environment as calling vcvarsall.bat undesirable, but it's apparently not a new regression as I thought. Sorry for the inaccurate bug report.

Digging through blame, I think it's probably been like this since eb6226d (doing uniqueness in this way changed vcvars_dict to return path as a list of new values, which environment_append will prepend all of, instead of a string that will simply set, and thus lost the ordering of new vs old values). But that landed in conan 1.8.0, so it's been like this for a long time.

Puzzling... Now I don't know how my build was working before I upgraded to 1.22. Obviously there's another moving part here that changed...

[memsharded]

I still find the fact that with tools.vcvars() does not produce the same environment as calling vcvarsall.bat undesirable,

Up to my knowledge, if only_diff=False is provided, you should get the same, right?

but it's apparently not a new regression as I thought. Sorry for the inaccurate bug report.

Not a problem! :)

Let me know please when you find any new hint. Thanks!

[puetzk]

Yes, I think only_diff=False would get an accurate result but there's no way to ask conans.CMake to do that (and even if there was a way, ~none of the public recipes would be using it).

[memsharded]

As a summary, the priories in vcvars helpers and build helpers is a bit entangled. There could be better ways to prioritize different sources. It also seems that better defaults could be used, like only_diff=False, but that would be breaking and can't be done now.

[memsharded]

One important thing to take into account @puetzk is that the profile should always have priority. If users put in their profile some (for example) PATH environment variable, they certainly want that to be honored, and be used, no matter what the recipes or build helpers do.

Then, the tools.vcvars() shouldn't probably do exactly the same as vcvarsall.bat, because otherwise that principle would be broken, wouldn't it? If the helpers and the code in the recipes have priority over the user data, then the user doesn't have the control anymore. So even with only_diff=False, the Conan helpers still will not do the same as directly calling vcvarsall.bat

[puetzk]

Now that #8630 has introduced _EnvVarPlaceHolder and an EnvVars object with both .prepend(...) and .append(...) handling, it seems like it should be possible to truly fix this by having the only_diff mode distinguish which of the new values values vcvarsall prepended and which were appended, so that when with tools.vcvars() re-composes thigns the result preserve the order in which vcvarsall.bat listed items (in particular, which it prepeneded as preferred places to find a tool and which it appended as fallbacks if e.g. there was no cmake.exe or ninja.exe in the system path).

[thorntonryan]

FWIW, If I'm following, I think this vcvars discrepancy with the CMake builders may be no longer be an issue in Conan 2.0 assuming you use the new CMakeToolChain generator.

In Conan 1.x, the CMake Builder's configure step adds the vcvars dict to the environment prior to calling configure:

conan/conans/client/build/cmake.py

Lines 238 to 240 in 345be91

	vcvars_dict = tools.vcvars_dict(self._settings, force=True, filter_known_paths=False,
	output=self._conanfile.output)
	context = _environment_add(vcvars_dict, post=self._append_vcvars)

Whose implementation is problematic for the reasons outlined in this issue.

But, if I follow, in Conan 2.0, instead of appending to the environment prior to calling CMake configure, I think the new conan.tools.cmake builder just calls into the conans.model.conan_file.run:

conan/conan/tools/cmake/cmake.py

Line 105 in 0242b26

self._conanfile.run(command)

Which in turn "wraps" the command to the conanbuild scope:

conan/conans/model/conan_file.py

Lines 413 to 419 in 0242b26

	def run(self, command, stdout=None, cwd=None, ignore_errors=False, env="conanbuild", quiet=False,
	shell=True):
	# NOTE: "self.win_bash" is the new parameter "win_bash" for Conan 2.0
	command = self._conan_helpers.cmd_wrapper.wrap(command)
	env = [env] if env and isinstance(env, str) else []
	envfiles_folder = self.generators_folder or os.getcwd()
	wrapped_cmd = command_env_wrapper(self, command, env, envfiles_folder=envfiles_folder)

Which I think just means we prepend conanbuild.bat && to the command.

And the new CMakeToolChain generator produces a conanbuild.bat:

conan/conan/tools/microsoft/visual.py

Lines 90 to 91 in 0242b26

	:param scope: ``str`` Launcher to be used to run all the variables. For instance,
	if ``build``, then it'll be used the ``conanbuild`` launcher.

which in turn calls conanvcvars.bat, which in turn calls vcvarsall.bat ...

So I think that means the new CMake builder from conan.tools.cmake just won't hit this problem, because it'll actually just call vcvarsall.bat with the correct settings prior to any CMake command, instead of trying to rebuild the environment vcvarsall.bat would have produced.

[memsharded]

Hi all,

Yes, that is true. The new design of environment in https://docs.conan.io/en/latest/reference/conanfile/tools/env.html is based on:

• It prioritizes user control, so Conanfile methods do not activate any environment when they are called.
• All environment activation is done via files, so it is always explicit, and user controllable
• Environment activation is very local, and happens only at the very self.run() invocation
• Different self.run() invocations can use different environments if necessary (like build-env and run-env), or None
• Definition of environment file scripts has been made more explicit in generate() method with VirtualBuildEnv, VirtualRunEnv and VCVars generators.

All of this environment management has been in Conan 1.X for some time already, and it will remain in Conan 2.0, while all the previous one has already been removed.

As you can see, a lot of this design was there to make sure that Conan pre-defined prioritization of environment doesn't become a blocker or an issue in those cases.

@puetzk Have you had a look to these new environment tools?

[memsharded]

This relates to legacy integrations, closing as outdated, please create new tickets related to the new ones, thanks