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

Stop removing items that were included in the project file #630

Merged
merged 4 commits into from
Jan 14, 2017

Conversation

dsplaisted
Copy link
Member

Instead, only apply the patterns that used to be called DefaultRemoves to the items that are implicitly included. If someone adds an include for a file to their project file, we should respect that.

Fixes #623
Fixes #588

The DefaultRemoves property was originally used so that when you had wildcard includes in your project file, you wouldn't have to specifically exclude the bin, obj, etc. folders. When we switched to implicit includes, I left it in with the idea that it would help you have a cleaner project file if you did want to explicitly see a wildcard include in your project, and it would help protect you from accidentally including items that you "shouldn't" from the output folders.

However, it's become apparent that it comes with two huge downsides:

  • If you actually want items that would match the patterns in DefaultRemoves, you will include them in the project file, and that won't work, and you will have absolutely no idea why and there's no good way to debug / figure it out
  • If you do manage to figure out why the items aren't being included, there's no good way to override the behavior to let you include a file. You have to turn off the DefaultRemoves entirely, which then means your implicit globs will suddenly pick up a bunch of files that they shouldn't, unless you jump through even more hoops in your project file.

This PR makes it so that if you put an item in your project file, it will be respected. The items that will be included implicitly remain the same as before.

This PR also takes the opportunity to remove the packages folder from the list of folders that the default includes don't apply to. It's unclear why this was originally added in the CLI. @davidfowl had this to say about it:

It was likely because of a mix of older packages folder (nuget v2) and xproj. Though that should only have been at the solution level...

Removing this exclusion will make default items in projects with code or other assets in a "packages" folder work as people will expect.

This change will need to be made together with a change to the Web SDK, as now it will be its responsibility to remove these items from its default includes. I will also submit a PR for that.

Instead, only apply the patterns that used to be called DefaultRemoves to the items that are implicitly included.  If someone adds an include for a file to their project file, we should respect that.

Fixes dotnet#623
Fixes dotnet#588
@dsplaisted
Copy link
Member Author

I've filed the Web SDK PR: dotnet/websdk#94

@@ -24,7 +24,7 @@ public void It_ignores_excluded_folders()
{
Action<GetValuesCommand> setup = getValuesCommand =>
{
foreach (string folder in new[] { "bin", "obj", "packages" })
foreach (string folder in new[] { "bin", "obj" })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine but I'm a tiny bit worried about doing it in combination with the higher-pri change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point taken, I'll back that part out and send it as a separate PR.

@dsplaisted
Copy link
Member Author

dsplaisted commented Jan 12, 2017

@MattGertz for approval

Scenario

Include items in the project file that are in the output paths, or have an extension that we exclude from the implicit item includes. For example, a developer wants to include an item from the output folder in the NuGet package:

  <ItemGroup>
    <Content Include="$(OutputPath)\$(AssemblyName).runtimeconfig.json">
      <Pack>true</Pack>
      <PackagePath>lib\$(TargetFramework)</PackagePath>
    </Content>
  </ItemGroup>

Currently, this will be ignored entirely, with no good way for the developer to understand why.

Or, they want to copy a folder to the output directory:

<Content Include="TestProjects\**\*" CopyToOutputDirectory="PreserveNewest" />

Currently, any files in the folder with an extension ending in "proj" (ie .csproj, .proj, etc) would not be copied, though the rest of the files in the folder would be.

Bug

#623 and #588

Workarounds

See #623. This is ugly and most people won't even get as far as figuring out what's wrong

Risk

Low - Tests should cover any impacted scenarios

Performance Impact

Low - the change moves a file filter from happening after the main contents of the project file is evaluated to before then

Regression Analysis

n/a

@MattGertz
Copy link

@dsplaisted @srivatsn I don't understand the full customer scenario here -- can you please add clarifications on the customer impact to the template?

@dsplaisted
Copy link
Member Author

@MattGertz I've updated the scenario as requested

@natidea
Copy link
Contributor

natidea commented Jan 12, 2017

Shiproom approved to merge

@srivatsn srivatsn merged commit ace633b into dotnet:dev15-rc3 Jan 14, 2017
vijayrkn pushed a commit to dotnet/websdk that referenced this pull request Jan 17, 2017
@dsplaisted dsplaisted deleted the default-removes-623 branch January 31, 2017 14:41
mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020
…426.3 (dotnet#630)

- Microsoft.DotNet.Arcade.Sdk - 1.0.0-beta.19226.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants