search

NuGet / Home

Repo for NuGet Client issues
Other
1.5k stars 252 forks source link

[Bug]: Packing a multi-target project fails to find the configured readme file #11467

Open tonygiang opened 2 years ago

tonygiang commented 2 years ago

NuGet Product Used

dotnet.exe

Product Version

6.0.101

Worked before?

No response

Impact

It's more difficult to complete my work

Repro Steps & Context

  1. Create a blank C# project.
  2. Create a readme.md file in the project.
  3. Configure this readme file to be included in the package by right click project > Properties > Package > General > README, browse to said readme.md. Build and pack project once to verify that it packs successfully at this step.
  4. Open the project's .csproj file, find TargetFramework tag and change it to TargetFrameworks. Note: adding additional target framework(s) in this tag is not necessary.
  5. Build and pack project again to see the error.

Verbose Logs

Error code: NU5039
The readme file 'readme.md' does not exist in the package.
A demonstrative video of reproducing this bug can also be found here: https://cdn.discordapp.com/attachments/598678594750775301/924001576643878923/2021-12-25_00-57-31.mp4
donnie-msft commented 2 years ago

When I repro this, I get an error indicating that there's no TFM for my project. It warns me that Restore won't work correctly. In that case, does the readme error make sense? /cc @nkolev92 If so, why doesn't the issue author see the same error? image

nkolev92 commented 2 years ago

@donnie-msft There's a video by the OP.

In that video, the OP hits the same isssue, but clicks Reload projects.

Can you try the same thing?

nkolev92 commented 2 years ago

Actually in the video, I can see the issue: 


<PropertyGroup>
...
<PackageReadmeFile>docs\readme.md</PackageReadmeFile>
...
</PropertyGroup>

<ItemGroup>
   <None Update="docs\readme.md">
      <Pack>True</Pack>
      <PackagePath>\</PackagePath>
</ItemGroup>

If that's the generated XML by the project system, I'm not surprised it doesn't work.

If you look at the blog: https://devblogs.microsoft.com/nuget/add-a-readme-to-your-nuget-package/#add-a-readme-in-your-project-file-recommended

The value of PackageReadMeFile property needs to match the PackagePath attribute equivalent. It's about the value in the package, not the value in the project directory.

donnie-msft commented 2 years ago

@donnie-msft There's a video by the OP.

In that video, the OP hits the same isssue, but clicks Reload projects.

Can you try the same thing?

@nkolev92 yes, I already tried it which formed the basis of my question to you.

tonygiang commented 2 years ago

If that's the generated XML by the project system, I'm not surprised it doesn't work.

The XML in the video was not generated by the project system because I manually modified the PackageReadmeFile field, though the auto-generated XML yields the same result, that is successful pack for single-target, error for multi-target. Putting the readme file at project root also yields the same result.

nkolev92 commented 2 years ago

@donnie-msft

Did you click reload projects? After I click that, I can actually pack.

@tonygiang

The PackageReadmeFile modification is incorrect. The property points inside the package, the item is supposed to point to the file location.

That being said, I can repro your problem with the TargetFrameworks field.

In particular:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFrameworks>net6.0</TargetFrameworks>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PackageReadmeFile>readme.md</PackageReadmeFile>
  </PropertyGroup>

  <ItemGroup>
    <None Update="readme.md">
      <Pack>True</Pack>
      <PackagePath>\</PackagePath>
    </None>
  </ItemGroup>

</Project>

Fails consistently.

tonygiang commented 2 years ago

I purposefully modified PackageReadmeFile that way to have a proof positive that any value in this field is not the cause of this bug, as demonstrated in the video: single-target packing is successful.

erdembayar commented 2 years ago

@nkolev92 What is the OP video here? If it's actual problem, then I can take it for next sprint.

nkolev92 commented 2 years ago

From the issue body:

A demonstrative video of reproducing this bug can also be found here: https://cdn.discordapp.com/attachments/598678594750775301/924001576643878923/2021-12-25_00-57-31.mp4

Seems like a real bug, but unless you are going to work on it in a quality week, given that it's a P1 please wait for the sprint planning before assigning.

erdembayar commented 2 years ago

@nkolev92 Do you have rough estimate for costing? If it's 2 or less than I can take for quality week.

nkolev92 commented 2 years ago

I don't really have a good estimate because I believe it has something to do with default content includes so the fix might not be NuGet side.

heng-liu commented 2 years ago

Hi @tonygiang , I could repro with the following project file:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFrameworks>net5.0</TargetFrameworks>
      <PackageReadmeFile>README.md</PackageReadmeFile>
  </PropertyGroup>

  <ItemGroup>
    <None Update="docs\README.md">
      <Pack>True</Pack>
      <PackagePath>\</PackagePath>
    </None>
  </ItemGroup>

</Project>

The error in the error list is: NU5039 The readme file 'README.md' does not exist in the package.

But when I change the line from <None Update="docs\README.md"> to <None Include="docs\README.md">, the pack is successful. Could you help check if changing "Update" to "Include" works for you?

tonygiang commented 2 years ago

I can confirm that changing "Update" to "Include" works.

nkolev92 commented 2 years ago

@heng-liu

There's a few different things going wrong here, which is why I mentioned the content includes are likely a problem.

The project system generates a file with the Update element and it doesn't work when you multi target.

In particular: 

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PackageReadmeFile>readme.md</PackageReadmeFile>
  </PropertyGroup>

  <ItemGroup>
    <None Update="readme.md">
      <Pack>True</Pack>
      <PackagePath>\</PackagePath>
    </None>
  </ItemGroup>

</Project>

works, but 

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFrameworks>net6.0</TargetFrameworks>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PackageReadmeFile>readme.md</PackageReadmeFile>
  </PropertyGroup>

  <ItemGroup>
    <None Update="readme.md">
      <Pack>True</Pack>
      <PackagePath>\</PackagePath>
    </None>
  </ItemGroup>

</Project>

Make sure you clear the obj/bin folder between the TargetFramework to TargetFrameworks change.

heng-liu commented 2 years ago

I was unable to repro the first scenario (TargetFramework + Update=> successful pack), which is shown in the video at the beginning part. When I tried to repro, it failed the pack for the same reason..Even though I unload the project and clear the obj/bin folder. May I know how to repro for this scenario? @tonygiang Thanks!

heng-liu commented 2 years ago

BTW,I found an issue https://github.com/dotnet/project-system/issues/7642. The issue is, when adding readme through package properties UI, when the file is at the project's directory or lower in the directory structure, the project file is changed incorrectly. One comment explains it causes a None item into an Update item. I'm not sure if changing it into Update is expected, or it is a bug? @nkolev92

tonygiang commented 2 years ago

I always build once before packing by habit. Perhaps that was the missing step?

nkolev92 commented 2 years ago

@heng-liu

Try:

PackFailure.zip

Extract it.

Run msbuild /restore /t:pack which will automatically build.

AFter that succeeds. Edit the csproj, run git clean -xdf and run msbuild /restore /t:pack and it should fail.

I can repro it locally, I'd be happy to help you repro offline.

nkolev92 commented 2 years ago

@heng-liu

The project system side sounds like it could be a problem, but I'm not 100% sure what the fix should be yet, or whether a fix for that is needed.

Understanding why TargetFramework vs TargetFrameworks behaves differently might help us understand what the overall root cause is.

heng-liu commented 2 years ago

Thanks @nkolev92 ! I'm now able to repro with the info you provided :) I'll see why TargetFramework vs TargetFrameworks behaves differently.

heng-liu commented 2 years ago

The root cause is this line has different behavior for TargetFramework vs TargetFrameworks. When it's TargetFramework : image

When it's TargetFrameworks : image

I then compared the msbuild.binlog between the two: When it's TargetFramework : image

When it's TargetFrameworks : image

heng-liu commented 2 years ago

Hi @nkolev92 , do you have any idea if this is an issue in NuGet?

nkolev92 commented 2 years ago

Not sure, probably no, but we might need to implement a workaround potentially depending on the findings.

That was the extent of my analysis when I reproed it, looked at the binlog and observed that difference.

It's a NuGet scenario that's broken, so understand where the issue is coming from is the only way to determine what's happening. I think the next step is to see where those default imports are happening and why _GetPackageFiles has a different outcome.

Note that _GetPackageFiles is defined on our end: https://github.com/NuGet/NuGet.Client/blob/fd00d6dd248ed74c2fe006bbd4b8ca53d8ebdeac/src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.targets#L490-L539

You can probably minimize the difference with something like: 

  <Target Name="GetNoneItems">
    <ItemGroup>
      <_PackageFiles Include="@(None)" Condition=" %(None.Pack) == 'true' ">
        <BuildAction Condition="'%(None.BuildAction)' == ''">None</BuildAction>
      </_PackageFiles>
    </ItemGroup>
    <Message Text="@(_PackageFiles)" Importance="High" />
  </Target>

Then you can call that target and observe the difference.

I'd also check if having multiple target frameworks changes anything.

nkolev92 commented 2 years ago

Actually, I think I see the problem.

It is possible that _GetPackageFiles is only running in the inner build (with TargetFramework set, outer build is when TargetFramework is not set).

See here: https://github.com/NuGet/NuGet.Client/blob/fd00d6dd248ed74c2fe006bbd4b8ca53d8ebdeac/src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.targets#L490

I'd expect that specifying multiple target frameworks causes it to fail.

https://github.com/NuGet/NuGet.Client/blob/fd00d6dd248ed74c2fe006bbd4b8ca53d8ebdeac/src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets#L616-L633 is an example of a target that runs in the inner build.

Now, it's important to consider whether this would break/complicate other scenarios, but it feels likely that we'd need to make _GetPackageFiles run an inner build.

nkolev92 commented 2 years ago

This issue is essentially the same as https://github.com/NuGet/Home/issues/6792, which basically says conditionally included content is not considered.

heng-liu commented 2 years ago

I tried to get the binlog of TargetFrameworks+ Include, and found the _GetPackageFiles runs successfully (the pack was also successful).

Is it possible that the problem is None item is empty for TargetFrameworks + Update scenario? Here is the comparison of TargetFrameworks+ Include(left + details in middle) and TargetFrameworks+ Update(right) image

Here are the binlogs for the three scenarios: binlogs.zip

nkolev92 commented 2 years ago

@heng-liu

When you declare an include in the csproj, you're basically declaring it for both inner and outer builds unless you condition it. The automatic includes are expanded as part of the inner build only, which is why it fails.

Outer build = not framework specific. Inner build = TargetFramework property is set to one of the many options in TargetFrameworks. So if anything is done conditionally, it'll only be available in the inner build.

tonygiang commented 2 years ago

I noticed that this issue keeps getting added and removed from Sprint milestones, and currently removed. What is its status as of now? Is it planned to be fixed at all?

aortiz-msft commented 2 years ago

Hey @tonygiang, yes, we are still planning on fixing this issue. However, we are lowering the priority since there's an easy workaround for this particular symptom of the root problem. We currently don't have a good understanding of when we will be able to get to it.

vRune4 commented 11 months ago

BTW,I found an issue dotnet/project-system#7642. The issue is, when adding readme through package properties UI, when the file is at the project's directory or lower in the directory structure, the project file is changed incorrectly. One comment explains it causes a None item into an Update item. I'm not sure if changing it into Update is expected, or it is a bug? @nkolev92

I stumbled across this comment: https://github.com/NuGet/Home/issues/11292#issuecomment-1317357876

After changing the Update= to an Include= my build runs.