-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[vcpkg] Fix MSBuild regressions #12062 and #12086. #12257
Conversation
This PR also renames the VcpkgUserTriplet MSBuild variable to VcpkgTriplet to minimize user confusion compared to previous practice and documentation.
d208ced
to
b8a8042
Compare
<!-- Set other defaults--> | ||
<PropertyGroup Condition="'$(VcpkgHasProps)'!='true'" > | ||
<VcpkgUserTriplet Condition="'$(VcpkgUserTriplet)' == ''">$(PlatformTarget)-$(VcpkgOSTarget)</VcpkgUserTriplet> | ||
<VcpkgUserTriplet Condition="'$(VcpkgUserTriplet)' == ''">$(VcpkgPlatformTarget)-$(VcpkgOSTarget)</VcpkgUserTriplet> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think renaming VcpkgUserTriplet to something like VcpkgDefaultTriplet would be useful here, since that is the purpose it is fulfilling in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternately, given Robert's rationale for ignoring other settings when the triplet is explicitly specified perhaps VcpkgUserTriplet should just go away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is fine, but the UI would need to indicate that is what is happening. Right now it doesn't. And do we want to ignore all settings or just the static one?
The UI would need to have a boolean toggle that when true allows editing the triplet and disables all fields that are ignored when the triplet is explicitly specified, and when false disables the freeform triplet field and enables the fields that only apply when you don't explicitly specify the triplet
<ProjectGuid>{5AFB7AF5-D8FC-4A86-B0D2-3BBD039ED03A}</ProjectGuid> | ||
<RootNamespace>VcpkgUseStatic</RootNamespace> | ||
<WindowsTargetPlatformVersion>10.0</WindowsTargetPlatformVersion> | ||
<VcpkgTriplet>x86-windows-static</VcpkgTriplet> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test file should cover exactly how I am currently using vcpkg, so thank you for adding it.
So I think the behavior has improved in some cases but some more nuanced ones are still problematic. The simple case of having a VcpkgTriplet specified by manually editing the vcxproj file (the only way we used to have available) now seems to work fine and be respected, tested in both vs2015 and vs2019. I've not had a chance to solidly reproduce the problematic or confusing behaviors and may not get to until Monday, but here is the gist of them:
Like I said, sorry I don't have solid steps to repro some of the weirder behaviors. If you know what may have changed to make vs2015 start working without this PR, please let me know. I'll let you know when I do have solid steps to reproduce. |
Also, if you'd prefer to just push this PR in and then have me followup with a new issue for the more nuanced stuff I'm seeing, of course feel free to do that. |
was probably this one: f24543e |
Thanks for beating me to fixing this. |
@@ -46,9 +54,6 @@ | |||
<PropertyGroup Condition="'$(VcpkgUseStatic)' == 'true'"> | |||
<VcpkgTriplet>$(VcpkgUserTriplet)-static</VcpkgTriplet> | |||
</PropertyGroup> | |||
<PropertyGroup Condition="'$(VcpkgUseStatic)' != 'true'"> | |||
<VcpkgTriplet>$(VcpkgUserTriplet)</VcpkgTriplet> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the right fix is just to add Condition="'$(VcpkgTriplet)' == ''"
to both this one and the previous one rather than deleting this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order for VcpkgTriplet
to render properly in the props pages, I believe it needs to be preset to a non-empty value in the props pages. Therefore, == ''
won't work here.
@@ -41,7 +41,7 @@ | |||
Category="General" | |||
Default="true"> | |||
</BoolProperty> | |||
<StringProperty Name="VcpkgUserTriplet" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change breaks the 'use static' option. I think the design is that VcpkgUserTriplet specifies the root, and then the static option conditionally appends -static ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change causes 'use static' to not be appended if the user has overriden the triplet. This seemed like an unimportant interaction, because if the user has overriden the triplet they can simply specify it as "-static".
'use static' appears to primarily service the use case of wanting everything at defaults (i.e. our autodetected triplets), but static linked. Once you've overriden the triplet, I don't see the use case anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only appending -static when the triplet isn't overridden makes for an extremely confusing and misleading UI. I really think the use static option should disappear. There are triplets which are static which do not end in static, e.g., x64-windows-static-md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would highly recommend a drop down menu with the available default triplets but i don't have a clue how to do that that is why i introduced the static option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A drop-down would be spiffy but I similarly have no idea. The list of choices is dynamic and can change when the vcpkg triplet folder changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BillyONeal Are you saying that for those that want to override the default triplet, you think 99% just want to append -static
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aggieNick02 Yes, because that's the only part of the triplet we don't currently guess correctly 99% of the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Curious if telemetry tells you that? I currently use static-md for most of my dependencies... I'm pretty sure that's not too uncommon, as a lot of projects (not vcpkg ports) that distribute binaries exclusively do .lib files with dynamic runtime, not static runtime.
Examples include Nvidia's CUDA and video encoding libraries and protobuf.
Is wanting a static lib with static c++ runtime really more common than wanting a static lib with dynamic runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is wanting a static lib with static c++ runtime really more common than wanting a static lib with dynamic runtime?
That is a fairly unusual configuration: the normal reason to choose static linking is to avoid needing to deploy lots of DLLs, but under /MD you still have to deploy a bunch of DLLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a fairly unusual configuration: the normal reason to choose static linking is to avoid needing to deploy lots of DLLs, but under /MD you still have to deploy a bunch of DLLs.
But /MD just means the visual studio c++ runtime is linked dynamically. You just need to ensure your installer installs the visual studio runtime, and nothing else.
When I create a new static library project in VS 2019, it is /MD by default. Honestly that would make me expect the default static triplets to be /MD, and a specialization be required (static-mt) if I wanted the runtime statically linked.
@BillyONeal classified the use static option not working well. Examples of other things that either don't work after this PR
I think having the option in the UI is the right long-term direction, but every day it is present with these bugs, along with inconsistencies (like use static only applying sometimes) is just going to confuse users. And it needs to work correctly with existing projects that had the triplet specified manually. My preference would be to disable the UI until we are sure it is solid and well-defined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think this is quite right but it's an improvement over what we were doing before.
Thank you everyone for the continued feedback on this; I think there is still further improvement to be made, but I want to fix the regressions we know how to fix before tackling further work :) As always, we're happy to review any PRs to make things better. |
This PR also renames the VcpkgUserTriplet MSBuild variable to VcpkgTriplet to minimize user confusion compared to previous practice and documentation.
Fixes Feature that adds vcpkg item to project settings in visual studio breaks builds that set VcpkgTriplet in .vcxproj, introduces other odd behaviors #12062 and Unable to build any package due to invalid path to vcpkg (missing platform in triplet name) #12086.
@aggieNick02 I'd appreciate any review or confirmation you can offer that this indeed fixes the issues