Allow Custom CopyToOutputDirectory Location With TargetPath #6237

benvillalobos · 2021-03-10T01:54:23Z

Fixes #2795
and indirectly fixes https://developercommunity.visualstudio.com/t/copytooutputdirectorypreservenewest-ignored-inside/1332219?from=email&viewtype=all#T-ND1363347

Context

There's currently no way to include items in a project such that:

Visual studio sees them in a specific folder (via <Link>).
They are published to a user-defined path (currently controlled via <Link>)

Changes Made

Modify the AssignTargetPath task to return early if TargetPath metadata is already set on a particular item.

Testing

Need to add one test covering this.
Tested locally with bootstrapped MSBuild on command line
Tested locally with a boostrapped msbuild on internal VS

Here's the repro I'm using:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>netstandard2.0</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <Content Include="Files\**">
      <Link>Files\%(Filename)%(Extension)</Link>
      <TargetPath>%(Filename)%(Extension)</TargetPath>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
    </Content>
  </ItemGroup>
</Project>

Notes

The other way of solving this problem has to do with Microsoft.Common.CurrentVersion.targets. We modify it so that the AssignTargetPath task look something like this:

    <AssignTargetPath Files="@(Content)" RootFolder="$(MSBuildProjectDirectory)" Condition="'%(Content.TargetPath)'==''">
      <Output TaskParameter="AssignedFiles" ItemName="ContentWithTargetPath" />
    </AssignTargetPath>
    <ItemGroup>
        <ContentWithTargetPath Include="@(Content)" Condition="'%(Content.TargetPath)'!=''"/>
    </ItemGroup>

This seems less efficient to me. AssignTargetPath is also called for all None, Content, and EmbeddedResource files. So if we go this batching route and want None or EmbeddedResource to have this feature, we'd need to batch those as well.

cdmihai · 2021-03-13T17:16:02Z

src/Tasks/AssignTargetPath.cs

+                    // https://github.com/dotnet/msbuild/issues/2795
+                    if (!string.IsNullOrEmpty(targetPath))
+                    {
+                        AssignedFiles[i].SetMetadata(ItemMetadataNames.targetPath, EscapingUtilities.Escape(targetPath));


nit: Instead of the continue and duplication you could let it fall through. First try TargetPath. If null or empty try Link. If null or empty run that if block. Then set the escaped value and return.

cdmihai · 2021-03-13T17:18:29Z

src/Tasks.UnitTests/AssignTargetPath_Tests.cs


-            Assert.True(success);
+        [Theory]
+        [InlineData("test/output/file.txt")]


Also add a fully qualified path to sanity ensure the task preserves those.

cdmihai

Even though this seems the better way of doing it, I assume there might be people whose builds might start copying files to different places. Add an escape hatch / change wave . On the other hand if they take the time to investigate and learn about the escape hatch, they might as well fix it :)

src/Tasks.UnitTests/AssignTargetPath_Tests.cs

Forgind · 2021-03-15T15:35:42Z

src/Tasks/AssignTargetPath.cs

+                    // https://github.com/dotnet/msbuild/issues/2795
+                    if (!string.IsNullOrEmpty(targetPath))
+                    {
+                        AssignedFiles[i].SetMetadata(ItemMetadataNames.targetPath, EscapingUtilities.Escape(targetPath));


Consider using some new variable ("targetPathOverride"? "targetPath2"? Choose a good name) to allow users to provide a custom targetPath while maintaining that this task can be called twice without adverse consequences. (Escaping seems necessary as part of what TaskItem requires for all its metadata.)

Going with TargetPathOverride for clarity.

…code

benvillalobos · 2021-03-15T22:15:44Z

@cdmihai No longer needed as discussed in the PR review. Users now set TargetPathOverride instead of TargetPath, which may have been previously set for various reasons. No change wave needed because this is explicitly opt-in now.

benvillalobos · 2021-03-19T18:33:10Z

@dsplaisted before we get this merged, could I have your take on this? /cc: @KathleenDollard

To quickly summarize what the goal is:

Have an item show in VS under a specific folder
Output that item to a folder that differs from how it shows in VS.
Output the item whenever the item itself is updated.

This change simply allows a TargetPathOverride metadata to take priority over using Link in setting the TargetPath.

cdmihai · 2021-03-19T20:47:56Z

@benvillalobos In addition to your goals, I would also add:

have a simple, clear way of specifying the output path for a single Content item.

benvillalobos · 2021-03-22T19:46:02Z

friendly reminder. @dsplaisted

benvillalobos · 2021-03-29T19:27:14Z

@dsplaisted I'm not sure how this change could impact the rest of the build. Do TargetPath and Link need to stay in sync for any reason?

dsplaisted · 2021-03-31T02:39:28Z

I'm pretty sure my worries about this were unfounded. What I was thinking of is specifying Link metadata to something inside the project folder in order to display it in the Visual Studio Solution Explorer in a different place than the physical path. There would be problems with doing that, I believe. But just changing the folder where it's copied to in the output is fine, I believe.

TargetPath is a way better name for this than TargetPathOverride, and I think TargetPath is already used in some places. Did you go with TargetPathOverride because you think it's less likely to break builds? Could we use TargetPath but put the new behavior behind a quirks / compatibility wave?

benvillalobos · 2021-03-31T16:20:45Z

Did you go with TargetPathOverride because you think it's less likely to break builds?

@dsplaisted That was exclusively the reason. We figured that changing what TargetPath does could impact customers who've expected it to work a certain way when they set TargetPath ahead of time (it will be overridden by link anyway). So the breaking scenario would be customers who have targetpath and link set, but expect the item to output to where Link specifies.

Note that the change doesn't set TargetPathOverride for future use. It allows TargetPathOverride to override what TargetPath is set to.

I'm open to placing this under a changewave if you think it's the better long term solution than having a separate metadata controlling this behavior.

…angewave

benvillalobos · 2021-03-31T16:39:37Z

Pushed up changes that use TargetPath if it's already set (instead of TargetPathOverride), and placed it under the 16.10 change wave. Added it to the changewave doc as well.

Forgind · 2021-03-31T16:53:14Z

Isn't the point of the method to set the TargetPath property if it isn't already set? It doesn't seem helpful to set the TargetPath property if it isn't already set using the TargetPath property, since that's guaranteed to not be set...

benvillalobos · 2021-03-31T17:06:08Z

@Forgind There's no guarantee that TargetPath isn't set when this task runs (no conditions on the targer or tasks), so I think it's reasonable to skip over it if it's already set.

src/Tasks.UnitTests/AssignTargetPath_Tests.cs

dsplaisted · 2021-04-01T02:48:39Z

So the breaking scenario would be customers who have targetpath and link set, but expect the item to output to where Link specifies.

Does the TargetPath metadata do anything today? If it doesn't do anything it's a lot less likely that people would be setting it, right?

…r way around

benvillalobos · 2021-04-01T18:24:41Z

As far as I can tell it only looks to be used when defining where files will be placed, which reinforces that we should be using TargetPath instead of TargetPathOverride.

benvillalobos · 2021-04-05T16:05:59Z

@dsplaisted ping for an approving review or feedback 🙂

TysonMN · 2021-05-27T13:05:36Z

Hello @benvillalobos. Now that version 16.10 has been released, I am able to test. Unfortunately, I think this change exchanged one problem for another.

<Content Include="Files\**">
  <Link>Files\%(Filename)%(Extension)</Link>
  <TargetPath>%(Filename)%(Extension)</TargetPath>
  <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</Content>

I have verified the correctness of the first two lines (i.e. the Link and TargetPath elements). However, the last line with the CopyToOutputDirectory element is not working. After completing the first build, if I tell Visual Studio to build again, then it builds the project instead of correctly saying that it is update to date.

I tested by modifying the MWE I provided in my original VSDC isssue.

benvillalobos · 2021-05-27T17:37:27Z

@TysonMN I created https://github.com/dotnet/msbuild/issues/6496 to track this

benvillalobos added 3 commits March 9, 2021 17:26

Allow manually setting TargetPath on items

aea6d38

Add test ensuring TargetPath takes priority if already set as metadata

1373432

Shouldy-fication

328f029

cdmihai reviewed Mar 13, 2021

View reviewed changes

cdmihai suggested changes Mar 13, 2021

View reviewed changes

KirillOsenkov reviewed Mar 14, 2021

View reviewed changes

src/Tasks.UnitTests/AssignTargetPath_Tests.cs Outdated Show resolved Hide resolved

Forgind reviewed Mar 15, 2021

View reviewed changes

Rename to targetpathoverride

4b3f3eb

Base automatically changed from master to main March 15, 2021 20:09

benvillalobos added 2 commits March 15, 2021 15:10

If targetpathoverride is not set fall back to link. Avoid duplicated …

d795b4e

…code

Add fully qualified path as test case

a6c3245

Forgind approved these changes Mar 15, 2021

View reviewed changes

benvillalobos requested a review from dsplaisted March 19, 2021 18:24

cdmihai approved these changes Mar 19, 2021

View reviewed changes

benvillalobos added the changewave16.10 label Mar 31, 2021

Use TargetPath instead of TargetPathOverride. Placed under a 16.10 ch…

8bc9c3b

…angewave

benvillalobos changed the title ~~Allow Custom CopyToOutputDirectory Location~~ Allow Custom CopyToOutputDirectory Location With TargetPath Mar 31, 2021

Add link to PR in change waves doc.

266ca4d

Reset change waves between tests

4941a4c

benvillalobos added 2 commits March 31, 2021 10:42

Test: Reset state before and after tests

12abb96

unix environment variables are case sensitive..

c874990

dsplaisted reviewed Apr 1, 2021

View reviewed changes

src/Tasks.UnitTests/AssignTargetPath_Tests.cs Outdated Show resolved Hide resolved

PR Feedback: Compare variable against what it should be, not the othe…

663295d

…r way around

KirillOsenkov approved these changes Apr 5, 2021

View reviewed changes

dsplaisted approved these changes Apr 5, 2021

View reviewed changes

benvillalobos added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Apr 5, 2021

Forgind merged commit 7804350 into dotnet:main Apr 5, 2021

benvillalobos mentioned this pull request May 27, 2021

CopyToOutputDirectory Does Not Respect PreserveNewest and TargetPath dotnet/project-system#7276

Closed

elinor-fung mentioned this pull request Jun 8, 2021

Update dotnet SDK to 6.0 Preview 4 dotnet/runtime#53808

Merged

rolfbjarne mentioned this pull request Dec 21, 2021

.NET: What to do about files in ResolvedFileToPublish. xamarin/xamarin-macios#12572

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Custom CopyToOutputDirectory Location With TargetPath #6237

Allow Custom CopyToOutputDirectory Location With TargetPath #6237

benvillalobos commented Mar 10, 2021 •

edited

cdmihai Mar 13, 2021

cdmihai Mar 13, 2021

cdmihai left a comment •

edited

Forgind Mar 15, 2021

benvillalobos Mar 15, 2021

benvillalobos commented Mar 15, 2021

benvillalobos commented Mar 19, 2021

cdmihai commented Mar 19, 2021

benvillalobos commented Mar 22, 2021

benvillalobos commented Mar 29, 2021

dsplaisted commented Mar 31, 2021

benvillalobos commented Mar 31, 2021

benvillalobos commented Mar 31, 2021

Forgind commented Mar 31, 2021

benvillalobos commented Mar 31, 2021

dsplaisted commented Apr 1, 2021

benvillalobos commented Apr 1, 2021

benvillalobos commented Apr 5, 2021

TysonMN commented May 27, 2021

benvillalobos commented May 27, 2021

Allow Custom CopyToOutputDirectory Location With TargetPath #6237

Allow Custom CopyToOutputDirectory Location With TargetPath #6237

Conversation

benvillalobos commented Mar 10, 2021 • edited

Context

Changes Made

Testing

Notes

cdmihai Mar 13, 2021

Choose a reason for hiding this comment

cdmihai Mar 13, 2021

Choose a reason for hiding this comment

cdmihai left a comment • edited

Choose a reason for hiding this comment

Forgind Mar 15, 2021

Choose a reason for hiding this comment

benvillalobos Mar 15, 2021

Choose a reason for hiding this comment

benvillalobos commented Mar 15, 2021

benvillalobos commented Mar 19, 2021

cdmihai commented Mar 19, 2021

benvillalobos commented Mar 22, 2021

benvillalobos commented Mar 29, 2021

dsplaisted commented Mar 31, 2021

benvillalobos commented Mar 31, 2021

benvillalobos commented Mar 31, 2021

Forgind commented Mar 31, 2021

benvillalobos commented Mar 31, 2021

dsplaisted commented Apr 1, 2021

benvillalobos commented Apr 1, 2021

benvillalobos commented Apr 5, 2021

TysonMN commented May 27, 2021

benvillalobos commented May 27, 2021

benvillalobos commented Mar 10, 2021 •

edited

cdmihai left a comment •

edited