Generate error message for duplicate items #624

dsplaisted · 2017-01-11T00:36:44Z

Fixes #600

Generate an error message if there are duplicate Compile, EmbeddedResource, or Content items. The error message looks like this:

C:\git\dotnet-sdk\bin\Debug\Sdks\Microsoft.NET.Sdk\build\Microsoft.NET.Sdk.DefaultItems.targets(66,5): error : Duplicate Compile items were included. The .NET SDK includes Compile items from your project directory by default. You can either remove these items from your project file, or set the 'EnableDefaultCompileItems' property to 'false' if you want to explicitly include them in your project file. For more information, see https://github.com/dotnet/sdk The duplicate items were: 'Class1.cs', 'DuplicateCompileItems.cs'

If default items are disabled, then the error message won't include information about how to disable them:

C:\git\dotnet-sdk\bin\Debug\Sdks\Microsoft.NET.Sdk\build\Microsoft.NET.Sdk.DefaultItems.targets(66,5): error : Duplicate Compile items were included. The duplicate items were: 'DuplicateCompileItems.cs'

Open questions or other notes:

Create a fwlink that will point to the documentation for default items. @blackdwarf, can you do this?
Should the .NET SDK check for duplicate Content items, or since it doesn't have any logic for them by default should that go in the Web SDK?
This doesn't check for duplicate None items, as I don't think those would typically cause an issue
Does formatting items in a list need to be localized?
Should we check for duplicate items at all if the default items are disabled?

rainersigwald · 2017-01-11T16:13:11Z

src/Tasks/Microsoft.NET.Build.Tasks/Resources/Strings.resx

+    <value>Duplicate {0} items were included.{1}  The duplicate items were: {2}</value>
+  </data>
+  <data name="DuplicateItemsHowToFix" xml:space="preserve">
+    <value> You can either remove these items from your project file, or set the '{0}' property to '{1}' if you want to explicitly include them in your project file.{2}</value>


I suspect that the experiences of #623 and dotnet/project-system#1126 indicate that we should also promote the Update and Remove mechanisms here. But that makes a very long error, which isn't ideal.

rainersigwald · 2017-01-11T16:14:58Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.DefaultItems.targets

+      MoreInformationLink="$(DefaultItemsMoreInformationLink)"
+      />
+
+    <!-- TODO: Do we check for duplicate content items here, or in the Web SDK? EnableDefaultContentItems isn't defined or used at all by the .NET SDK -->


I'd say to do it here because Content is a type known in Common.targets, and this is the closest layer above that. If it were defined and used only in Web, I'd say to move it there.

rainersigwald · 2017-01-11T16:28:09Z

src/Tasks/Microsoft.NET.Build.Tasks/CheckForDuplicateItems.cs

+                }
+
+                //  TODO: Does quoting and the separator between items in the list need to be localized?
+                string duplicateItemsFormatted = string.Join(", ", duplicateItems.Select(d => $"'{d.Key}'"));


I don't know what the right answer is. My guess is: change this to a semicolon (like MSBuild uses to separate items) and don't localize it (MSBuild itself doesn't seem to, probably in part because there's no distinction between "format list of items to print" and "format list of items to pass a string to code" which would have different localization needs).

nguerrera · 2017-01-11T21:30:25Z

src/Tasks/Microsoft.NET.Build.Tasks/Resources/Strings.resx

+    <value>Duplicate {0} items were included.{1}  The duplicate items were: {2}</value>
+  </data>
+  <data name="DuplicateItemsHowToFix" xml:space="preserve">
+    <value> You can either remove these items from your project file, or set the '{0}' property to '{1}' if you want to explicitly include them in your project file.{2}</value>


This is really hard to follow for translators: leading whitespace, placeholder followed by two spaces followed by another sentence, placeholder immediately after a sentence. Can we just log consecutive independent messages? If not, maybe don't worry about DRY and just put the full text of each message a user can see so the translator has all of the context. At a minimum, use the <comment> element to explain how these compose.

nguerrera · 2017-01-11T21:31:36Z

src/Tasks/Microsoft.NET.Build.Tasks/CheckForDuplicateItems.cs

+
+        protected override void ExecuteCore()
+        {
+            var duplicateItems = Items.GroupBy(i => i.ItemSpec).Where(g => g.Count() > 1).ToList();


Why ToList()?

Also is g.Count() cheap here in practice? (do the groups implement ICollection?)

I would be interested in seeing some measurment where this task shows up on the perf summary. Maybe it doesn't register in comparison to surrounding logic, but my instinct from lower level work is to cringe at all of this LINQ.

dsplaisted · 2017-01-12T17:45:57Z

Do we check for duplicate content items here, or in the Web SDK? EnableDefaultContentItems isn't defined or used at all by the .NET SDK

I'd say to do it here because Content is a type known in Common.targets, and this is the closest layer above that. If it were defined and used only in Web, I'd say to move it there.

I think the question is, are we checking for duplicate items in general, or are we just checking for duplicate items caused by the implicit includes in the SDK? Does adding an extra check and error message add value if the problem is unrelated to the implicit includes?

I'm leaning towards saying that these checks and error messages should only apply when implicit includes are enabled. That would also mean that the validation for Content items more logically belongs in the Web SDK.

@rainersigwald @nguerrera @srivatsn Thoughts?

… generate error if implicit items are enabled

dsplaisted · 2017-01-12T19:31:22Z

I've updated the PR to only use one resource string for the error message. It should now look like this (mostly the same as it was previously):

C:\git\dotnet-sdk\bin\Debug\Sdks\Microsoft.NET.Sdk\build\Microsoft.NET.Sdk.DefaultItems.targets(66,5): error : Duplicate 'Compile' items were included. The .NET SDK includes 'Compile' items from your project directory by default. You can either remove these items from your project file, or set the 'EnableDefaultCompileItems' property to 'false' if you want to explicitly include them in your project file. The duplicate items were: 'Class1.cs'; 'DuplicateCompileItems.cs' [C:\git\dotnet-sdk\bin\Debug\Tests\It_gives_an_error_message_if_duplicate_compile_items_are_included\DuplicateCompileItems\DuplicateCompileItems\DuplicateCompileItems.csproj]

I've also modified it to only generate the error if the implicit items were enabled. If not, then it will be up to targets further down the pipeline to detect the issue and generate an error as they always have.

dsplaisted · 2017-01-12T19:40:00Z

@MattGertz for approval

Scenario

A user upgrades from RC2 to RC3. The projects they created with RC2 will generate compile errors saying that the source files were specified multiple times. This is because we now include the source files implicitly. This change will generate an error message that explains how to fix the issue:

error : Duplicate 'Compile' items were included. The .NET SDK includes 'Compile' items from your project directory by default. You can either remove these items from your project file, or set the 'EnableDefaultCompileItems' property to 'false' if you want to explicitly include them in your project file. The duplicate items were: 'Class1.cs'; 'DuplicateCompileItems.cs'

Bug

#600

Workarounds

n/a

Risk

Low - Tests should cover any impacted scenarios

Performance Impact

TBD but should be low - it involves calling an additional task for each of the Compile, EmbeddedResource, and Content item types and checking for duplicates.

Regression Analysis

n/a

nguerrera · 2017-01-12T23:26:13Z

src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.DefaultItems.targets

+
+    <PropertyGroup>
+      <!-- TODO: Create a fwlink for this -->
+      <DefaultItemsMoreInformationLink>https://github.com/dotnet/sdk</DefaultItemsMoreInformationLink>


We need to have an acceptable link before we merge.

natidea · 2017-01-12T23:32:39Z

src/Tasks/Microsoft.NET.Build.Tasks/CheckForDuplicateItems.cs

+        {
+            if (DefaultItemsEnabled && DefaultItemsOfThisTypeEnabled)
+            {
+                var duplicateItems = Items.GroupBy(i => i.ItemSpec).Where(g => g.Count() > 1).ToList();


Can required inputs be null? Change to

(Items ?? Enumerable<ITaskItem>.Empty())

natidea · 2017-01-12T23:33:47Z

Shiproom approved

blackdwarf · 2017-01-12T23:48:28Z

@dsplaisted where does the new link go to?

dsplaisted · 2017-01-13T00:02:41Z

@blackdwarf It goes to https://github.com/dotnet/core/blob/master/release-notes/sdk-implicit-items.md. You're an owner of the link as well @terrajobst, @richlander, and a few others.

AdrienTorris · 2017-03-09T09:53:35Z

When I upgraded my project from VS2017 RC to VS2017 it causes an error in all my class library projects. After reinstalling VS (yes I read that it was the solution ...) I deleted the line <Compile Include="**\*.cs" /> in all the csproj files of my class libraries to can build again.
Is it the good workaround ? Or set the 'EnableDefaultCompileItems' property to 'false' is a the good way to pass through this situation ?

richlander · 2017-03-09T17:31:30Z

I think this explains it: https://github.com/dotnet/core/blob/master/release-notes/1.0/sdk/1.0-rc3-default-compile-items.md. Does that make sense?

…0190424.4 (dotnet#624) - Microsoft.AspNetCore.Mvc.Analyzers - 3.0.0-preview6-19224-04 - Microsoft.AspNetCore.Mvc.Api.Analyzers - 3.0.0-preview6-19224-04

Check for duplicate items and generate an error if they are found

495afdc

dnfclas added the cla-already-signed label Jan 11, 2017

dsplaisted force-pushed the duplicate-item-error-600 branch from c68c901 to 687a64f Compare January 11, 2017 00:43

Don't check for duplicate PackageReference items

08f2893

dsplaisted force-pushed the duplicate-item-error-600 branch from 687a64f to 08f2893 Compare January 11, 2017 00:45

dsplaisted requested review from srivatsn, nguerrera and natidea January 11, 2017 00:57

rainersigwald mentioned this pull request Jan 11, 2017

RemoveDuplicates task should have a "had duplicates" output dotnet/msbuild#1541

Closed

rainersigwald approved these changes Jan 11, 2017

View reviewed changes

nguerrera reviewed Jan 11, 2017

View reviewed changes

nguerrera suggested changes Jan 11, 2017

View reviewed changes

dsplaisted mentioned this pull request Jan 12, 2017

Allow projects to override implicit PackageReference items with item Include syntax #633

Merged

Simplify duplicate item error message to be one resource string, only…

cccf1fe

… generate error if implicit items are enabled

MattGertz added this to the 1.0 RC3 milestone Jan 12, 2017

MattGertz added the Pending Shiproom Approval label Jan 12, 2017

nguerrera reviewed Jan 12, 2017

View reviewed changes

natidea reviewed Jan 12, 2017

View reviewed changes

natidea approved these changes Jan 12, 2017

View reviewed changes

natidea added the Approved to merge label Jan 12, 2017

Add aka.ms link for more information on implicit items

2085098

dsplaisted merged commit 159b8fc into dotnet:dev15-rc3 Jan 13, 2017

dsplaisted deleted the duplicate-item-error-600 branch January 13, 2017 00:03

srivatsn mentioned this pull request Jan 13, 2017

Generate a more helpful error message if Compile (or other implicit items) are specified multiple times #600

Closed

This was referenced Jan 17, 2017

The item "Class1.cs" was specified more than once in the "Sources" parameter #668

Closed

Show duplicate item errors due to implicit items in design-time builds #675

Merged

srivatsn mentioned this pull request Jan 18, 2017

Hide items with 'Visible=false' metadata dotnet/project-system#1126

Closed

jeffkl mentioned this pull request Jan 30, 2017

Duplicate 'Content' items error when building asp.net core projects in VS 2017 RC dotnet/msbuild#1629

Closed

stan-sz mentioned this pull request Jan 10, 2023

Issue a warning for duplicate items (Compile, Reference, ProjectReference) dotnet/msbuild#804

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate error message for duplicate items #624

Generate error message for duplicate items #624

dsplaisted commented Jan 11, 2017

rainersigwald Jan 11, 2017

rainersigwald Jan 11, 2017

rainersigwald Jan 11, 2017

nguerrera Jan 11, 2017 •

edited

Loading

nguerrera Jan 11, 2017

nguerrera Jan 11, 2017 •

edited

Loading

dsplaisted commented Jan 12, 2017

dsplaisted commented Jan 12, 2017

dsplaisted commented Jan 12, 2017

nguerrera Jan 12, 2017

natidea Jan 12, 2017

natidea commented Jan 12, 2017

blackdwarf commented Jan 12, 2017

dsplaisted commented Jan 13, 2017

AdrienTorris commented Mar 9, 2017 •

edited

Loading

richlander commented Mar 9, 2017

Generate error message for duplicate items #624

Generate error message for duplicate items #624

Conversation

dsplaisted commented Jan 11, 2017

rainersigwald Jan 11, 2017

Choose a reason for hiding this comment

rainersigwald Jan 11, 2017

Choose a reason for hiding this comment

rainersigwald Jan 11, 2017

Choose a reason for hiding this comment

nguerrera Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

nguerrera Jan 11, 2017

Choose a reason for hiding this comment

nguerrera Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

dsplaisted commented Jan 12, 2017

dsplaisted commented Jan 12, 2017

dsplaisted commented Jan 12, 2017

Scenario

Bug

Workarounds

Risk

Performance Impact

Regression Analysis

nguerrera Jan 12, 2017

Choose a reason for hiding this comment

natidea Jan 12, 2017

Choose a reason for hiding this comment

natidea commented Jan 12, 2017

blackdwarf commented Jan 12, 2017

dsplaisted commented Jan 13, 2017

AdrienTorris commented Mar 9, 2017 • edited Loading

richlander commented Mar 9, 2017

nguerrera Jan 11, 2017 •

edited

Loading

nguerrera Jan 11, 2017 •

edited

Loading

AdrienTorris commented Mar 9, 2017 •

edited

Loading