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

Propagate arch parameter to vcvars in msbuild helper #6928

Merged
merged 6 commits into from Apr 30, 2020

Conversation

danimtb
Copy link
Member

@danimtb danimtb commented Apr 29, 2020

Changelog: BugFix: Propagate arch parameter to tools.vcvars_command() in MSBuild() build helper.
Docs: omit

#TAGS: slow

  • Refer to the issue that supports this Pull Request: closes #6746
  • 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.

@danimtb danimtb added this to the 1.25 milestone Apr 29, 2020
@danimtb danimtb self-assigned this Apr 29, 2020
@@ -83,7 +83,7 @@ def build(self, project_file, targets=None, upgrade_project=True, build_type=Non
props_file_contents = self._get_props_file_contents(definitions)
property_file_name = os.path.abspath(property_file_name)
save(property_file_name, props_file_contents)
vcvars = vcvars_command(self._conanfile.settings, force=force_vcvars,
vcvars = vcvars_command(self._conanfile.settings, arch=arch, force=force_vcvars,
Copy link
Member

@memsharded memsharded Apr 29, 2020

Choose a reason for hiding this comment

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

Are we sure this is not breaking? So far the arch argument was used to define just the MSBuild command:

p:Platform=Release|x86

But the vcvars context could be using the toolchain of 64 bits, just that it can "cross build" to 32 bits if we want to.

Just a question, to double check.

Copy link
Member Author

@danimtb danimtb Apr 29, 2020

Choose a reason for hiding this comment

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

I wanted to try this just so see if there is any failing test in our test suite. If it is considered breaking, we can add a new attribute vccars_arch to allow the override

Copy link
Contributor

@SSE4 SSE4 Apr 30, 2020

Choose a reason for hiding this comment

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

there are four (at least) versions of vcvars:

06-Apr-20  15:20             2,151 x64 Native Tools Command Prompt for VS 2019.lnk
06-Apr-20  15:20             2,209 x64_x86 Cross Tools Command Prompt for VS 2019.lnk
06-Apr-20  15:20             2,151 x86 Native Tools Command Prompt for VS 2019.lnk
06-Apr-20  15:20             2,209 x86_x64 Cross Tools Command Prompt for VS 2019.lnk

the arch parameter of vcvars_command controls the target architecture. in other words, you may select between x64 native or x64_x86 cross tools.
therefore, specifying arch=x86 you probably want 32-bit binaries as result (implies x64_x86 Cross Tools), and specifying arch=x86_64 you want 64-bit binaries (implies x64 Native Tools).
and I believe it should be aligned with the platform for msbuild:

  • arch=x86 -> p:Platform=Release|x86 (x64_x86 Cross Tools)
  • arch=x86_64 -> p:Platform=Release|x64 (x64 Native Tools)

and similar results running 32-bit Windows (which is unlikely these days):

  • arch=x86 -> p:Platform=Release|x86 (x86 Native Tools)
  • arch=x86_64 -> p:Platform=Release|x64 (x86_x64 Cross Tools)

otherwise, you'll probably get compiler/linker errors.
I believe that should be the behavior, and MSBuild tests should reflect that.

Copy link
Member

@memsharded memsharded Apr 30, 2020

Choose a reason for hiding this comment

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

I see.
As this is changing behavior, it seems this would be more correct, so to merge this we should declare #6746 a bug, and not a feature. Wdyt @SSE4 @danimtb ?

Copy link
Contributor

@SSE4 SSE4 Apr 30, 2020

Choose a reason for hiding this comment

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

no objections.

Copy link
Member Author

@danimtb danimtb Apr 30, 2020

Choose a reason for hiding this comment

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

Updated changelog with "bugfix"

Copy link
Member

@memsharded memsharded Apr 30, 2020

Choose a reason for hiding this comment

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

Can you please @danimtb add some test that capture this behavior? We are changing something that doesn't affect our tests at all.

@danimtb danimtb marked this pull request as ready for review Apr 30, 2020
@danimtb danimtb assigned memsharded and unassigned danimtb Apr 30, 2020
@@ -83,7 +83,7 @@ def build(self, project_file, targets=None, upgrade_project=True, build_type=Non
props_file_contents = self._get_props_file_contents(definitions)
property_file_name = os.path.abspath(property_file_name)
save(property_file_name, props_file_contents)
vcvars = vcvars_command(self._conanfile.settings, force=force_vcvars,
vcvars = vcvars_command(self._conanfile.settings, arch=arch, force=force_vcvars,
Copy link
Member

@memsharded memsharded Apr 30, 2020

Choose a reason for hiding this comment

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

Can you please @danimtb add some test that capture this behavior? We are changing something that doesn't affect our tests at all.

@memsharded memsharded merged commit 3cea43f into conan-io:develop Apr 30, 2020
2 checks passed
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.

3 participants