-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Infer PlatformTarget from RuntimeIdentifier #326
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
Conversation
@@ -0,0 +1,45 @@ | |||
<!-- | |||
*********************************************************************************************** | |||
Microsoft.NET.TargetFrameworkInference.targets |
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.
Wrong name, cut and paste
return selfContained ? | ||
Path.Combine("Debug", targetFramework, RuntimeEnvironment.GetRuntimeIdentifier(), PublishSubfolderName) : | ||
Path.Combine("Debug", targetFramework, PublishSubfolderName); | ||
if (runtimeIdentifier.Length == 0 && selfContained) |
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.
whitespace
In what timeframe would you expect to switch to a RID-specific output path? |
Tomorrow if urgent, is it? |
Late tonight if even more urgent. I have an appointment from 5-6 PM. |
e50f91e
to
1025317
Compare
My appointment was canceled. Still working on tests. |
Found bugs, don't merge. Will reopen when fixed. |
1025317
to
02c4c32
Compare
@srivatsn @eerhardt @dsplaisted @dotnet/project-system Tests added, bugs fixed. Please review. |
Require RID for .NETFramework exes
02c4c32
to
f89608d
Compare
|
||
var buildCommand = new BuildCommand(Stage0MSBuild, testAsset.TestRoot); | ||
buildCommand | ||
.Execute($"/p:RuntimeIdentifier=win7-x86", "/p:PlatformTarget=x64") |
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 there ever a case where it would be correct to do this (ie have RuntimeIdentifier
be for x86 but PlatformTarget
be x64)?
|
||
<When Condition="$(RuntimeIdentifier.EndsWith('-arm')) or $(RuntimeIdentifier.Contains('-arm-'))"> | ||
<PropertyGroup> | ||
<PlatformTarget>arm</PlatformTarget> |
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.
@gkhanna79 did some work to enable arm32. Does that have a different RID/architecture than arm64?
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.
Its RID is win8-arm.
</Otherwise> | ||
</Choose> | ||
|
||
<Target Name="CheckRuntimeIdentifier"> |
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.
Should we put in an "escape hatch" for this Target? Like a property that will shut it off completely if the users really, really knows what they are doing and wants to skip this check.
…hesFor21 Apply source build patches for 2.1
Also, require RID for .NETFramework exes
@srivatsn @eerhardt Working on a test, but given urgency, would be good to get review started in parallel.
Finally, I ran into test issues with redirecting the build output path to have the RID so I reverted that for now. I'm thinking that can wait.