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

Implicit RID support #19725

Closed
wants to merge 2 commits into from
Closed

Conversation

WeihanLi
Copy link
Contributor

@WeihanLi WeihanLi commented Aug 13, 2021

#19576

I'm sorry that I'm not sure how to implement this in the right way, and the test cases are not implemented

@agocke could you please help me on this

@WeihanLi WeihanLi marked this pull request as draft August 13, 2021 17:03
@WeihanLi WeihanLi force-pushed the feature/implicit-rid-support branch 2 times, most recently from 07c82fa to 0d164ae Compare August 14, 2021 03:38
@WeihanLi WeihanLi marked this pull request as ready for review August 14, 2021 04:24
@agocke
Copy link
Member

agocke commented Aug 17, 2021

Happy to help with this, just finishing up a few things I have on my plate. Should be able to get to it later today or tomorrow.

@agocke
Copy link
Member

agocke commented Aug 18, 2021

Hmm, I wonder if this can be a change purely in src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets. The idea would be that we would use the SDK RID if UseCurrentRuntimeIdentifier is true or RuntimeIdentifer is unset and SelfContained is set to true.

@marcpopMSFT
Copy link
Member

Isn't this just the switch --use-current-runtime? I don't think we'd want to make this implicit but would rather prefer customers specify the switch.

@agocke
Copy link
Member

agocke commented Aug 18, 2021

@marcpopMSFT I think that's a question for @richlander, as that contradicts the issue description. I think the reason to set this by default is because that's effectively what we do for FX-dependent apps via the apphost. Why require extra properties for self-contained apps than FX-dependent?

@WeihanLi WeihanLi force-pushed the feature/implicit-rid-support branch from 0d164ae to 46af602 Compare August 19, 2021 00:19
@WeihanLi
Copy link
Contributor Author

WeihanLi commented Sep 30, 2021

Hi @agocke, is there any more changes needed, or should wait for more feedback.
cc: @richlander @marcpopMSFT

I could not run all the tests on my side, for some failed tests in Microsoft.NET.Build.Tasks.UnitTests, it would fail even when I revert my changes, could you please help me with the failed test cases.

I tested the SDK with my test project locally, seemed to work fine, the test output is as follows:

PS C:\projects\source\dotnet\sdk> dotnet publish -c Release --self-contained C:\projects\test\DynamicStaticFileProvider
Microsoft (R) Build Engine version 17.0.0-preview-21411-06+b0bb46ab8 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  Restored C:\projects\test\DynamicStaticFileProvider\DynamicStaticFileProvider.csproj (in 9.19 sec).
  You are using a preview version of .NET. See: https://aka.ms/dotnet-core-preview
  DynamicStaticFileProvider -> C:\projects\test\DynamicStaticFileProvider\bin\Release\net5.0\win-x64\DynamicStaticFileProvider.dll
  DynamicStaticFileProvider -> C:\projects\test\DynamicStaticFileProvider\bin\Release\net5.0\win-x64\publish\

and the project file had not defined the RID, the project file content is as follows:

<Project Sdk="Microsoft.NET.Sdk.Web">
  <PropertyGroup>
    <TargetFramework>net5.0</TargetFramework>
  </PropertyGroup>
</Project>

@WeihanLi WeihanLi closed this Feb 18, 2022
@WeihanLi WeihanLi deleted the feature/implicit-rid-support branch February 18, 2022 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants