Skip to content

Fix: Correct semicolon handling in MSVC environment variable stacking #18979

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: develop2
Choose a base branch
from
Open

Fix: Correct semicolon handling in MSVC environment variable stacking #18979

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
82c47f9
Fix: Correct semicolon handling in MSVC environment variable stacking
alexandrelaverdure-eaton Sep 23, 2025
02cf752
Update conan/tools/microsoft/msbuilddeps.py
alexandrelaverdure-eaton Sep 24, 2025
7decbee
Merge branch 'conan-io:develop2' into fix/env-var-stacking
alexandrelaverdure-eaton Sep 24, 2025
a8d3063
Merge branch 'conan-io:develop2' into fix/env-var-stacking
alexandrelaverdure-eaton Sep 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 16 additions & 24 deletions conan/tools/microsoft/msbuilddeps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import fnmatch
import fnmatch
import os
import re
import textwrap
Expand All @@ -11,7 +11,7 @@
from conan.internal.api.install.generators import relativize_path
from conan.internal.model.dependencies import get_transitive_requires
from conan.tools.microsoft.visual import msvc_platform_from_arch
from conan.internal.util.files import load, save
from conans.util.files import load, save
Copy link
Member

Choose a reason for hiding this comment

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

This is broken, the conans space no longer contain this.


VALID_LIB_EXTENSIONS = (".so", ".lib", ".a", ".dylib", ".bc")

Expand Down Expand Up @@ -64,22 +64,22 @@ class MSBuildDeps:
</PropertyGroup>
<ItemDefinitionGroup>
<ClCompile>
<AdditionalIncludeDirectories>$(Conan{{name}}IncludeDirectories)%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<PreprocessorDefinitions>$(Conan{{name}}PreprocessorDefinitions)%(PreprocessorDefinitions)</PreprocessorDefinitions>
<AdditionalIncludeDirectories>$(Conan{{name}}IncludeDirectories);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
Copy link
Member

Choose a reason for hiding this comment

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

But what if the $(Conan{{name}}IncludeDirectories) variable is empty? It will lead to a value for <AdditionalIncludeDirectories> that will start with ;, which is also a bit ugly?

I think the purpose of not having the ; here was to avoid this case

Copy link
Author

Choose a reason for hiding this comment

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

I agree i would not want that to happen, but would you said that you agree with the behavior i'm trying to change | fix ?

Copy link
Member

Choose a reason for hiding this comment

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

The truth is that I didn't fully understand the issue. I thought this was tested enough, so I am a bit surprised to learn about this failing in this way. Because this means that MSBuildDeps generator is broken when using more than 1 dependency? It is not very clear.
The output of the IDE windows is a bit irrelevant, it would be better to have the contents of the .props files and check them.
This is the reason I'd like to see a failing/red test first, to fully understand the scope of the issue. The test can easily act on the .props files.

Copy link
Author

@alexandrelaverdure-eaton alexandrelaverdure-eaton Sep 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I understand, and nothing is broken per se, but as you can see in the image, I was debugging and thought I might be missing an import. This was because I didn’t see the property in the "Additional Properties" tab—the one I blurred in red.
Eventually, I found it at the end of the list in the conan.props file. #18978

One of our senior dev, @DavidBertrand@Eaton.com, explained to me how tokenization and commands are stacked together during the compile/link process using .props files.

<PreprocessorDefinitions>$(Conan{{name}}PreprocessorDefinitions);%(PreprocessorDefinitions)</PreprocessorDefinitions>
<AdditionalOptions>$(Conan{{name}}CompilerFlags) %(AdditionalOptions)</AdditionalOptions>
</ClCompile>
<Link>
<AdditionalLibraryDirectories>$(Conan{{name}}LibraryDirectories)%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories>
<AdditionalDependencies>$(Conan{{name}}Libraries)%(AdditionalDependencies)</AdditionalDependencies>
<AdditionalDependencies>$(Conan{{name}}SystemLibs)%(AdditionalDependencies)</AdditionalDependencies>
<AdditionalLibraryDirectories>$(Conan{{name}}LibraryDirectories);%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories>
<AdditionalDependencies>$(Conan{{name}}Libraries);%(AdditionalDependencies)</AdditionalDependencies>
Copy link
Author

Choose a reason for hiding this comment

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

@memsharded The main issue would be here since the semicolons are part of the $(Conan{{name}}PreprocessorDefinitions) variable.
so instead of having in the "Additional Properties" tab
ConanImport1;ConanImport2;
TheFollowingImport;
you get
ConanImport1;ConanImport2;TheFolowingImport;

<AdditionalDependencies>$(Conan{{name}}SystemLibs);%(AdditionalDependencies)</AdditionalDependencies>
<AdditionalOptions>$(Conan{{name}}LinkerFlags) %(AdditionalOptions)</AdditionalOptions>
</Link>
<Midl>
<AdditionalIncludeDirectories>$(Conan{{name}}IncludeDirectories)%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<AdditionalIncludeDirectories>$(Conan{{name}}IncludeDirectories);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
</Midl>
<ResourceCompile>
<AdditionalIncludeDirectories>$(Conan{{name}}IncludeDirectories)%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<PreprocessorDefinitions>$(Conan{{name}}PreprocessorDefinitions)%(PreprocessorDefinitions)</PreprocessorDefinitions>
<AdditionalIncludeDirectories>$(Conan{{name}}IncludeDirectories);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
<PreprocessorDefinitions>$(Conan{{name}}PreprocessorDefinitions);%(PreprocessorDefinitions)</PreprocessorDefinitions>
</ResourceCompile>
</ItemDefinitionGroup>
{% else %}
Expand Down Expand Up @@ -157,17 +157,9 @@ def _vars_props_file(self, require, dep, name, cpp_info, build):
:return: varfile content
"""

def add_valid_ext(libname, libdirs=None):
def add_valid_ext(libname):
ext = os.path.splitext(libname)[1]
if ext in VALID_LIB_EXTENSIONS:
return f"{libname};"

lib_name = f"{libname}.lib"
if libdirs and not any(lib_name in os.listdir(d) for d in libdirs if os.path.isdir(d)):
meson_name = f"lib{libname}.a"
if any(meson_name in os.listdir(d) for d in libdirs if os.path.isdir(d)):
lib_name = meson_name
return f"{lib_name};"
Comment on lines -165 to -170
Copy link
Member

Choose a reason for hiding this comment

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

Why removing this code? This code was here to satisfy some issue with Meson build system generated dependencies. Better not to modify it.

Copy link
Author

Choose a reason for hiding this comment

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

i supposed it was introduced later, we still run on conan version 2.16.1.
I will update to the latest version for future commit.

return '%s;' % libname if ext in VALID_LIB_EXTENSIONS else '%s.lib;' % libname

pkg_placeholder = "$(Conan{}RootFolder)".format(name)

Expand All @@ -187,7 +179,7 @@ def join_paths(paths):
rel = full_path[len(root_folder)+1:]
full_path = ("%s/%s" % (pkg_placeholder, rel))
ret.append(full_path)
return "".join("{};".format(e) for e in ret)
return ";".join(ret)

root_folder = dep.recipe_folder if dep.package_folder is None else dep.package_folder
root_folder = escape_path(root_folder)
Expand All @@ -199,10 +191,10 @@ def join_paths(paths):
res_dirs = join_paths(cpp_info.resdirs)
include_dirs = join_paths(cpp_info.includedirs)
lib_dirs = join_paths(cpp_info.libdirs)
libs = "".join([add_valid_ext(lib, cpp_info.libdirs) for lib in cpp_info.libs])
libs = "".join([add_valid_ext(lib) for lib in cpp_info.libs]).removesuffix(";")
# TODO: Missing objects
system_libs = "".join([add_valid_ext(sys_dep) for sys_dep in cpp_info.system_libs])
definitions = "".join("%s;" % d for d in cpp_info.defines)
system_libs = "".join([add_valid_ext(sys_dep) for sys_dep in cpp_info.system_libs]).removesuffix(";")
definitions = "".join("%s;" % d for d in cpp_info.defines).removesuffix(";")
compiler_flags = " ".join(cpp_info.cxxflags + cpp_info.cflags)
linker_flags = " ".join(cpp_info.sharedlinkflags + cpp_info.exelinkflags)

Expand Down
Loading