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

[question] What is the equivalent of conans.tools.MSBuild.build() upgrade_project in conan.tools.microsoft.MSBuid? #12155

Open
1 task done
SpaceIm opened this issue Sep 20, 2022 · 12 comments

Comments

@SpaceIm
Copy link
Contributor

SpaceIm commented Sep 20, 2022

To build xz_utils/5.2.5 with Visual Studio 2022 in conan-io/conan-center-index#13038, vcxproj file must be updated with a different Platform Toolset, how to achieve that in conan v2?

(otherwise it fails with this kind of error since we try to build against a vs2017 solution:

C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Microsoft\VC\v170\Microsoft.CppBuild.targets(460,5): error MSB8020: The build tools for Visual Studio 2017 (Platform Toolset = 'v141') cannot be found.
To build using the v141 build tools, please install Visual Studio 2017 build tools.
Alternatively, you may upgrade to the current Visual Studio tools by selecting the Project menu or right-click the solution, and then selecting "Retarget solution". [C:\Users\spaceim\.conan\data\xz_utils\5.2.5\_\_\build\5a61a86bb3e07ce4262c80e1510f9c05e9b6d48b\src\windows\vs2017\liblzma.vcxproj]

)

@SpaceIm SpaceIm changed the title [question] What is the equivalent of conans.tools.MSBuild upgrade_project in conan.tools.microsoft.MSBuid? [question] What is the equivalent of conans.tools.MSBuild.build() upgrade_project in conan.tools.microsoft.MSBuid? Sep 20, 2022
@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 20, 2022

I had to write something like this in xz_utils:

    def _fix_msvc_platform_toolset(self, vcxproj_file, old_toolset):
        platform_toolset = {
            "Visual Studio": {
                "8": "v80",
                "9": "v90",
                "10": "v100",
                "11": "v110",
                "12": "v120",
                "14": "v140",
                "15": "v141",
                "16": "v142",
                "17": "v143",
            },
            "msvc": {
                "170": "v110",
                "180": "v120",
                "190": "v140",
                "191": "v141",
                "192": "v142",
                "193": "v143",
            }
        }.get(str(self.settings.compiler)).get(str(self.settings.compiler.version))
        replace_in_file(
            self,
            vcxproj_file,
            f"<PlatformToolset>{old_toolset}</PlatformToolset>",
            f"<PlatformToolset>{platform_toolset}</PlatformToolset>",
        )

then:

        if (self.settings.compiler == "Visual Studio" and Version(self.settings.compiler) >= "15") or \
           (self.settings.compiler == "msvc" and Version(self.settings.compiler) >= "191"):
            msvc_version = "vs2017"
            old_toolset = "v141"
        else:
            msvc_version = "vs2013"
            old_toolset = "v120"
        build_script_folder = os.path.join(self.source_folder, "windows", msvc_version)
        self._fix_msvc_platform_toolset(os.path.join(build_script_folder, "liblzma.vcxproj"), old_toolset)
        self._fix_msvc_platform_toolset(os.path.join(build_script_folder, "liblzma_dll.vcxproj"), old_toolset)

But it doesn't scale (need a recipe update for each new Visual Studio release), and conan already has all this information. A helper function could provide platform toolset associated with a given Visual Studio version.

@memsharded
Copy link
Member

The platform toolset is already defined in the provided .props file by MSBuildToolchain, isn't it working? We have some tests that should cover this.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 20, 2022

It's overridden in vcxproj files (as well as runtime & debug/release configurations).

https://github.com/xz-mirror/xz/blob/v5.2.5/windows/vs2017/xz_win.sln
https://github.com/xz-mirror/xz/blob/v5.2.5/windows/vs2017/liblzma.vcxproj

It's not clear to me what are good sln / vcxproj files contents MSBuildToolchain & MSBuild expect, so that profile is honored.

@memsharded
Copy link
Member

This is what I mean, there are tests that have the PlatformToolset defined, like

<PlatformToolset>v141</PlatformToolset>
in the project file, and apparently that value is correctly overriden by the conan_toolchain.props file, not the opposite.
Is the toolchain being injected in the vcxproj? Something like:

<ImportGroup Label="PropertySheets">
    ...
    <Import Project="..\generators\conantoolchain.props" />
  </ImportGroup>

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 20, 2022

Is the toolchain being injected in the vcxproj? Something like:

<ImportGroup Label="PropertySheets">
    ...
    <Import Project="..\generators\conantoolchain.props" />
  </ImportGroup>

Good question, I don't think so. How this magic is supposed to happen? Do we have to programmatically edit vcxproj files in recipes with this content?

I was naively thinking that having MSBuildToolchain in generate(), and MSBuild in build() (with proper sln file given as argument) was sufficient like all other Toolchain / build helper combo like CMake, Meson or Autotools.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 20, 2022

Could it come from the fact that I've not used vs_layout() (because it does not provide a src_folder argument, it's a showstopper for me since I want to put source code under a subfolder to avoid mixing-up patches exported in the recipe with source files) but basic_layout()?
=> Nop, same issues with vs_layout().

Do I have to move generators folder under the same folder than sln file? EDIT: no, doesn't work either.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 20, 2022

My understanding of https://learn.microsoft.com/fr-fr/cpp/build/project-property-inheritance?view=msvc-170 is that properties in vcxproj files have precedence over .props files.

@memsharded
Copy link
Member

Yes, it is necessary to inject the .props files, for third-party vcxproj, I guess it is necessary to apply a patch

Nop, same issues with vs_layout().

Yes, layout is not enough

My understanding of https://learn.microsoft.com/fr-fr/cpp/build/project-property-inheritance?view=msvc-170 is that properties in vcxproj files have precedence over .props files.

This is what the test covers, and seems if you do the inclusion after the properties declaration in vcxproj, it works

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 20, 2022

Ok, so unlike all other toolchain helpers in conan v2, MSBuildToolchain is intrusive, correct? It won't be easy to edit CCI recipes relying on legacy MSBuild helper.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Sep 20, 2022

But the problem is this vcxproj has another logic to designate Configuration name. I guess it can be edited, but it's ugly. We have to:

  • inject a subpart of conantoolchain.props in these vcxproj
  • ensure that conantoolchain_<build_type>_<arch>.props is in the same directory than vcxproj I guess
  • then ensure to satisfy a little bit the logic of vcxproj regarding configuration & platform names (slighty different than the one of props generated by conan, they add MT to configuration name if MT runtime) because include dirs & definitions are conditionnal.
  • ensure to not have conflict beween RuntimeLibrary of vcxproj & the one from props added manually

It won't scale. I guess in this recipe it would be easier to not use MSBuildToolchain & set msbuild.build_type to {build_type}{runtime if MT} (sadly we would lose any external injection of custom cflags, cxxflags & ldflags from profile). And patch a little bit to allow Debug MTd. I don't know why it was working with legacy MSBuild helper, even in test_package of old PR I don't see runtime discrepancy, and it was a transparent integration.

Anyway programmatically edit a vcxproj file is very cumbersome, nobody will want to maintain such recipes.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Dec 29, 2022

@memsharded I'm wondering if /p:ForceImportBeforeCppTargets=<props paths> (where props paths are absolute paths to props files generated by MSBuildToolchain & MSBuildDeps if they are part of generators) is not the missing flag in new MSBuild helper.
It was used in legacy MSBuild helper, but for some reason it's missing in the new one, so how is it supposed to work programmatically in a conanfile.py?

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Dec 31, 2022

Yes, it is necessary to inject the .props files, for third-party vcxproj, I guess it is necessary to apply a patch

Nop, same issues with vs_layout().

Yes, layout is not enough

My understanding of https://learn.microsoft.com/fr-fr/cpp/build/project-property-inheritance?view=msvc-170 is that properties in vcxproj files have precedence over .props files.

This is what the test covers, and seems if you do the inclusion after the properties declaration in vcxproj, it works

I don't know whether current tests properly cover everything, but #12817 is required to avoid to patch MSBuild files in real projects (like libsodium & xz_utils). Beyond that manual patch is tedious, location of manual injection of these props is very important, so not something you want to delegate to users.

Also something I've learned, is that recipes have to be very careful with configuration attribute of MSBuildToolchain & MSBuildDeps, and build_type attribute of MSBuild. As soon as upstream doesn't use "standard" configuration names, it breaks if these attributes are not carefully set.

Quite tricky, in libsodium we have to specify different names between configuration of MSBuildToolchain & MSBuildDeps and build_type of MSBuild, due to this block in their sln file:

Global
	GlobalSection(SolutionConfigurationPlatforms) = preSolution
		DynDebug|Win32 = DynDebug|Win32
		DynDebug|x64 = DynDebug|x64
		DynRelease|Win32 = DynRelease|Win32
		DynRelease|x64 = DynRelease|x64
		LtcgDebug|Win32 = LtcgDebug|Win32
		LtcgDebug|x64 = LtcgDebug|x64
		LtcgRelease|Win32 = LtcgRelease|Win32
		LtcgRelease|x64 = LtcgRelease|x64
		StaticDebug|Win32 = StaticDebug|Win32
		StaticDebug|x64 = StaticDebug|x64
		StaticRelease|Win32 = StaticRelease|Win32
		StaticRelease|x64 = StaticRelease|x64
	EndGlobalSection
	GlobalSection(ProjectConfigurationPlatforms) = postSolution
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.DynDebug|Win32.ActiveCfg = DebugDLL|Win32
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.DynDebug|Win32.Build.0 = DebugDLL|Win32
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.DynDebug|x64.ActiveCfg = DebugDLL|x64
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.DynDebug|x64.Build.0 = DebugDLL|x64
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.DynRelease|Win32.ActiveCfg = ReleaseDLL|Win32
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.DynRelease|Win32.Build.0 = ReleaseDLL|Win32
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.DynRelease|x64.ActiveCfg = ReleaseDLL|x64
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.DynRelease|x64.Build.0 = ReleaseDLL|x64
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.LtcgDebug|Win32.ActiveCfg = DebugLTCG|Win32
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.LtcgDebug|Win32.Build.0 = DebugLTCG|Win32
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.LtcgDebug|x64.ActiveCfg = DebugLTCG|x64
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.LtcgDebug|x64.Build.0 = DebugLTCG|x64
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.LtcgRelease|Win32.ActiveCfg = ReleaseLTCG|Win32
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.LtcgRelease|Win32.Build.0 = ReleaseLTCG|Win32
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.LtcgRelease|x64.ActiveCfg = ReleaseLTCG|x64
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.LtcgRelease|x64.Build.0 = ReleaseLTCG|x64
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.StaticDebug|Win32.ActiveCfg = DebugLIB|Win32
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.StaticDebug|Win32.Build.0 = DebugLIB|Win32
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.StaticDebug|x64.ActiveCfg = DebugLIB|x64
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.StaticDebug|x64.Build.0 = DebugLIB|x64
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.StaticRelease|Win32.ActiveCfg = ReleaseLIB|Win32
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.StaticRelease|Win32.Build.0 = ReleaseLIB|Win32
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.StaticRelease|x64.ActiveCfg = ReleaseLIB|x64
		{A185B162-6CB6-4502-B03F-B56F7699A8D9}.StaticRelease|x64.Build.0 = ReleaseLIB|x64
	EndGlobalSection
	GlobalSection(SolutionProperties) = preSolution
		HideSolutionNode = FALSE
	EndGlobalSection
EndGlobal

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

No branches or pull requests

2 participants