Design for allowing restore of multiple equivalent frameworks (Target Frameworks as Aliases)

[nkolev92]

Design for #5154

Rendered

cc @dsplaisted

[Nirmal4G]

Some ideas and suggestions. Feel free to ignore it. 😅🙃

[nkolev92]

Thanks for taking a look @Nirmal4G

For context, this is design I'm still working on, I'd say it's at about 50%.

Hoping to add more details about the scenarios and just in general provide more concrete proposals soon.

[Nirmal4G]

@nkolev92 These are some of my observations and preliminary suggestions, ideas and takeaways from this draft. I'm not taking this as a concrete proposal. I'm just viewing this from a 10-foot view (or design perspective) of the proposal to see how it could impact normal as well as advanced users. In that mindset, read through my comments again. You would then see what I was talking about.

[Nirmal4G]

Hoping to add more details about the scenarios and just in general provide more concrete proposals soon.

Sure, Will look though once you mark this ready for review. Sorry for the uninvited comments. I'll stop now.

[nkolev92]

Hoping to add more details about the scenarios and just in general provide more concrete proposals soon.

Sure, Will look though once you mark this ready for review. Sorry for the uninvited comments. I'll stop now.

Oh no worries. I appreciate the feedback as this is a large doc so getting feedback early on is always great.

My comment was more about that fact that I'll be adding even more info to the doc, setting expectations about when something like this would even be close to happening :)

[JonDouglas]

@ViktorHofer & @ericstj

[ghost]

This PR has been automatically marked as stale because it has no activity for 30 days. It will be closed if no further activity occurs within another 15 days of this comment, unless it has a "Status:Do not auto close" label. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

[zivkan]

oops, vacation and other priorities has kept this feature on the backburner, but it's still planned.

[jviau]

For a functions extensibility scenario, I am considering having extension packages include a well-known marker file in their nupkg. I was going to leverage the fact that the assets file already has all nupkg file contents listed in it to speed up that process.

With this change, is the suggestion I would need to manually scan the file system of every nuget package? Or do you have an idea of a more general way I can add metadata to a nuget package and read it during a build?

Today we have an assembly attribute and we use Mono.Cecil to scan runtime assemblies from nuget looking for that attribute. This is understandably not performant. Which is why I am considering the marker file approach. I wouldn't block on this change just for our use case, but some metadata system for nupkgs that can be read at build time would be helpful (but obviously that would be an entirely different proposal).

[nkolev92]

@ericstj @ViktorHofer @dsplaisted @baronfel @zivkan @jeffkl

This is ready for review.

It's a complicated, but I'd love to get through it async as much as possible because of the length itself :D

[dsplaisted]

How hard would it be to have some sort of extensibility point so that you can add your own logic to choose the "nearest" framework if the NuGet logic results in more than one match?

[ViktorHofer]

To add to that question, in the "No match found" case, would it make sense to return that information without raising an error / warning so that we could allow for customization in the msbuild target instead of in the task (which would give us much more flexibility)?

We still want to raise an error if there's no customization (I'm trying to avoid the word "fallback" here as it has a different meaning for NuGet). That would probably need to be moved outside of the task, into the target.

[nkolev92]

How hard would it be to have some sort of extensibility point so that you can add your own logic to choose the "nearest" framework if the NuGet logic results in more than one match?

It's not impossible. I'd love to see any usage of this multiple framework project reference scenario before we want that direction though.

To add to that question, in the "No match found" case, would it make sense to return that information without raising an error / warning so that we could allow for customization in the msbuild target instead of in the task (which would give us much more flexibility)?

Can you help me understand how this could help?
If there's no nearest match/restore/build should be failing.
I see this as equivalent to a nearest framework not matching at all.

We still want to raise an error if there's no customization (I'm trying to avoid the word "fallback" here as it has a different meaning for NuGet). That would probably need to be moved outside of the task, into the target.

The target that calls the task does not run during restore.
In the core restore algorithm, NuGet picks the nearest project ref and that's based on the data provided to NuGet and collected at the beginning of restore.
Effectively, NuGet does the same thing internally when choosing to flow the package references coming through projects.

[ViktorHofer]

I see, fair point. I was basically wondering if we can add an extension point / a callback to provide additional information that would allow us to extend the selection algorithm for the multiple or non found cases.

Runtime today overwrites the entire selection algorithm target (msbuild) and task (nuget) due to not an extension point being available. This isn't great and needs an update every time the P2P protocol is touched and the msbuild target gets updated.

[nkolev92]

I know you're not focusing on that right now, but what are some practical examples where the runtime needs to overwrite the selection.
I was hoping alias matching would address a bunch of those needs.

[ViktorHofer]

RID based selection. I.e. net10.0-linux should be able to reference net10.0-unix (as unix is a parent of linux in the graph). The current algorithm as you describe in the doc wouldn't be able to handle that but it would support direct matches, i.e. net10.0-unix referencing net10.0-unix.

We can probably workaround that limitation if necessary or even change the RIDs that we target in TFMs but yeah...

[nkolev92]

I see. That's the next step then yeah.
Rids are a cross product, but maybe it'll be possible to add first class support for nearest that accounts for the RID as well.

[dsplaisted]

I probably wouldn't try to include RID matching in NuGet or the SDK for the first version of this, ideally just an extensibility point so that people could add it.

[nkolev92]

I think in V1, we can just add alias matching support, but I'll create a follow-up for the extensibility point.
Right now, the current technical implementation does not allow a computational extensibility.

[zivkan]

Customers are expected to change SDKAnalysisLevel if they have build errors when upgrading to a newer SDK. I can't remember the name of the property, but there's an SDK version that feels more appropriate for what this is aiming to achieve.

[nkolev92]

Yeah, there's an SDK version property for sure, we raise telemetry on that, but I see SDKAnalysisLevel to mean, behave like the previous SDK version, and to me that'd imply the older restore behavior in general.

I don't feel too strongly though, and can easily be convinced otherwise.

[zivkan]

In my mind, SdkAnalysisLevel is about warnings and errors (diagnostics), not features, and looking at the original spec, I feel my opinion is reinforced: https://github.com/dotnet/designs/blob/main/proposed/sdk-analysis-level.md

The spec doesn't say it, but I always thought SdkAnalysisLevel was inspired by Roslyn's AnalysisLevel, and Roslyn also has LangVersion which I consider to be the feature toggle. The SDK/NuGet doesn't have a "global feature toggle" like Roslyn does with LangVersion. Using SdkAnalysisLevel is a choice that can be made, but then customers lose the ability to enable new features while disabling new diagnostics. I guess we need to decide whether that's an important distinction to allow customers to have.

[nkolev92]

I think Chet has a good compass when it comes to SDKAnalysisLevel.

Curious what you think @baronfel

tldr;
NuGet will introduce a V4 of the assets file.
It requires a new .NET SDK reader, ie a specific .NET SDK version.

Should that "switch" be based off of the SDKAnalysisLevel or NETCoreSdkVersion property.
More context on the pros and cons in the above comments.

[zivkan]

Considering the assets file has an entry for the ridless target, in addition to the per-rid targets, I think it would be challening to tell the difference between the two.

If the parser keeps raw strings in memory and then parses the alias/rid only once all targets are parsed from json, then you can find what the max number of / are, and assume the ridless has n-1.

But I think it's easier if we fail restore if the alias has a /. It's easier to block first and later allow, than the other way around.

Since we're already incrementing the version number, I think it'd be ideal if we could make another breaking change to the format, so instead of

"targets": {
   "alias" : {  },
   "alias/rid": {  }
}

we could instead have

"targets": {
   "alias": {
     "": { },
     "rid": {  }
  }
}

But I suspect this might be a lot harder to implement.

However, as is pointed out just below, the target alias is used as the directory name for the obj and bin directories, and / is a directory separator, so I think that's another reason we could just block / from the alias

[nkolev92]

Yeah, I'd be fine with blocking that yeah.
Honestly, I can't image this to be a super critical scenario that would need to be addressed.
The more changes we make, the higher the risk of the change anyways.