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

Allow Custom CopyToOutputDirectory Location With TargetPath #6237

Merged
merged 12 commits into from Apr 5, 2021

Conversation

@BenVillalobos
Copy link
Member

@BenVillalobos BenVillalobos commented Mar 10, 2021

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:

  1. Visual studio sees them in a specific folder (via <Link>).
  2. 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.

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

This comment has been minimized.

@cdmihai

cdmihai Mar 13, 2021
Contributor

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.


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

This comment has been minimized.

@cdmihai

cdmihai Mar 13, 2021
Contributor

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

Copy link
Contributor

@cdmihai cdmihai left a comment

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 :)

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

This comment has been minimized.

@Forgind

Forgind Mar 15, 2021
Member

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.)

This comment has been minimized.

@BenVillalobos

BenVillalobos Mar 15, 2021
Author Member

Going with TargetPathOverride for clarity.

Base automatically changed from master to main Mar 15, 2021
@BenVillalobos
Copy link
Member Author

@BenVillalobos BenVillalobos commented Mar 15, 2021

@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 BenVillalobos requested a review from dsplaisted Mar 19, 2021
@BenVillalobos
Copy link
Member Author

@BenVillalobos BenVillalobos commented Mar 19, 2021

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

To quickly summarize what the goal is:

  1. Have an item show in VS under a specific folder
  2. Output that item to a folder that differs from how it shows in VS.
  3. 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
Copy link
Contributor

@cdmihai cdmihai commented Mar 19, 2021

@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
Copy link
Member Author

@BenVillalobos BenVillalobos commented Mar 22, 2021

friendly reminder. @dsplaisted

@BenVillalobos
Copy link
Member Author

@BenVillalobos BenVillalobos commented Mar 29, 2021

@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
Copy link
Member

@dsplaisted dsplaisted commented Mar 31, 2021

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
Copy link
Member Author

@BenVillalobos BenVillalobos commented Mar 31, 2021

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.

@BenVillalobos BenVillalobos changed the title Allow Custom CopyToOutputDirectory Location Allow Custom CopyToOutputDirectory Location With TargetPath Mar 31, 2021
@BenVillalobos
Copy link
Member Author

@BenVillalobos BenVillalobos commented Mar 31, 2021

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
Copy link
Member

@Forgind Forgind commented Mar 31, 2021

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
Copy link
Member Author

@BenVillalobos BenVillalobos commented Mar 31, 2021

@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.

@dsplaisted
Copy link
Member

@dsplaisted dsplaisted commented Apr 1, 2021

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?

@BenVillalobos
Copy link
Member Author

@BenVillalobos BenVillalobos commented Apr 1, 2021

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
Copy link
Member Author

@BenVillalobos BenVillalobos commented Apr 5, 2021

@dsplaisted ping for an approving review or feedback 🙂

@Forgind Forgind merged commit 7804350 into dotnet:main Apr 5, 2021
7 checks passed
7 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210401.11 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
@TysonMN
Copy link

@TysonMN TysonMN commented May 27, 2021

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
Copy link
Member Author

@BenVillalobos BenVillalobos commented May 27, 2021

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

Successfully merging this pull request may close these issues.

6 participants