Target Platform Intrinsic Functions #5429
Conversation
|
@rainersigwald I've updated the NuGet dependency to get the latest version, which has the target platform changes. Is this the right way to go about it? |
sfoslund
force-pushed
the
TargetPlatformFuncs
branch
from
6d983c0
to
36edfc3
Compare
sfoslund
force-pushed
the
TargetPlatformFuncs
branch
from
36edfc3
to
d6adeb4
Compare
|
Looks like the test failures are unrelated to this PR. @rainersigwald what's the timeline with working to 16.7? Edit: It sounds like July 2nd, is that right? Is this good to go in once the branch is ready? |
|
Test failures should have been fixed by #5439. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
All tests passed. Good to go? |
|
@wli3 see #5429 (review), we're waiting until the fork for 16.7 |
|
Looks like we need to rush a bit after the branch cut. cswinrt wants to be done in preview 8. 7/15 is the branch cut date. After this is merged, we need to get on the task of parsing out |
|
Get a build of this local msbuild and then swap the content inside stage 0 for you local SDK might work. We can try together |
|
@wli3, I have the skeleton of the code written and I already did a little bit of testing with this MSBuild change in my stage 0. If you're going to be blocked I can clean it up and test some more tomorrow and put out a draft PR so you can review. |
|
That's great! Let's do that |
|
Once NuGet/NuGet.Client#3479 is merged we'll want to update the NuGet version number to point to the updated implementation, since the one we're using at the moment has known bugs. |
|
We could merge it today? |
|
@wli3 We're going to want to update the NuGet version number once NuGet/NuGet.Client#3479 is merged, so we're still blocked on that. Right now this is implemented on top of the buggy target platform work. |
|
@sfoslund I converted this to a draft so we don't look at it in triage until the NuGet thing is resolved. Can you promote it to "ready for review" when that's done and consumed here? |
|
@rainersigwald I've updated the nuget version so this should be ready to go. |
eng/Packages.props
Outdated
| @@ -18,7 +18,7 @@ | |||
| <PackageReference Update="Microsoft.VisualStudio.Setup.Configuration.Interop" Version="1.16.30" /> | |||
| <PackageReference Update="Microsoft.Win32.Registry" Version="4.3.0" /> | |||
| <PackageReference Update="NuGet.Build.Tasks" Version="$(NuGetBuildTasksVersion)" /> | |||
| <PackageReference Update="NuGet.Frameworks" Version="$(NuGetBuildTasksVersion)" /> | |||
| <PackageReference Update="NuGet.Frameworks" Version="5.7.0-rtm.6710" /> | |||
There was a problem hiding this comment.
What does rtm stand for? And will this always be available?
There was a problem hiding this comment.
I think it means release to manufacturing?... (@wli3 is that right?) I don't see why it wouldn't be avaliable.
There was a problem hiding this comment.
ya, "release to manufacturing". In our case it is mostly mean this version is ready for try. And it should be stable.
although remember to remove line once nuget proper insertion is in.
There was a problem hiding this comment.
What do you mean proper nuget insertion? We're using reflection here so we won't need insertions into MSBuild.
There was a problem hiding this comment.
What's the fallback if it can't find 5.7.0-rtm-6710?
There was a problem hiding this comment.
I don't know that there is a fallback, but this dependency is only for testing. Otherwise we use reflection and try to load the dll from its VS install location or next to the MSBuild dll. This logic is here in the last intrinsic functions PR I made.
@rainersigwald updating the master nuget reference seems to create some version mismatches. Is there a simple way to solve this? |
ghost
added this to the 
Fixes #5792 We had two areas where MSBuild checked TargetFramework based on whether it started with netcore and netstandard. With net5.0 resolving to net5.0 instead of netcoreapp5.0, this logic would no longer succeed, so here's a quick fix via intrinsic function GetTargetFrameworkIdentifier introduced in #5429. Co-authored-by: Forgind <Forgind@users.noreply.github.com>
No description provided.