project-system icon indicating copy to clipboard operation
project-system copied to clipboard

Consuming projects are not updated when a CopyToOutputDirectory item is updated in a project with no project references.

Open PathogenDavid opened this issue 5 years ago • 13 comments

Visual Studio Version: 2017 - 15.9.10

Also reproduced in 2019 RC.4 - 16.0.0

Summary:

Fast up-to-date checks will consider a project to be up-to-date even when a project it references is not up to date due to a stale CopyToOutputDirectory item.

(I've found some workarounds which are described here.)

Steps to Reproduce:

  1. Clone this repository.
    • The demonstration repository contains details on how the projects are set up. The most important thing to know is that TextFileA.txt and TextFileB.txt are copied to the output directory with PreserveNewest and the program prints their contents when it runs.
    • The repository also contains more details on my findings about the source of the issue.
    • The steps from the repository for reproducing the problem are reproduced below for posterity.
  2. Open the solution in Visual Studio.
  3. Select TestProgram as the startup project.
  4. Press F5 to start the program. The output is as follows:
    Hello World!
    Hello from ClassA!
    Text file A reporting for duty!
    Hello from ClassB!
    Text file B reporting for duty!
    Done.
    
  5. Close the program and add a message TextFileA.txt and press F5 to start the program again. The output includes your message:
    Hello World!
    Hello from ClassA!
    Text file A reporting for duty! Hello, world!
    Hello from ClassB!
    Text file B reporting for duty!
    Done.
    
  6. Close the program and add a message to TextFileB.txt instead, press F5 to start the program again. The output will not include your message:
    Hello World!
    Hello from ClassA!
    Text file A reporting for duty! Hello, world!
    Hello from ClassB!
    Text file B reporting for duty!
    Done.
    
  7. Observe that the file is updated in LibraryB's output (LibraryB\bin\Debug\netstandard2.0\TextFileB.txt), but not in TestProgram's output (TestProgram\bin\Debug\netcoreapp2.2\TextFileB.txt).

Expected Behavior:

TextFileB.txt is updated in TestProgram's output folder when it is changed.

Actual Behavior:

TextFileB.txt is not updated.

User Impact:

Ironically I actually found this bug while debugging a similar issue with a legacy-style C# project. Either way, the symptoms are the same even though it appears the cause is not.

This issue is very confusing because you can see that LibraryB was built by Visual Studiom, but projects depending on it directly/indirectly were not. The copied file will be present in the output of LibraryB, but not in TestProgram. You can rebuild the solution or make dummy changes in TestProgram to force things to update, but this is fairly annoying to remember to do. In our case, the copied file was a native library, so it was like changes to the native library weren't working.

Workarounds:

Besides rebuilding, I've found two workarounds to this issue:

  1. Add a dummy reference to the library.
  2. Create the copy marker yourself using MSBuild.

Both are described in further detail in my demonstration repository.

PathogenDavid avatar Mar 28 '19 02:03 PathogenDavid

Some extra thoughts that I didn't want to wedge in above:

I wasn't sure whether or not to report this here or in the MSBuild repo. It's not clear if MSBuild should be creating the CopyComplete files all the time or if the fast up-to-date check needs to be more robust in regards to files copied to the output.

It's probably worth noting that this issue also happens with legacy .NET projects, except the actual cause seems to be different. The files won't be copied even if the CopyComplete file is updated. I suspect the legacy fast up-to-date checker doesn't check the CopyComplete files, but I'm not aware of way to get debug output from it. I'm working to get the project in question updated to the modern project format anyway, so I probably won't pursue that rabbit hole. (I made my demonstration before I realized that modern projects and legacy projects used different logic for fast up-to-date.)

PathogenDavid avatar Mar 28 '19 02:03 PathogenDavid

Please also test: https://github.com/dotnet/project-system/issues/3637 when fixing this.

davkean avatar Jan 27 '20 23:01 davkean

@davkean the issue #3637 is not fixed. I already tested it.

ocallesp avatar Jan 29 '20 19:01 ocallesp

@davkean the issue #3637 is not fixed. I already tested it.

That issue looks like a duplicate of this one. Dave means you should verify the scenario in that issue when you evaluating a fix to this issue.

@rainersigwald I wonder how MSBuild handles this case. Does a project walk its project reference graph to find all PreserveNewest items?

drewnoakes avatar Jan 30 '20 04:01 drewnoakes

It's actually quite complicated! The longstanding default behavior depends on how the build was invoked (https://github.com/microsoft/msbuild/issues/1054). We recently (https://github.com/microsoft/msbuild/pull/4865) added some flags to explicitly specify recursive or non-recursive behavior.

rainersigwald avatar Jan 30 '20 18:01 rainersigwald

Does this mean that this is fixed, but only if we opt into the new behavior?

davkean avatar Jan 30 '20 22:01 davkean

Okay, we sat down and looked at this. I can see what's going on; we're only factoring in CopyToOutputDirectory for items that come the same project's evaluation, which doesn't include these implicitly included items from other projects. We need to call GetCopyToOutputDirectoryItems, which returns a set of items representing all the items that need to be copied, it has two metadata items:

CopyToOutputDirectory [Enum] -> Never, PreserveNewest, Always TargetPath -> Destination

Then we can stop inspecting the copy to output directory metadata on evaluation items, as it will be superceded by above.

davkean avatar Feb 06 '20 04:02 davkean

Possible fix https://github.com/ocallesp/project-system/commit/d639aaa47df6134f48d0c818df761963aab7fc0c Pending to add unit test

ocallesp avatar Feb 13 '20 21:02 ocallesp

We had to revert the fix as it caused a regression.

davkean avatar Feb 27 '20 22:02 davkean

Revert commits https://github.com/dotnet/project-system/pull/5923

ocallesp avatar Mar 17 '20 18:03 ocallesp

This bug is blocked by the msbuild bug 4923

ocallesp avatar Mar 27 '20 16:03 ocallesp

I had hoped to fix this with #7932. That PR used existing MSBuild targets to discover the set of items that would need to be copied during build, transitively across the project graph.

The approach had two problems:

  1. MSBuild gathers this information by calling in to referenced projects, transitively. Across all projects in a solution, this can be quite a large graph. All these calls down the graph introduce a lot of overhead, which makes the change infeasible.
  2. When a project changes, its design-time build runs. Projects that transitively reference that project do NOT run design-time builds. As a result, the referencing project will be out of date if the referenced project's copy items change.

Either of those problems would make that approach infeasible.

We need a way to efficiently and correctly compute CopyToOutputDirectoryItems, transitively across project references. Calling through today's MSBuild targets will not work.

One potential design would be for each project to offer its direct copy items to the project system, and have some means for this to be aggregated across the solution. That project reference graph can be walked when needed to identify items.

Challenges:

  • Making it work across all project systems.
  • Selecting the target framework correctly when a referenced project has multiple targets. @rainersigwald suggested this information may be available already.

drewnoakes avatar Mar 29 '22 03:03 drewnoakes

@drewnoakes We cheated for transitive project references and put them in the assets file. I would consider a similar approach for this.

davkean avatar Mar 29 '22 04:03 davkean

An update on this issue. I have another solution under test here that fixes the issue and doesn't have the issues of the previous attempts. My only concern is that it may cause a lot more file system checks in large solutions with deep P2P reference graphs, which may regress performance. I'm doing more testing in this area.

drewnoakes avatar Nov 28 '22 11:11 drewnoakes