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

Fix: Visual Studio toolset is passed from settings to the MSBuild helper #4250

Merged
merged 8 commits into from Jan 21, 2019

Conversation

Projects
None yet
5 participants
@SSE4
Copy link
Contributor

commented Jan 8, 2019

closes #3766

Changelog: Fix: Visual Studio toolset is passed from settings to the MSBuild helper
Docs: conan-io/docs#1052

@ghost ghost assigned SSE4 Jan 8, 2019

@ghost ghost added the stage: review label Jan 8, 2019

@SSE4 SSE4 force-pushed the SSE4:msbuild_toolset branch 2 times, most recently from 9c7120d to dfd394e Jan 8, 2019

@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2019

I gonna to update this to use MSBuild instead of build_sln_command in tests

SSE4 added some commits Jan 8, 2019

- extract the msvs_toolset method
Signed-off-by: SSE4 <tomskside@gmail.com>
- ensure toolset from settings is applied to the msbuild
Signed-off-by: SSE4 <tomskside@gmail.com>
@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2019

updated tests to use MSBuild

@SSE4 SSE4 force-pushed the SSE4:msbuild_toolset branch from dfd394e to 31e7a13 Jan 13, 2019

- use MSBuild instead of build_sln_command in tests
Signed-off-by: SSE4 <tomskside@gmail.com>

@SSE4 SSE4 force-pushed the SSE4:msbuild_toolset branch from 31e7a13 to 4f2638a Jan 13, 2019

@lasote lasote requested review from danimtb and jgsogo Jan 17, 2019

@lasote lasote assigned lasote and unassigned SSE4 Jan 17, 2019

@lasote

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

Please, solve conflicts.

@lasote lasote added this to the 1.12 milestone Jan 17, 2019

@danimtb
Copy link
Member

left a comment

LGTM. Just take a look at my suggestion to simplify the code

Show resolved Hide resolved conans/client/tools/win.py Outdated
@jgsogo
Copy link
Member

left a comment

Documentation for MSBuild::build() states that:

toolset (Optional, Defaulted to None): Specify a toolset. Will append a /p:PlatformToolset option.

The default value to None will no longer be true. I don't know if it was intentional or not, but it modifies current behavior. I understood that by default it should use the same toolset that is being computed in tools.msvs_toolset (except for Win XP that defaults will be *_xp)

Show resolved Hide resolved conans/client/generators/visualstudio_multi.py Outdated
Show resolved Hide resolved conans/client/tools/win.py Outdated
Show resolved Hide resolved conans/test/unittests/util/msvs_toolset_test.py Outdated
Show resolved Hide resolved conans/test/unittests/client/build/msbuild_test.py Outdated
Show resolved Hide resolved conans/test/unittests/client/build/msbuild_test.py Outdated
Show resolved Hide resolved conans/test/unittests/client/build/msbuild_test.py
- address review comments & pep8
Signed-off-by: SSE4 <tomskside@gmail.com>

@ghost ghost assigned SSE4 Jan 17, 2019

@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2019

@jgsogo default value is still None.
however, previous behavior used toolset specified in solution itself, which wasn't correct.
new behavior uses toolset according to your settings (either settings.toolset, or default toolset for the compiler version).
with old behavior, for instance, we seen projects compiled with Visual Studio 2010 toolset, while settings were set for Visual Studio 2017. it was observed on appveyor, and some users weren't able to compile projects locally, because they never had Visual Studio 2010 installed, so toolset wasn't found on their machine.

- missing imports
Signed-off-by: SSE4 <tomskside@gmail.com>
@lasote
Copy link
Contributor

left a comment

There was a bug, that's correct.
But the relevant discussion about "breaking behavior" is: if someone is not specifying the toolset in the profile, before this PR, no /p:PlatformToolset was applied and that was correct. Now, the "default" /p:PlatformToolset is applied. I think this could be breaking if the user was specifying the toolset in a different place and now it is affected by the /p:PlatformToolset directive.
I think it would make more sense to not apply /p:PlatformToolset when no toolset setting is specified.

@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

@lasote what kind of different place where user specified toolset before?

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

@jgsogo default value is still None

Not really. Before, /p:PlatformToolset was not being added to the command line for most cases, as it was None, unless explicitly defined by user.

This behavior changes, and now, /p:PlatformToolset is going to be set, almost always, as it will get the default toolset of the compiler.version automatically.

This might be ok, we just need to make sure it is not breaking anyone, and if things like other toolsets (_xp, for example) won't be affected.

Show resolved Hide resolved conans/client/tools/win.py Outdated
@lasote

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

@lasote what kind of different place where user specified toolset before?

I don't know, it really doesn't matter, but I would bet there is a way to default a different toolset. Anyway in this case, if the user is not specifying a toolset, I think the right thing to do is not adjusting it. What is the benefit of doing it? You are not following any user input, just guessing a default.

@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

so, if you're not specifying the toolset, and build two projects with some "defaults", you may get one of them built via v110 toolset, and another one with v140_xp toolset, for instance. is it really something we would expect specifying None toolset?

@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

as an example: bincrafters/community#535
currently, compiling with compiler.version=15 and compiler.toolset=None may result in usage of v100 toolset, which is not even available on machine.

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

currently, compiling with compiler.version=15 and compiler.toolset=None may result in usage of v100 toolset, which is not even available on machine.

That is really weird. Then the issue is that the SDL2 project contains some toolset for defining v100 hardcoded in its .vcproj files? How is it possible that another toolset is used by default?

You might be totally right, and it totally makes sense to define the toolset always if possible.

@lasote

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2019

I need to understand the causes behind that behavior to take a decision.

- remove Visual Studio 2019 (16), it goes to its own PR
Signed-off-by: SSE4 <tomskside@gmail.com>
@lasote

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

I have been thinking about this and yes, you are right. With the current model, it makes sense to adjust the toolset to the default of the compiler, because in our model the IDE version is defining the toolset to be used (unless other is specified). Please, @SSE4 take a look at the toolsets_versions dict declared in the model/info.py, I would like to see all the toolset-matching information together.

@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

okay, I'll take a look (that one uses "reversed dict" - keys and values are swapped compare to that we have conans/client/tools/win.py, so I am open to suggestion how to do it properly in python having a bi-directional dictionary?)

@lasote

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

ok, not very important for me but ok. It is more important for me to have them near.

@SSE4

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2019

ah, okay, I'll just put two dictionaries near then :)

- extract MSVS_DEFAULT_TOOLSETS_INVERSE from conans/model/info.py
Signed-off-by: SSE4 <tomskside@gmail.com>
@lasote

lasote approved these changes Jan 21, 2019

@lasote lasote merged commit d82a243 into conan-io:develop Jan 21, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Jan 21, 2019

@lasote lasote referenced this pull request Jan 30, 2019

Merged

Document toolset MSBuild #1052

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.