Andy's first updates

zivkan · zivkan · commit 80a0db98249a · 2023-11-23T09:54:04.000+01:00
diff --git a/accepted/2022/Multiple-Equivalent-Framework-Support-TFM-As-Aliases.md b/accepted/2022/Multiple-Equivalent-Framework-Support-TFM-As-Aliases.md
@@ -1,13 +1,12 @@
 # Support for handling multiple equivalent framework
 
-- [Nikolche Kolev](https://github.com/nkolev92)
-- Start Date: (2022-08-18)
+- [Nikolche Kolev](https://github.com/nkolev92), [Andy Zivkovic](https://github.com/zivkan)
 - GitHub Issue ([5154](https://github.com/NuGet/Home/issues/5154)) - Treat Target Framework as Aliases
 
 ## Summary
 
 .NET SDK projects are capable of multi targeting based on a target framework where you can have different dependencies for each framework.
-In some cases, it is convenient to multi target based on different pivots such as target appplication version or maybe the specific runtime, as packages can contain runtime assets.
+In some cases, it is convenient to multi target based on different pivots such as target application version or maybe the specific runtime, as packages can contain runtime assets.
 This proposals covers the steps that need to be taken to enable that capability.
 
 ## Motivation
@@ -66,34 +65,42 @@ MSBuild version 17.3.0+92e077650 for .NET
 
 The missing part is allowing the same framework to be targeting among these aliases.
 
-The following scenarios would be enabled:
+The following hypothetical scenarios would be enabled.
+Note that these examples are not being proposed as syntax that will be implemented.
+Rather, they're intended to demonstrate a scenario that is not possible today.
+
+Firstly, a single project that creates multiple packages for different platforms, but the same TFM:
 
 ```xml
 <Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
-    <TargetFrameworks>latestnet-linux;latestnet-mac</TargetFrameworks>
+    <TargetFrameworks>linux;mac</TargetFrameworks>
   </PropertyGroup>
 
-  <PropertyGroup Condition=" '$(TargetFramework)' == 'latestnet-mac' OR '$(TargetFramework)' == 'latestnet-linux'">
+  <PropertyGroup>
     <TargetFrameworkIdentifier>.NETCoreApp</TargetFrameworkIdentifier>
     <TargetFrameworkVersion>v6.0</TargetFrameworkVersion>
     <TargetFrameworkMoniker>.NETCoreApp,Version=v6.0</TargetFrameworkMoniker>
+
+    <DefineConstants>$(DefineConstants);$(TargetFramework)</>
+    <PackageId>$(AssemblyName).$(TargetFramework)</PackageId>
   </PropertyGroup>
-  
-  </Project>
+
+</Project>
 ```
 
-Alternatively, the VS extension scenario would be something like:
+Secondly, a Visual Studio Extension project targeting multiple versions of VS could look similar to the following.
+In this example, the Visual Studio Extensibility SDK would be responsible for setting `TargetFrameworkIdentifier`, `TargetFrameworkVersion` and `TargetFrameworkMoniker`.
 
 ```xml
 <Project Sdk="Microsoft.VisualStudio.Extensibility.Sdk">
 
   <PropertyGroup>
-    <TargetFrameworks>vs17.2;vs17.3</TargetFrameworks>
+    <TargetFrameworks>vs17.7;vs16.11</TargetFrameworks>
   </PropertyGroup>
-  
-  </Project>
+
+</Project>
 ```
 
 ### Technical explanation
@@ -120,87 +127,124 @@ While this scenario does have a number of interesting uses, there are many comma
 | dotnet list package | dotnet list package | L | The work here would likely follow the work from the the restore side |
 | Visual Studio challenges | Multi targeting, displaying (transitive) dependencies | L(+) | |
 
-
-
-### Build challenges
+### Assets file changes
 
 In PackageReference, NuGet has a contract with the .NET SDK, where NuGet writes the assets file and the .NET SDK consumes it.
+Therefore, the assets file schema that NuGet writes must match what's expected by the .NET SDK.
 In most scenarios, people use `dotnet restore` and `dotnet build --no-restore` and there are no issues.
-It is not uncommon that people use `nuget.exe restore` due to the fact that they have non-SDK projects in their solution.
+It is not uncommon that people use `nuget.exe restore` due to the fact that they have non-SDK projects in their solution and they have not modernized their CI build to use MSBuild restore.
+Given that NuGet.exe and .NET SDK do not ship together, it is very common that the assets file was generated by a different version of NuGet.exe than what the .NET SDK can contains.
+Normally, this works.
+However, when NuGet makes a breaking change to the assets file, a significant number of customers will experience issues.
+We should design the feature to avoid incorrect results, and also make error messages as easy as possible to understand and resolve.
 
-Given that NuGet.exe and .NET SDK do not ship together, it is super common that the assets file was generated by a newer/older version of NuGet.exe than the .NET SDK can support. 
+#### Assets file version
 
-Normally, this works. NuGet rarely makes non-additive changes to the assets file.
+A breaking change in the assets file will be very difficult to implement unless we allow a period where NuGet supports multiple versions of the assets file in the same release.
+Additionally, the [.NET SDK assets file reader](https://github.com/dotnet/sdk/blob/main/src/Tasks/Microsoft.NET.Build.Tasks/ResolvePackageAssets.cs) and [non-SDK style projects assets file reader](https://github.com/dotnet/NuGet.BuildTasks) have different implementations, across different repos.
+Therefore, unless Nuget supports multiple multiple asset file versions in a single binary, it would be necessary to insert the change simultaneously across NuGet, the .NET SDK, NuGet.BuildTools, VS, and possibly MSBuild.
+This is so infeasible it's effectively impossible.
 
-Sample part of an assets file:
+Therefore, the proposal is that the .NET SDK will define an MSBuild property `ProjectAssetsFileVersion`, with allowed values `3` or `4`.
+When the property is not defined, it will assume `3`, in order to maintain backwards compatibility with the existing build tools.
+The .NET SDK already defines property `PropertyAssetsFile`, which is the location it reads the assets file from, hence the proposal of `ProjectAssetsFileVersion`.
+It may be that the non-SDK style project assets file reader (NuGet.BuildTools) may never migrate to v4 assets file, so supporting both assets file schemas could be a permanent cost, rather than a temporary situation.
 
-```json
-  "targets": {
-    ".NETFramework,Version=v4.7.2": {},
-    ".NETFramework,Version=v4.8.0": {}
-  },
+Reading the assets file will need a new data structure, and therefore will need new APIs to read.
+The goal is to enable the new data structure to be compatible with both v3 and v4 assets files, so that consumers of the assets file don't need multiple code paths to handle both assets file versions.
+This is not only relevant for the .NET SDK, but NuGet's own Visual Studio integration.
 
-```
+The assets file already contains a `version` property, always written out as the first property on the root object.
+This enables our assets file reader to use polymorphic deserialization to handle.
+Although if this turns out to be difficult to implement, we can provide separate V3 and V4 read methods (but both using the new data structures), and callers will need to check the `ProjectAssetsFileVersion` MSBuild property to determine which one to use.
+However, I'm reasonably confident that polymorphic deserialization should be feasible to implement.
 
-NuGet's assets file is written pivoting on the actual target framework.
+#### Inner build pivot
 
-To solve this, NuGet would need to:
+There are multiple places in the assets fie where the "NuGet Framework" (before .NET 5 it was just the `TargetFrameworkMoniker` MSBuild property, but since .NET 5 it can include the TargetPlatformMoniker as well, although in a simplified format) is used as a JSON property key.
+Sometimes with an additional Runtime Identifier (RID).
+Since the goal of this spec is to allow multiple `TargetFramework`s to resolve to the same "NuGet Framework", this means these parts of the assets file have to change.
 
-- Add vNext for the assets file format, version 4. Version 4 would either a dictionary based on the `TargetFramework` value instead of the effective target framework, built by coming `TargetFrameworkMoniker` and `TargetPlatformMoniker`.
-  - This behavior would probably have to be opt-in to avoid breaking all combinations of new `NuGet.exe`, old `.NET SDK` combinations, but we can choose to intentionally not do this to encourage maintaining the 2 versions equivalently.
-- Make a decision what would be a satisfactory behavior for combining newer/older versions of `NuGet.exe` and `.NET SDK`. This is largely just confirming some of the recent decisions.
+Here is a short extract from an assets file:
 
-To solve this, the .NET SDK would need:
+```json
+  "targets": {
+    ".NETFramework,Version=v4.8": {},
+    ".NETFramework,Version=v4.8/win7-x64": {},
+    "net8.0": {},
+    "net8.0/win7-x64": {},
+    "net8.0-windows7.0": {},
+    "net8.0-windows7.0/win7-x64": {},
+  },
+```
+
+Currently the `targets` section of the assets file is loaded into a `Dictionary<string, NotRelevant>`, and therefore the `targets` node cannot contain two identical TFMs when two Target Frameworks resolve to the same TFM. The proposal is to change this, and all other examples in the assets file, to use the `TargetFramework` as a string literal instead.
 
-- Be able to `build` with both versions 3 and version 4 of the assets file. The NuGet dependencies would naturally flow into the .NET SDK and the work is to just differentiate between the 2 different versions.
-  - The amount of work needed to be done here would likely be affected by the exact shape of the NuGet APIs. These changes may be seamless.
+Todo: figure out best choice for proposed data model & schema.
+Will there be a problem if customer uses `TargetFramework` with a `/` character?
 
-### Restore challenges
+### Lock file changes
 
-In addition to the assets file, restore has a lock file which has a similar schema, that schema would need to be amended. Fortunately when Central Package Management and its transitive pinning was introduced we introduced the concept of a PackagesLockFile version succesfully. We'd just add a version 3.
+NuGet's "repeatable build" feature adds a package lock file to the project directory.
+The restore has a lock file has a similar schema to the assets file, so that schema would also need to be amended.
+Fortunately when Central Package Management and its transitive pinning was introduced we introduced the concept of a PackagesLockFile version successfully.
+We'd just add a version 3.
 
 NuGet would:
 
 - Add version 3 of the packages lock file.
 
-NuGet's intra restore caching is based on the existing of a single framework. It is difficult to predict the amount of work that'd be required to implement this correctly, but it'll certainly involve public API changes to NuGet libraries. (NuGet ships libraries, NuGet.exe, VS and dotnet.exe)
+NuGet's intra restore caching is based on the existing of a single framework.
+It is difficult to predict the amount of work that'd be required to implement this correctly, but it'll certainly involve public API changes to NuGet libraries.
+(NuGet ships libraries, NuGet.exe, VS and dotnet.exe)
 
-### Pack challenges
+### Pack changes
 
-Multi-targetted pack only really works when each build leg (TargetFramework value) has 1 target location such as `lib/net5.0`.
-Packing projects that do target the same framework multiple may be helpful in some scenarios, so completely blocking it is likely not a great idea, but having a coherent experience would require better understanding of the scenarios at question.
-
-At the minimum, the pack command should be able to detect the scenario at hand and *not succeed queitly* as there's no guarantee what gets packed.
-This scenario would require separate design work.
+For the first version of this feature, projects that have more than one `TargetFramework` (inner-build) resolve to the same "NuGet Framework" will result in a pack error, therefore preventing these projects from being packed.
+See [Extending Pack, under Future Possibilities](#extending-pack) for more information.
 
 ### dotnet.exe challenges
 
-Today, some values in dotnet.exe work with an aliased `TargetFramework`, such as `build` and `publish`. 
+Today, some values in dotnet.exe work with an aliased `TargetFramework`, such as `build` and `publish`, using the `--framework` or `-f` argument.
 The expectation is that all the commands support this, but this would require a deeper investigation.
 
 #### dotnet list package
 
-`dotnet list package` has an output that pivots on the effective target framework. This would need to be changed to use the aliased value.
+`dotnet list package` has an output that pivots on the effective target framework.
+This would need to be changed to use the aliased value.
 
 NuGet would need to:
 
 - Display the output with an aliased pivot.
-- Update the machine readable output to handle the new schema, <https://github.com/NuGet/Home/issues/11449>. Given that the machine readable output has not shipped yet, this may be something that can be done ahead of time.
+- Update the machine readable output to handle the new schema, <https://github.com/NuGet/Home/issues/11449>.
 
 #### Visual Studio challenges
 
-Currently the PM UI does not have any support for multi targeting. This isn't anything new, but it can affect the experience of customers moving to SDK projects for the first time.
+Currently the PM UI does not have any support for multi targeting.
+This isn't anything new, but it can affect the experience of customers moving to SDK projects for the first time.
+Especially for customers who are not comfortable hand editing XML or MSBuild files.
 
 Another concern are all the APIs for displaying the dependencies, transitive dependencies and Get Installed Packages APIs in NuGet.VisualStudio.Contracts.
 All of these APIs may require an incremental addition.
 
+#### Project to Project references
+
+MSBuild has a [`ProjectReference` protocol](https://github.com/dotnet/msbuild/blob/25fdeb3c8c2608f248faec3d6d37733ae144bbbb/documentation/ProjectReference-Protocol.md), which explains how `ProjectReferences` work.
+This will need changes to support scenarios where a project where more than one `TargetFramework` resolves to the same "NuGet Framework".
+
+Additionally, NuGet's `PackageReference` requires a project's entire dependency graph to be resolved, which includes projects, in order to calculate transitive packages.
+Therefore, NuGet has the same, or at least very similar, problems as MSBuild with regards to `ProjectReference`s.
+
+This needs further discussion between the MSBuild and NuGet teams.
+
 ## Drawbacks
 
 <!-- Why should we not do this? -->
 
 ## Rationale and alternatives
 
-- N/A. This is a binary decision to either support or not support this scenario.
+From a high level feature point of view, the alternative is that customers must use separate projects, rather than a single project file.
+Therefore, this feature is a binary decision to either support or not support this scenario.
 
 ## Prior Art
 
@@ -215,6 +259,31 @@ All of these APIs may require an incremental addition.
 <!-- What parts of the proposal need to be resolved before the proposal is stabilized? -->
 <!-- What related issues would you consider out of scope for this proposal but can be addressed in the future? -->
 
+### Exact assets file schema changes
+
+Should we "simply" replace the TFM in property keys with the Target Framework alias?
+For example, change ".NETFramework,Version=4.7.2/win-x64" with "vs17.7/win-x64"?
+But there will be parsing problems when a `TargetFramework` contains a `/` character.
+While "something/another thing/win-x64" can determine that "win-x64" is a RID, when we get the RID-less "something/another thing" string, it's not clear that what follows the `/` is a RID, unless the assets file parser starts knowing the entire RID graph.
+
+### How ProjectReferences will work
+
+Currently ProjectReferences (P2P) do a compatibility check, both by MSBuild and NuGet (although I'm sure MSBuild delegates to NuGet APIs).
+When a project has multiple `TargetFramework`s that map to a single "NuGet Framework", which one should MSBuild and NuGet use?
+MSBuild needs to choose in order to know which dll to pass to the compiler (in case different aliases have different APIs), and so test projects actualy test the desired product binary.
+NuGet needs to know for the package dependency graph.
+
+A simple option is to match the alias string.
+But do we want to support different projects using different aliases?
+If so, then a way to "set the desired alias in the target project" will be needed.
+That may be usable as a way to [override selection of "nearest" framework](https://github.com/NuGet/Home/issues/7416), which may or may not be considered a good thing, depending on your view if overriding asset selection is good or not.
+
 ## Future Possibilities
 
-<!-- What future possibilities can you think of that this proposal would help with? -->
+<!-- What future possibilities can you think of that this proposal would help with? -->
+
+### Extending Pack
+
+It seems feasible that if a single project can target multiple versions of Visual Studio, through a new Visual Studio Extensibility SDK, that someone will also want to create a package that supports multiple versions of Visual Studio.
+
+Designing how this could work is out of scope for this spec, and can be introduced in a different spec.
