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

moving the <None> items into targets to force correct ordering #97

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SimaTian
Copy link

@SimaTian SimaTian commented Jan 9, 2025

Follow up for Order of projects in solution file affects build results
Original reproduction here

The blog here mentions how to include generated file. This case was a bit specific and required slightly different setup because the wasn't enough to enforce correct ordering.

Basically the issue is that if the item isn't within a target, it is resolved during Evaluation phase - e.g. as soon as the project file is loaded into the MSBuild, but ProjectReference that is setting up the project dependency ordering is a Target so this happens:

  • MSBuild first loads a project (including evaluation) as specified by the .sln file
    • this is affected by the original project ordering change that resulted in the build failing
  • Then it evaluates the targets related to project references, see here.
    • specifically ResolveProjectReferences target
  • this builds the referenced projects - however the item is already processed by this point, resulting in an empty copy

By moving the item into a custom target and forcing order by setting AfterTargets, the files are moved correctly. Tests pass.

Interestingly enough, since the files are not processed further, another(albeit rather weird) way to have a working build without using the target is to run the build twice. This both made my reproduction attempts somewhat unwieldy and hinted me to a correct solution.

Kudos for Jan Krivanek for letting me know about the MSBuildism blog and to Jenny Bai for her work on reproducing the issue and collecting the logs.

@chtenb
Copy link
Member

chtenb commented Jan 15, 2025

@SimaTian Thanks for looking into this. If I understand your explanation correctly, the issue is that by default, plain <ItemGroup> entries are resolved before things like build and project references are resolved. I can see why that is a problem here. I understand how putting it in a target may resolve this issue, as you demonstrated in this PR.

I'm currently working on a linux port of this library, and I tried to apply your idea to this branch as well (the linux branch). For various reasons, I needed to make these None definition more complex here, so it looks like this:

<Target Name="RunAfterBuildIsDone" AfterTargets="ResolveProjectReferences">
  <ItemGroup>
    <!-- Include all files in the Resources folder but copy them directly to the output directory -->
    <None Update="Resources/*">
      <CopyToOutputDirectory>Always</CopyToOutputDirectory>
      <Link>%(RecursiveDir)/../%(Filename)%(Extension)</Link>
      <Pack>true</Pack>
      <PackageCopyToOutput>true</PackageCopyToOutput>
    </None>
    <!-- The graphviz subfolder is only present and relevant on linux --> 
    <None Update="Resources/graphviz/*">
      <CopyToOutputDirectory>Always</CopyToOutputDirectory>
      <Link>%(RecursiveDir)/../../graphviz/%(Filename)%(Extension)</Link>
      <Pack>true</Pack>
      <PackageCopyToOutput>true</PackageCopyToOutput>
    </None>
  </ItemGroup>
</Target>

However, after putting these in a target per your suggestion, these commands start misbehaving. Among other things, they now emit the following warnings:

/usr/lib/dotnet/sdk/8.0.111/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(221,5): warning NU5119: File '/home/chiel/prj/Graphviz.NetWrapper/Rubjerg.Graphviz/.editorconfig' was not added to the package. Files and folders starting with '.' or ending with '.nupkg' are excluded by default. To include this file, use -NoDefaultExcludes from the commandline [/home/chiel/prj/Graphviz.NetWrapper/Rubjerg.Graphviz/Rubjerg.Graphviz.csproj]
/usr/lib/dotnet/sdk/8.0.111/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(221,5): warning NU5119: File '/home/chiel/prj/Graphviz.NetWrapper/Rubjerg.Graphviz/.editorconfig' was not added to the package. Files and folders starting with '.' or ending with '.nupkg' are excluded by default. To include this file, use -NoDefaultExcludes from the commandline [/home/chiel/prj/Graphviz.NetWrapper/Rubjerg.Graphviz/Rubjerg.Graphviz.csproj]
/usr/lib/dotnet/sdk/8.0.111/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(221,5): warning NU5119: File '/home/chiel/prj/Graphviz.NetWrapper/Rubjerg.Graphviz/.gitignore' was not added to the package. Files and folders starting with '.' or ending with '.nupkg' are excluded by default. To include this file, use -NoDefaultExcludes from the
commandline [/home/chiel/prj/Graphviz.NetWrapper/Rubjerg.Graphviz/Rubjerg.Graphviz.csproj]
/usr/lib/dotnet/sdk/8.0.111/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(221,5): warning NU5119: File '/home/chiel/prj/Graphviz.NetWrapper/Rubjerg.Graphviz/.gitignore' was not added to the package. Files and folders starting with '.' or ending with '.nupkg' are excluded by default. To include this file, use -NoDefaultExcludes from the
commandline [/home/chiel/prj/Graphviz.NetWrapper/Rubjerg.Graphviz/Rubjerg.Graphviz.csproj]

I'm extremely confused as to how putting the items in a target results in these warnings. I looks like the <None> definitions no longer just apply to files in the Resources subfolder?

@SimaTian
Copy link
Author

SimaTian commented Mar 3, 2025

Hello @chtenb, I'm sorry I completely missed your response, I wasn't monitoring this issue, I just checked since I was cleaning my workspace and chanced upon the folder I used for the reproduction.
I'll try to set up some better way of reaching me for the future cases similar to this one & monitor this one closely meanwhile.

Did you have any luck since you encountered the issue? I'll add this on my todo list and check later this week. I'm sorry for the delay.

@chtenb
Copy link
Member

chtenb commented Mar 3, 2025

@SimaTian Hi again.

I'm still having the same issue on the linux branch, I have not been able to find out why your fix doesn't work with the more complex copy definition.

RE issue monitoring: activity in threads I've participated in always show up in my github notifications by default. I think you can also setup email notifications if you don't look at github regularly.

@SimaTian
Copy link
Author

SimaTian commented Mar 3, 2025

My github notifications are completely overrun with dotnet stuff so I'm agressively filtering for those from msbuild repository only. (I'm not yet used to the world where I occasionally contribute to repositories that aren't mine)
I will add "add non-dotnet repositories to whitelist" to my todo list.

@SimaTian
Copy link
Author

@chtenb
finally got the time to look into it a bit more
I think the warning you see is the result of Link. What are you trying to achieve with it please?
I found two interpretations and based on behavior I consider this one to be correct. (The other one being this which I'm not sure about how to parse. Sadly it's the one from official documentation.)

One detail I'm unsure about after some testing:
The RecursiveDir is working with the '**' wildcard in an ItemName which you're not using so it probably evaluates to and empty string.

If you're trying to copy files to a directory, could you try going this route instead?
https://stackoverflow.com/questions/51924129/copy-files-from-nuget-package-to-output-directory-with-msbuild-in-csproj-and-do
If that is what you're trying to do. If not, please correct me.

Doing something like:

 <Target Name="RunAfterBuildIsDone" AfterTargets="ResolveProjectReferences">
   <ItemGroup>
        <FilesToCopy Include="./Resources/*" />
    </ItemGroup>
    <Copy SourceFiles="@(FilesToCopy)" DestinationFolder="$(TargetDir)\%(RecursiveDir)" SkipUnchangedFiles="true" />

</Target>

Appeared to be working, though it's most likely not doing exactly what you're trying to do.
I'm sorry I'm not super familiar with MSBuild yet so my advice is somewhat limited.

@chtenb
Copy link
Member

chtenb commented Mar 13, 2025

As you have seen on the linux branch, we have this command in Rubjerg.Graphviz.csproj, which is intended to copy the contents (files and folders) of the Resources/ folder to the output directory. Hence the .., to escape the "Resources" folder itself.

    <None Update="Resources/*">
      <CopyToOutputDirectory>Always</CopyToOutputDirectory>
      <Link>%(RecursiveDir)/../%(Filename)%(Extension)</Link>
      <Pack>true</Pack>
      <PackageCopyToOutput>true</PackageCopyToOutput>
    </None>

Now this seems to work fine, except when I move this into a Target in the manner that your suggested as a solution for forcing the correct build step order.

I'm not hung up on this exact solution, but how would I replicate this behavior within a Target per your suggestion? So copying the contents of Resources/ to the output directory.

@SimaTian
Copy link
Author

SimaTian commented Mar 14, 2025

There are several things at play here, and I would have to dig deeper than I can right now. I'll leave some suggestions here hoping that it will work. If yes, great. If no, I will check later when I have more time.

  • the <Link> metadatum is doing something weird and I think it probably does something different than what you want. Check the output folder structure - something weird is happening there given your current link. (Not to mention that the way I understood the documentation I was convinced that Link is a cosmetic only utility for VS. Well, apparently it isn't.)

  • the warning you see means that somehow the way your current copy is structured, it tries to copy everything in the Resource parent folder and it's complaining about the .xxx files while creating the nuget package. (also you can see them copied into a folder Debug/../graphviz which is a location where you probably don't want them)

  • that being said, it only happens when it is within a target so it might be bug in MSBuild targets. I will have to look closer at that.

However from what I could find and dig into, the CopyToOutputDirectory is handled by some MSBuild target, not in the MSBuild engine itself - and it is just using the <Copy> task under the hood.
So maybe you can try the <Copy> stuff from my previous comment to copy the files you want manually (using wildcards of course)

After that there is the teeeny issue with packing where I'm unsure how does it interact with copy task.
I would first get the copy to work properly, then start worying about packing. There ought to be a way to make it work, though it might be iffy.

@chtenb
Copy link
Member

chtenb commented Mar 14, 2025

Thanks for your response. I've tried to make it work with Copy in the past, but it always failed to register transitively. So referencing projects wouldn't get the resources in their output. I don't remember if it worked with the nuget packaging process, maybe there were issues with that as well like you say. The <None>/<Link> solution is something that is suggested on several blogs on the internet, but I'm not sure what the official solution would be. I haven't found any microsoft resources on this, even though it feels like it would be a pretty mundane issue that many people would encounter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants