search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.42k stars 4.76k forks source link

System.ComponentModel.TypeConverter package is resolving to wrong asset for netcore50aot and uap101aot) #18464

Closed SedarG closed 4 years ago

SedarG commented 8 years ago

TargetFrameworks supported by https://dotnet.myget.org/feed/dotnet-core/package/nuget/System.ComponentModel.TypeConverter are:

Restoring this package for netcore50aot or uap101aot will result in NetStandard1.0 to be picked up, which is the wrong asset (it's missing embedded RD.xml). We need to explicitly target netcore50aot with the correct binary asset.

This issue exists on live package as well as 4.1.0 stable package which is being included in the closure of UWP MP 5.2.2

/cc @weshaggard

weshaggard commented 8 years ago

cc @ericstj Is it just the missing RD.Xml that is the issue if so should we just include that instead of explicitly adding a netcore50aot asset?

ericstj commented 8 years ago

NetStandard1.0 to be picked up, which is the wrong asset

Which is the "right asset" then? I don't see any build of this with an RD.xml. As Wes said if you want an RD.xml just add that... 'NETStandard1.0' is correct given the set of impls currently built for this library. Also, don't just cross-compile to add an RD.xml you can add that to any configuration of the library.

SedarG commented 8 years ago

Which is the "right asset" then?

I don't know which one is the right asset. it could be as simple as adding RD.xml to the binary under lib/netstandard1.0, or changing the packaging such that the asset under lib/netstandard1.5, which does contain RD.xml, gets picked by NuGet restore (by possibly placing that binary also under lib/netcore50. I have no idea which binary is the right one to pick.

ericstj commented 8 years ago

changing the packaging such that the asset under lib/netstandard1.5, which does contain RD.xml, gets picked by NuGet restore

That would be incorrect. NETCore50 only supports netstandard1.4, so as I mentioned, the current packaging is correct.

It sounds like you're just making a statement that the implementation requires an rd.xml whenever it's used by netnative. In that case the issue is simply to add an RD.xml to all implementations that may be used by netnative. In this case that'd be both of the netstandard implementations.

@SedarG can you provide more detail on what the content of the rd.xml should be and how it should be included?

SedarG commented 8 years ago

can you provide more detail on what the content of the rd.xml should be and how it should be included?

the existing RD.xml which is being applied to netstandard1.4 needs to be applied to netstandard1.0 too. This can be done by adding an EmbeddedResource, for relevant TargetGroup in csproj. See https://github.com/dotnet/corefx/blob/master/src/System.ComponentModel.TypeConverter/src/System.ComponentModel.TypeConverter.csproj#L101 for reference

@ericstj I see that we're not cross compiling for netstandard1.0 on master anymore. that binary get harvested from 4.1.0 package. How can we service this library?

ericstj commented 8 years ago

Yes, you can service the library by bringing back the netstandard1.0 configuration in the builds. It looks like @Priya91 removed this configuration a bit prematurely in https://github.com/dotnet/corefx/commit/878080cb52f209ee00c330e32cc20e1281d3b54f#diff-844c1fa74fb54f0f606c7bb1394b8417L13.

Just to restate the guidance: folks shouldn't be deleting the downlevel configurations of projects if that configuration is substantial and folks intend to service it. /cc @weshaggard @stephentoub as FYI.

SedarG commented 8 years ago

@Priya91 , would you mind bringing back netstandard1.0 configuration.

Priya91 commented 8 years ago

@Priya91 , would you mind bringing back netstandard1.0 configuration.

Spoke with @ericstj, and when this work was done, the guidelines was to remove the previous configurations and let servicing happen from rel branches. But now, the general guideline is for fat packages above netstandard to have cross compiling enabled is less cost for a bug fix. Will make the change.

weshaggard commented 8 years ago

Just to restate the guidance: folks shouldn't be deleting the downlevel configurations of projects if that configuration is substantial and folks intend to service it. /cc @weshaggard @stephentoub as FYI.

@ericstj there is no good way to determine this and so the only guidance would be to keep all configurations that have any code, which as we have talked adds a ton of complexity to our repo. I would still like to eliminate builds by default and only included them if we need to ship an update to that configuration in the latest package. Or for cases like this actually make the change in the servicing/release branch instead.

@SedarG for this issue can we not just include an rd.xml file directly in the package and not need to embed it in the binary?

SedarG commented 8 years ago

for this issue can we not just include an rd.xml file directly in the package and not need to embed it in the binary?

I'm not sure about it. the RdXml file needs to be at ILC input directory for the toolchain to pick it up. If NuGet targets copies the RdXml together with the library it'd work.

@weshaggard, @ericstj why aren't we servicing 4.1.0 package instead?

weshaggard commented 8 years ago

I'm not sure about it. the RdXml file needs to be at ILC input directory for the toolchain to pick it up. If NuGet targets copies the RdXml together with the library it'd work.

I'm not entirely sure if this happens or not but I do think it is worth trying out.

@weshaggard, @ericstj why aren't we servicing 4.1.0 package instead?

We can do that but it requires actually doing the work in our release branch and producing a new package from there and then updating master to consume it once it is done.

SedarG commented 8 years ago

I'm not entirely sure if this happens or not but I do think it is worth trying out.

I'd rather not. None of the libs ship rd.xml like that today.

We can do that but it requires actually doing the work in our release branch and producing a new package from there and then updating master to consume it once it is done

I see. It doesn't sound like too big of a win.

@Priya91, when will you be able to bring back the missing configuration

Priya91 commented 8 years ago

@Priya91, when will you be able to bring back the missing configuration

I'm working on other things today, will do tom.

Priya91 commented 8 years ago

@SedarG Looked at bringing back netstandard1.0 configuration back to typeconverters, the subsequent files have been deleted by @alexperovich in commit .. This is taking more work than expected.

weshaggard commented 8 years ago

@Priya91 your work will also conflict with @AlexGhiondea PR https://github.com/dotnet/corefx/pull/12081 please be sure to coordinate.

danmoseley commented 8 years ago

@SedarG, this is apparently not a minor work item and @Priya91 is busy with networking API. This may not be possible to do soon.

/cc @karelz

karelz commented 8 years ago

Is it part of netstandard2.0? Or what else is it needed for?

SedarG commented 8 years ago

Is it part of netstandard2.0? Or what else is it needed for?

It's really hard to answer this question. netstandard2.0 is ambiguity. Are you referring to it as a release target or, the netstandard 2.0?

I'm giving it a try: (This is an AOT only failure. So answering your question in context of AOT). We are not planning to expand uap10.1 more than where it is right-now. In that regard the NS2.0 APIs exposed in the new version of this library will not be exposed to UWP scenarios.

On the other hand, this is a regression in already shipped UWP Meta Package (5.2.2). Setting aside the obvious - we need to fix this regression for current users of UWP. For the next release of UWP, the highest priority is to make sure that it has a smooth out-of-the-box experience. So we should either

Hope I could answer your question.

danmoseley commented 7 years ago

@zamont do you understand this issue? Is it still relevant?

joshfree commented 7 years ago

@joperezr could you take a quick look?

joperezr commented 7 years ago

Is it still relevant?

No. TypeConverter doesn't have it's own package any more as it is part of both of our metapackages (netcoreapp and uap). This means that the assembly that is used for UWP6.0 will always contain the rd.xml, and when people target uap10.0 (netcore50) the behavior will fall back to whatever it used to be when we shipped netcore50, which is to resolve the netstandard1.0 asset that doesn't have rd.xml. I would just close this issue since the broken state is only for uap10.0 so we wouldn't be regressing anything.