Make .NET Framework fallback open-ended #2425
Comments
|
Not strictly related, but 15.8 added P2P refs based on AssetTargetFallback, but failed to match the warning of package refs. While I've got interested people's attention: NuGet/Home#7137 |
|
[Copying comment I made on the original issue] Joining a bit late this conversation, but wouldn't it be best if we first try net461 in case there is one? For example, there is a package with a net47 and a net461 implementation. Previously we would have used the net461 one, but if we make that change I believe we will now grab the net47 one, when we know that the net461 is more likely to be compatible. Should we add some special logic for that case? |
|
@joperezr That is a very interesting point! It could turn this into a breaking change when different assets are resolved. The whole episode of retroactively making things compatible with netstandard 1.5+, and thereby changing assets resolved with an sdk upgrade, still gives me nightmares... I believe we can make the default cc @rrelyea |
|
That's what I was getting at with this comment: dotnet/standard#724 (comment). IMHO one of the goals of this issue is to make the compat mapping easier to understand. Having a tiered fallback feels more complicated. Having an inverse fallback also feels wrong as it is counter to all other nuget behavior. The retroactive netstandard compatibility break was a lot harder to deal with because it happened inside nuget and there was no way to undo it once someone updated nuget. Here we have a more flexibility since this is in msbuild and property can be overridden. |
|
We need to be much more careful about saying "you can tweak your project to get unbroken" IMHO. There are times when it's the right call, but I see us defaulting to it too often. We can't make things easier to understand when we change the nuget graph of your app from one sdk version to another because that change is very hard to understand. This is not like adding a warning, which is at least in your face as a new behavior with an obvious remedy. It's incredibly subtle if you just start getting net47 binaries in your bin folder where previously you had net46. The break could be discovered well after the sdk upgrade. Maybe we can make net50 the default fallback for netcoreapp2.1/netstandard2.1. If you want to get it earlier than that, then you can put net50 in your project. |
|
That's a very good point and I'm glad you pointed this out. As @nguerrera said:
👍 😱
Couldn't we just do this: I understand that this is less ideal than implementing the actual logic, but given how often .NET Framework ships, this might be a good compromise. The "nice" thing about this is that adding new items to the right isn't a breaking change as it would only make more scenarios work that previously would have resulted in no assets, so even if the SDK doesn't ship exactly at the same time as the .NET Framework targeting pack, the worst that can happen is that customers complain that certain NuGet packages (i.e. the ones that target the latest version of the .NET Framework) aren't consumable via the compatibility mode, which in general should be rare as packages under active development hopefully offer a .NET Standard asset anyway. Thoughts? |
|
I know that this would make the logic more complex which goes against what @ericstj would want (from what I read on this thread) but seems like setting it to that value would at least give us the behavior that we want. |
|
I think that is a little ugly, but actually totally addresses the concern, so 👍 |
|
Agreed ugly, but works. Not exactly fun to maintain, but at least its WYSIWYG. I'd want to run that by NuGet to make sure it doesn't cause some crazy perf hit for packages with non-applicable assset groups (not failing ones, but those which still install). I'm thinking meta-packages, or packages with some optional assets that are only applicable on non-NETFX/NETCoreApp/NETStandard frameworks. I know that theoretically AssetTargetFallback should only trigger when a package is not at all applicable, but I can't remember if the implementation bears that characteristic completely. It could be exaggerated by carrying that pattern out until it has like 20 or 30 imaginary versions and running it against a set of sufficiently diverse packages. /cc @rrelyea |
|
@livarcocc @nguerrera can we get this done for .NET Core 3.0? |
|
Can I mark this work item as committed in our backlog? Or are there more concerns regarding timelines? |
|
@nkolev92 @jainaashish @rrelyea Do you see a problem with this proposal: See Eric's comment just above about perf. Obviously, we can and should measure that, but do you have any concerns (perf or otherwise) from the outset? |
|
I was just bitten by this...packages updated to net472 and it broke me until I added the AssetTargetFallback. Bump on getting this in for 3.0. |
|
That will definitely carry a performance overhead when ATF is actually hit. I don't expect it to spiral out of control, but it will be noticeable. We'd need to investigate some more and do some testing to confirm this. Another concern is ATF related deficiencies in the implementation. |
|
cc @terrajobst. We're getting asked to implement this by preview 4, but we would need NuGet's blessing to do it as proposed. |
Is it proportional to where the matching TFM appears in the list or proportional to the number of TFMs regardless of which one matches? In other words, if a package has net461 compatible assets, will there be any slow down to resolve them between AssetTargetFallback=net461 and AssetTargetFallback=net461;net462;net47;net471;net472;net48 ? |
|
It's proportional to the number of tfms until it matches. So the answer to the question is no. |
|
So it sounds like it would only slow down cases that fail today, in which case I think it is OK. How can we verify that? |
|
I agree. Makes sense then. |
Aside: Hadn't seen that before. It instantly explained dotnet/installer#78 (now closed as duplicate). :) I think we're going to see a lot of people hit this with .NET Core 3.0 bringing up classic desktop UI. It's separate, though. |
|
Should the ordering go the other way? If an asset exists for both net48 and net461, should we actually prefer the net48 one? |
|
@Pilchie We are keeping net461 first for compat. See whole discussion above about how suddenly getting different assets is the subtlest of breaks. Recall the whole change in what can use netstandard2.0 between nuget versions. If we want to prefer newer over the rest, then this might as well be net461;net48. Anything lower than net48 after net48 doesn't serve any purpose. If you can't resolve for net48, you can't resolve for net472 either, etc. It makes some sense to me to try to pick the earlier TFMs when crossing streams. (Older TFMs have fewer API to trigger an incompatibility). To be completely honest, though, I dislike this whole feature being implicit and having to decide what magic to use. I wish it were just explicit. |
|
We can change it to net48 by default for new TFMs. netstandard2.1, netcoreapp3.0. And maybe we change netcoreapp2.0/netstandard2.0 to That would probably reduce the perf risk. |
@nguerrera already addressed the compat angle. But more importantly, the .NET Framework compat mode is best effort. If a package overs 4.6.1 and 4.8, presumably that's because it uses more features in the 4.8 binary. The 4.6.1 binary has a higher chance of working in .NET Standard compliant implementations though, which is why it still makes sense to prefer them over the later assets. As far as the perf riks goes: assuming the perf hit only slows down error cases, then I'm OK with that, unless the result is unbearable. |
|
Putting this in now and verifying perf. One quirk, the warning now looks like this (all the frameworks are listed). It's correct, just a little long. |
|
We can improve/fix that message in the latest clients for sure. At the least we should first call out the exact tfm used. Also I thought it was supposed to be |
|
I don't think that was decided. My reading was that we should go with what is in the proposal unless there's a perf issue. I have tested and cannot find a perf issue. With refs to 10 packages that have only net48 assets, the full list restores in the same time as just net48 in ATF. |
|
net461;net48 is rather random. I think it's easier to explain the feature as the earliest available asset for >= net461. |
When a developer references a NuGet package that doesn't provide any assets for the current .NET platform, the SDK will fallback to try .NET Framework assets, provided the current .NET platform supports at least .NET Standard 2.0.
The fallback works by telling the NuGet selection algorithm "pretend I'm .NET Framework 4.6.1 so select me assets for that". We determined that to be unfortunate and would like to change that to .NET Framework 5.0.
The rationale being:
Update
@nguerrera raised concerns regarding breaking people. So we generally want an algorithm that picks the "smallest" .NET Framework as that wouldn't change the behavior from today or we add new versions of .NET Framework. So I propose instead of mapping it straight to 5.0 we do this:
/cc @weshaggard @ericstj @davidfowl @Petermarcu @dsplaisted @nguerrera
The text was updated successfully, but these errors were encountered: