Add Netcoreapp ridless config to System.Private.Uri And System.Private.Xml by Anipik · Pull Request #38142 · dotnet/runtime

Anipik · 2020-06-19T01:59:02Z

This is related to effort on removing the cross compilation. A lot of projects are being cross compiled because of having a project dependency on these 2 projects.
We are adding a ridless tfm so that other projects can use this tfm for compiling. This would never be shipped.
I am currently using the windows implementation for the ridless one.

Dotnet-GitSync-Bot · 2020-06-19T01:59:06Z

I couldn't figure out the best area label to add to this PR. Please help me learn by adding exactly one area label.

ViktorHofer

Your PR title is slightly misleading. The configurations were already (hard-coded) version less before.

src/libraries/System.Private.Xml/src/System.Private.Xml.csproj

src/libraries/System.Diagnostics.StackTrace/src/System.Diagnostics.StackTrace.csproj

safern · 2020-06-19T17:54:51Z

src/mono/wasm/wasm.targets

    <WasmPInvokeModules Include="libSystem.Native"/>
    <WasmPInvokeAssemblies Include="$(MonoArtifactsPath)\System.Private.CoreLib.dll"/>
-    <WasmPInvokeAssemblies Include="$(ArtifactsBinDir)\System.Runtime\$(NetCoreAppCurrent)-Unix-$(Configuration)\System.Runtime.dll"/>
+    <WasmPInvokeAssemblies Include="$(ArtifactsBinDir)\System.Runtime\$(NetCoreAppCurrent)-$(Configuration)\System.Runtime.dll"/>


This list seems very error-prone. Maybe we can add P2Ps and get the output assemblies?

cc: @ViktorHofer

I can create an issue to track this.

Anipik · 2020-06-22T15:48:13Z

@ericstj @ViktorHofer @safern can you review this one ?

ViktorHofer · 2020-06-22T15:53:52Z

We are adding a ridless tfm so that other projects can use this tfm for compiling. This would never be shipped.

Instead we could annotate the P2Ps that target System.Private.Uri and System.Private.Xml and specify a non TargetOS agnostic TargetFramework. That sounds like a cleaner approach to me and we would avoid building a configuration for the sake of easier multi-targeting.

cc @ericstj

ericstj · 2020-06-22T17:50:47Z

Instead we could annotate the P2Ps that target System.Private.Uri and System.Private.Xml and specify a non TargetOS agnostic TargetFramework

What do you propose the pattern is?

<ProjectReference Include="...\System.Private.Uri.csproj" TargetFramework="$(NetCoreAppCurrent)-$(TargetOS)" />

<ProjectReference Include="...\System.Private.Uri.csproj" UseTargetOS="true" />

Whatever we do we'd need to be careful to implement at the caller and not through global properties. Here: https://github.com/dotnet/arcade/blob/4e0dc26a70f3f9bb9bc7ae65937f85c88332abdd/src/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk/src/build/Microsoft.DotNet.Build.Tasks.TargetFramework.Sdk.targets#L74-L78

ViktorHofer · 2020-06-22T18:00:01Z

be careful to implement at the caller and not through global properties

Isn't the TargetFramework already a global property? Wouldn't something like: `<ProjectReference Include="x.csproj" AdditionalProperties="TargetFramework=$(NetCoreAppCurrent)-$(TargetOS)" just work and not need any TargetFramework.Sdk changes?

ericstj · 2020-06-22T19:22:14Z

We don't want projects setting TargetFramework of another project. That results in targeting information from one project being leaked into the referencing project. It can then get out of sync and cause major issues.

Playing around with global properties in authoring is a recipe for disaster (race conditions, invalid build configurations, etc). We need a well defined behavior here that is controlled by the config system.

TargetFramework metadata would specify to the configuration system: resolve this Project Reference as if the calling project targeted a different TargetFramework than actual. In this case, net5.0-Windows_NT instead of net5.0. The config system would still be responsible for selecting the TF from the set of supported ones in the referenced project and it would be the only one setting the global property.

The second suggestion just further constrains this. Rather than letting a project use any TargetFramework you could only allow a project that doesn't include a -$(TargetOS) suffix to resolve a project reference as if it did.

src/libraries/System.Private.Uri/src/System.Private.Uri.csproj

ericstj

I like it! Thanks for coming up with this solution!

src/libraries/System.Net.WebSockets.Client/src/System.Net.WebSockets.Client.csproj

ericstj · 2020-06-23T13:47:46Z

src/libraries/System.Private.Uri/src/System.Private.Uri.csproj

    <Compile Include="System\Uri.Windows.cs" />
  </ItemGroup>
-  <ItemGroup Condition="'$(TargetsUnix)' == 'true'">
+  <ItemGroup Condition="'$(TargetsAnyOS)' == 'true'">


Nit: I’m not a huge fan of this property name, but the behavior is correct.

Yeah that property name is definitely misleading...

It has been present since the old config system, So we need to be careful if we decide to change this one name.

...s/System.Runtime.Serialization.Formatters/src/System.Runtime.Serialization.Formatters.csproj

ViktorHofer · 2020-06-23T17:31:09Z

@Anipik can you please add a comment to the projects where you are treating the non RID target framework as Unix and WASM? #38142 (comment)

Anipik · 2020-06-25T23:37:41Z

@Anipik can you please add a comment to the projects where you are treating the non RID target framework as Unix and WASM?

I missed it, i will add it

remove unnecessary cross compilation

1d9ba15

Anipik requested review from ViktorHofer, ericstj and safern June 19, 2020 01:59

ViktorHofer reviewed Jun 19, 2020

View reviewed changes

src/libraries/System.Private.Xml/src/System.Private.Xml.csproj Outdated Show resolved Hide resolved

Anipik changed the title ~~Add Netcoreapp versionless config to System.Private.Uri And System.Private.Xml~~ Add Netcoreapp ridless config to System.Private.Uri And System.Private.Xml Jun 19, 2020

Anipik added 3 commits June 19, 2020 09:29

Merge branch 'master' into privateUri

3358ae3

remove extra <

ad36b65

correct pinvoke path

12d10ba

Anipik requested a review from marek-safar as a code owner June 19, 2020 17:16

safern reviewed Jun 19, 2020

View reviewed changes

src/libraries/System.Diagnostics.StackTrace/src/System.Diagnostics.StackTrace.csproj Show resolved Hide resolved

safern reviewed Jun 19, 2020

View reviewed changes

dotnet deleted a comment Jun 22, 2020

ericstj reviewed Jun 23, 2020

View reviewed changes

src/libraries/System.Private.Uri/src/System.Private.Uri.csproj Outdated Show resolved Hide resolved

use ridless for unix os

6c7ec42

ericstj approved these changes Jun 23, 2020

View reviewed changes

remove extra warn

4517f18

Anipik merged commit 9907151 into dotnet:master Jun 23, 2020

ericstj mentioned this pull request Jun 25, 2020

GetCurrentProcessID to Get Pid #38365

Closed

Anipik deleted the privateUri branch August 25, 2020 21:52

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Conversation

Anipik commented Jun 19, 2020

Uh oh!

Dotnet-GitSync-Bot commented Jun 19, 2020

Uh oh!

ViktorHofer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

safern Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

Anipik Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

Anipik commented Jun 22, 2020

Uh oh!

ViktorHofer commented Jun 22, 2020

Uh oh!

ericstj commented Jun 22, 2020

Uh oh!

ViktorHofer commented Jun 22, 2020

Uh oh!

ericstj commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ericstj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ericstj Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

Anipik Jun 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ViktorHofer commented Jun 23, 2020

Uh oh!

Anipik commented Jun 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ericstj commented Jun 22, 2020 •

edited

Loading