Skip to content
This repository has been archived by the owner on Jan 12, 2024. It is now read-only.

Remove duplicate files instead of throwing an exception. #151

Merged
merged 3 commits into from
Dec 1, 2017

Conversation

thorgeir
Copy link

@thorgeir thorgeir commented Nov 7, 2017

When the _PackageContent variable contains duplicates I think it should not result in an exception, maybe a warning but not an exception. Because form my experience the duplicates are not different files but the same file referenced twice.
Removing duplicates in the CreatePackage task works and the package contains all the files it should.

Maybe the duplicates could be resolved earlier in the AssignPackagePath task?

@dnfclas
Copy link

dnfclas commented Nov 7, 2017

@thorgeir,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@kzu
Copy link
Contributor

kzu commented Nov 7, 2017

Because form my experience the duplicates are not different files but the same file referenced twice.

Maybe we can make sure of that by also comparing the actual files? (maybe a quick size+update time?)

Because picking multiple different source files and attempting to package them to the same location is most definitely an authoring error that you'd want to flag and make very obvious, I think.

Would need adjusting the unit test that verifies this behavior:

src\Build\NuGet.Build.Packaging.Tests\given_a_library_with_private_assets_reference.cs(60,0): at NuGet.Build.Packaging.given_a_library_with_private_assets_reference.when_getting_package_contents_then_contains_private_lib_assets_as_primary_output_and_also_package_reference()
      NuGet.Build.Packaging.given_duplicate_package_files.build_fails [FAIL]
        Assert.Equal() Failure
        Expected: Failure
        Actual:   Success
        Stack Trace:
          src\Build\NuGet.Build.Packaging.Tests\given_duplicate_package_files.cs(20,0): at 

(current CI failure)

@thorgeir
Copy link
Author

thorgeir commented Nov 8, 2017

Sure, I'll try to add that.
Regarding the unit tests, I was unable to compile the multi_platform_solution locally. I have xamarin installed but the ios and android projects are not loading, what am I missing?

@kzu
Copy link
Contributor

kzu commented Nov 9, 2017

You need to install the .NET Core SDK which brings in the SDK-style projects/imports.

The updated implementation looks 👍

@thorgeir
Copy link
Author

thorgeir commented Nov 9, 2017

I can't see how this change affected the given_a_library_with_private_assets_reference.when_getting_package_contents_then_contains_private_lib_assets_as_primary_output_and_also_package_reference test I tried reverting back to the original code and it still failed.

@kzu
Copy link
Contributor

kzu commented Nov 9, 2017

hm, that's weird, since there were no other code changes between the last successful build and this one :(

kzu added a commit to kzu/NuGet.Build.Packaging that referenced this pull request Dec 1, 2017
The package cache lower cases all paths, which no longer matches the
package id as we were previously doing. We need to match in a case-
insensitive way therefore for private assets matching to succeed.

This fixes a broken unit test that was blocking two PRs NuGet#151 and NuGet#150.

Also, fix the nuget package versions at the time of this test authoring,
so we don't accidentally break if newer packages ever change and cause
the expectations to fail.
kzu added a commit that referenced this pull request Dec 1, 2017
The package cache lower cases all paths, which no longer matches the
package id as we were previously doing. We need to match in a case-
insensitive way therefore for private assets matching to succeed.

This fixes a broken unit test that was blocking two PRs #151 and #150.

Also, fix the nuget package versions at the time of this test authoring,
so we don't accidentally break if newer packages ever change and cause
the expectations to fail.
@kzu kzu force-pushed the remove-duplicates branch from 4b2db2f to 6712abc Compare December 1, 2017 20:52
@kzu kzu force-pushed the remove-duplicates branch from 6712abc to 67160f3 Compare December 1, 2017 20:54
Consider easy scenarios like assuming same package path and
source full path, same package path, different source file
but same length and file size (i.e. same source file added
as Content and copied to each project output and added from
there). But also add de-duplication of files where even if
the last write is different, they still have the same contents,
which could be the case for generated files whose source is
shared across projects (i.e. in a shared project, or generated
via MSBuild, etc.).

This covers all de-duplication scenarios we could think of,
and only reports the true duplicates that can't be disambiguated
by any of the above means, and are truly authoring errors.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants