Fix: Correct semicolon handling in MSVC environment variable stacking #18979
Conversation
|
|
There was a problem hiding this comment.
Thanks very much for your contribution.
I think it would be better to start with a test in the test suite that illustrate the change that we want to do, and why. It should be a red/failing test, because the expected behavior represented in the test is not what is being computed by Conan.
Then, from there, we can go and do the minimal changes to the code to satisfy that fix, but no more.
For example, avoid doing any other unrelated changes, for example, the change to from conans.util.files import load, save will break everything, it is not clear at all why this was changed, can you please elaborate? Please try to run at least the test/unittest and test/integration tests as recommended in the dev guide
| 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 |
There was a problem hiding this comment.
This is broken, the conans space no longer contain this.
| <ClCompile> | ||
| <AdditionalIncludeDirectories>$(Conan{{name}}IncludeDirectories)%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> | ||
| <PreprocessorDefinitions>$(Conan{{name}}PreprocessorDefinitions)%(PreprocessorDefinitions)</PreprocessorDefinitions> | ||
| <AdditionalIncludeDirectories>$(Conan{{name}}IncludeDirectories);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, @[email protected], explained to me how tokenization and commands are stacked together during the compile/link process using .props files.
| 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};" |
There was a problem hiding this comment.
Why removing this code? This code was here to satisfy some issue with Meson build system generated dependencies. Better not to modify it.
There was a problem hiding this comment.
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.
| <AdditionalDependencies>$(Conan{{name}}Libraries)%(AdditionalDependencies)</AdditionalDependencies> | ||
| <AdditionalDependencies>$(Conan{{name}}SystemLibs)%(AdditionalDependencies)</AdditionalDependencies> | ||
| <AdditionalLibraryDirectories>$(Conan{{name}}LibraryDirectories);%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories> | ||
| <AdditionalDependencies>$(Conan{{name}}Libraries);%(AdditionalDependencies)</AdditionalDependencies> |
There was a problem hiding this comment.
@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;
There was a problem hiding this comment.
I am afraid this cannot be merged yet.
As you can see, the tests are still broken, because of the usage of from conans that no longer exist. I don't fully understand how that can be working for you locally, have you tested the functionality running from source? Because it should throw an error about this erroneous import.
Also, I still think that proposing modifications to the code is not the best way to start. The best is to start with a test that captures and clearly shows the behavior, hopefully a "red" failing test that makes evident that the behavior and result is not the expected one.
The test can be an integration test, that just uses the generator, then read the .props files to check them (not necessary to actually build anything, that simplifies the tests).
You can have a look to existing "integration" tests that uses MSBuildDeps like the ones in test\integration\toolchains\microsoft\test_msbuilddeps.py, the testing framework we use is relatively straightforward for that kind of tests.
This was assigned to 2.22 release, but that is being branched tomorrow, and as this is still broken, I am clearing the milestone. If we can move forward this with the tests first.
Many thanks!
4 participants
Changelog: (Bugfix): Describe here your pull request
Docs: https://github.com/conan-io/docs/pull/XXXX
developbranch, documenting this one.Related to issue #18978
First PR to an open source project :)