Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assets.json doesnot contains the project references for some of the platform specific older(< net5.0) frameworks #46338

Closed
Anipik opened this issue Dec 22, 2020 · 12 comments
Labels
area-Infrastructure-libraries untriaged New issue has not been triaged by the area owner

Comments

@Anipik
Copy link
Contributor

Anipik commented Dec 22, 2020

For older frameworks we remove the os from tfm while running the restore, hence some of the project references under the targetsPlatform condition are not recogonized for those platforms. Hence they are not added in the project.assets.json file.
We need to fix this as pack task uses the assets file to add the package dependencies.

cc @ViktorHofer @safern @ericstj

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Infrastructure-libraries untriaged New issue has not been triaged by the area owner labels Dec 22, 2020
@ghost
Copy link

ghost commented Dec 22, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

For older frameworks we remove the os from tfm while running the restore, hence some of the project references under the targetsPlatform condition are not recogonized for those platforms. Hence they are not added in the project.assets.json file.
We need to fix this as pack task uses the assets file to add the package dependencies.

cc @ViktorHofer @safern @ericstj

Author: Anipik
Assignees: -
Labels:

area-Infrastructure-libraries, untriaged

Milestone: -

@ViktorHofer
Copy link
Member

Our custom target framework platform shouldn't end in the generated packages anyway? As the NuGet won't add TargetPlatformMoniker support to .NETCoreApp < 5, I assume we won't be able to fix the described issue without being able to control the passed in dependencies per tfm?

@Anipik
Copy link
Contributor Author

Anipik commented Dec 22, 2020

Our custom target framework platform shouldn't end in the generated packages anyway?

Thats correct it will be proxy for that is platformless one (the one in the assets file)

As the NuGet won't add TargetPlatformMoniker support to .NETCoreApp < 5, I assume we won't be able to fix the described issue without being able to control the passed in dependencies per tfm?

We can fix the issue by modifying our csprojs to use conditions which are true for netcoreapp2.0 and netcoreapp2.0-platform for just project references.

@ViktorHofer
Copy link
Member

ViktorHofer commented Dec 22, 2020

We can fix the issue by modifying our csprojs to use conditions which are true for netcoreapp2.0 and netcoreapp2.0-platform for just project references.

Wouldn't that mean that these P2Ps wouldn't then be part of the project.assets.json file per project and hence these project wouldn't be restored?

AFAIK the project.assets.json file is shared between restore and pack and can't be mutated just for packaging.

@Anipik
Copy link
Contributor Author

Anipik commented Dec 22, 2020

Wouldn't that mean that these P2Ps wouldn't then be part of the project.assets.json file per project and hence these project wouldn't be restored?

Today they are not part of projects.assets.json, after fixing these, project references will be part of projects.assets.json

@ViktorHofer
Copy link
Member

Can you please elaborate on the current issue and how you are trying to fix it? Maybe you can also provide an example so that we know that we are talking about the same thing?

@Anipik
Copy link
Contributor Author

Anipik commented Dec 22, 2020

This is system.diagnostics.eventlog
The project target netcoreapp2.0-windows.

 <ItemGroup Condition="'$(TargetsWindows)' == 'true'">
    <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Win32.Registry\src\Microsoft.Win32.Registry.csproj" />
    <ProjectReference Include="$(LibrariesProjectRoot)System.Threading.AccessControl\src\System.Threading.AccessControl.csproj" />

....
...
  <ItemGroup Condition="!$(TargetFramework.StartsWith('net4'))">
    <ProjectReference Include="$(LibrariesProjectRoot)System.Security.Principal.Windows\src\System.Security.Principal.Windows.csproj" />
  </ItemGroup>

The project.assets.json file just contains this entry for netcoreapp2.0

      "System.Security.Principal.Windows/6.0.0-dev": {
        "type": "project",
        "framework": ".NETStandard,Version=v2.0",
        "compile": {
          "bin/placeholder/System.Security.Principal.Windows.dll": {}
        },
        "runtime": {
          "bin/placeholder/System.Security.Principal.Windows.dll": {}
        }
      },

It should contain similar entry for other 2 project references as well. does it makes sense ?

i intend to fix it by adding

<ItemGroup Condition="$(TargetFramework.StartsWith('netcoreapp2.0'))">
    <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Win32.Registry\src\Microsoft.Win32.Registry.csproj" />
    <ProjectReference Include="$(LibrariesProjectRoot)System.Threading.AccessControl\src\System.Threading.AccessControl.csproj" />
</Itemgroup>

@ViktorHofer
Copy link
Member

It should contain similar entry for other 2 project references as well. does it makes sense ?

Absolutely, thanks for clarifying. We discussed that when we enabled project restore and knew that we can't have P2Ps conditioned on the platform. I thought we even had a target that checked for that but apparently we never added one. How many cases of such pattern do we have?

@Anipik
Copy link
Contributor Author

Anipik commented Dec 22, 2020

I thought we even had a target that checked for that but apparently we never added one. How many cases of such pattern do we have?

i didnt run the numbers, i will update when i do so. I will try to put up the pr for this by today.

@Anipik
Copy link
Contributor Author

Anipik commented Dec 22, 2020

On second thought i will add with enabling the package csproj for these projects

@ViktorHofer
Copy link
Member

Do you already have a number of how many projects are affected? I think we should fix this anyway as it's an inconsistency in the restore graph.

@Anipik
Copy link
Contributor Author

Anipik commented Dec 23, 2020

Do you already have a number of how many projects are affected?

the number is around 9-10 but that was just a regex search. i am thinking if we could automate it or add a check for this.

@Anipik Anipik closed this as completed Jan 4, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

3 participants