dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.23k stars 1.35k forks source link

[Bug]: No idiomatic method to prevent content files from referenced projects overwriting files in main project #10573

Closed nbarnwell closed 1 month ago

nbarnwell commented 2 months ago

Issue Description

I have read about ErrorOnDuplicatePublishOutputFiles but that only works on Publish (i.e. not build), and isn't what I want. I want to be able to specify that a file should not be brought in from the referenced project at all. I have tried adding PrivateAssets=none to the ProjectReference in the csproj file, to no avail.

Steps to Reproduce

  1. Clone https://github.com/nbarnwell/ConflictingContentFilesTest
  2. Build in Visual Studio

Expected Behavior

The appsettings file in the output folder should be the one from the main project.

Actual Behavior

The appsettings file in the output folder is the one from the ExternalLibrary project.

Analysis

I believe this is a change in behaviour of project references from .NET Framework? I understand bringing in content files from packages, but not projects. With a package, you have deliberately packaged it to contain certain files, but that step is implicit in project references, so you have no control over what is brought in. There is some setting somewhere about not bringing in content files from transitive project references, but that's not what I'm looking for, either.

Versions & Configurations

MSBuild version 17.10.4+10fbfbf2e for .NET Framework 17.10.4.21802

Microsoft Visual Studio Professional 2022 (64-bit) - Current Version 17.10.4

KalleOlaviNiemitalo commented 2 months ago

If the application project has its own appsettings.json file, then '$(CopyConflictingTransitiveContent)' == 'false' from https://github.com/dotnet/msbuild/pull/4931 should already prevent the appsettings.json of the referenced project from being copied. I wonder why that's not working.

nbarnwell commented 2 months ago

I have asked that, but apparently "transitive" content is when a project references a project that itself references another project? So for ProjectA->ProjectB->ProjectC, ProjectC's content files wouldn't end up in ProjectA's build output. That's not the problem I'm having, though. This is direct project reference's content files, not transitive ones. Although, I've read a lot of similar-but-not-the-same issues now and I'm starting to get confused with the various responses, hence the desire for the MS-approved "idiomatic" way to stop this happening.

nbarnwell commented 2 months ago

Out of interest, I asked Github Copilot:

How do I prevent content files from a referenced project from overwriting files with the same name in the referencing project?

And it offered me something that I edited to:

  <ItemGroup>
    <!-- Exclude specific content files from being copied -->
    <Content Remove="ExternalLibrary\appsettings.json" />
  </ItemGroup>

  <Target Name="CopyReferencedContent" AfterTargets="Build">
    <!-- Manually copy content files if needed, with conditions -->
    <Copy SourceFiles="appsettings.json" DestinationFolder="$(OutputPath)" SkipUnchangedFiles="true" />
  </Target>

That does seem to work in my sample repro project. This can't be the idiomatic solution to this, though? Surely?

KalleOlaviNiemitalo commented 2 months ago

<Content Remove="ExternalLibrary\appsettings.json" /> looks unnecessary in the application project.

nbarnwell commented 2 months ago

<Content Remove="ExternalLibrary\appsettings.json" /> looks unnecessary in the application project.

I agree, but it's AI, so it's to be taken with a pinch of salt. This is another attempt which appears to work so far (though I've had varying results with various solutions so I don't quite trust it enough to just be a "drop-in" when this happens in future):

  <ItemGroup>
    <Content Update="appsettings.json">
      <ExcludeFromSingleFile>true</ExcludeFromSingleFile>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
      <CopyToPublishDirectory>PreserveNewest</CopyToPublishDirectory>
    </Content>
    <None Remove="appsettings.json" />
    <Content Include="appsettings.json">
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </Content>
  </ItemGroup>
KalleOlaviNiemitalo commented 2 months ago
  1. Build in Visual Studio

Do you get the same results if you build the solution with dotnet build?

KalleOlaviNiemitalo commented 2 months ago

Re CopyConflictingTransitiveContent -- despite "Transitive" in the name, it should work in your scenario (except it will fool the Fast Up-to-date Check). I suppose the content is transitive in the sense that there is a reference from the application project to the library project, and a reference from the library project to the content file.

Are you using Visual Studio build acceleration? That doesn't seem to respect the CopyConflictingTransitiveContent logic.

KalleOlaviNiemitalo commented 2 months ago

This might be a duplicate of https://github.com/dotnet/project-system/issues/9001. If so, the fix https://github.com/dotnet/project-system/pull/9454 is included in Visual Studio 2022 version 17.11.1. AFAICT, it was not backported to LTSC 17.10 versions.

nbarnwell commented 2 months ago

This might be a duplicate of dotnet/project-system#9001. If so, the fix dotnet/project-system#9454 is included in Visual Studio 2022 version 17.11.1. AFAICT, it was not backported to LTSC 17.10 versions.

That's interesting - my Visual Studio did an update only yesterday and is now on that version. I'll do some more tests.

nbarnwell commented 2 months ago

This might be a duplicate of dotnet/project-system#9001. If so, the fix dotnet/project-system#9454 is included in Visual Studio 2022 version 17.11.1. AFAICT, it was not backported to LTSC 17.10 versions.

Well, I've done several builds on VS 17.11.1 on both the original project that I couldn't share, and the example repro project, and with basic csproj values (i.e. no special targets or properties beyond what VS puts in when you set a file as Content etc), I seem to get the correct appsettings.json file now.

So it seems like my issue might possibly be fixed, though it'll take a little while to earn my trust. :)

Last question - does your understanding of the fix rule out similar issues on AzDO build agents? I'm asking because it sounds like the fix was for the VS-specific build acceleration and therefore msbuild (i.e. dotnet build) was never affected?

Thanks for your help with this. I'd been losing my mind trying to make sense of all the seemingly related github issues. :S

KalleOlaviNiemitalo commented 2 months ago

IIRC, if you set a more verbose logging level under "SDK-Style Projects" / "Up to Date Checks" in the options of Visual Studio 2022 version 17.11.1 and build the project, then the Output window will report that it disabled build acceleration because of conflicting files.

For lower versions of Visual Studio, you can set <AccelerateBuildsInVisualStudio>false</AccelerateBuildsInVisualStudio> in the source and it should have the same effect.

I don't know for sure whether AzDO build agents can use build acceleration.

AR-May commented 1 month ago

Team triage: @GangWang01 could you please try to reproduce and investigate the issue?

GangWang01 commented 1 month ago

With the provided steps to reproduce and VS version I could not reproduce. The appsettings.json file in the output folder was from the main project, not the referenced project. image

https://github.com/dotnet/msbuild/pull/4931 works well and gets the referenced project's appsettings.json removed from the list of items copied to output folder. image

KalleOlaviNiemitalo commented 1 month ago

@GangWang01 IIRC the issue would occur on Visual Studio 2022 version 17.10.* if you enable build acceleration in VS options and build the solution twice. On the second build it would not use MSBuild and would just do the build-acceleration file copy and mess it up.

GangWang01 commented 1 month ago

@KalleOlaviNiemitalo Build acceleration did copy the appsettings.json file from referenced project to the output folder of main project when doing the second build with any change to the appsettings.json file from referenced project. It doesn't have the logic like CopyConflictingTransitiveContent.

GangWang01 commented 1 month ago

This might be a duplicate of dotnet/project-system#9001. If so, the fix dotnet/project-system#9454 is included in Visual Studio 2022 version 17.11.1. AFAICT, it was not backported to LTSC 17.10 versions.

As @KalleOlaviNiemitalo pointed out, it's duplicate with the issue of project system.

GangWang01 commented 1 month ago

Closed as it's an issue of project system and it should be fixed in 17.11 according to the milestone in the pull request https://github.com/dotnet/project-system/pull/9454.