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

Remove duplicate files instead of throwing an exception. - #151

Merged
kzu merged 3 commits into
NuGet:devfrom
thorgeir:remove-duplicates
Dec 1, 2017
Merged

kzu merged 3 commits into
NuGet:devfrom
thorgeir:remove-duplicates

Conversation

@thorgeir

@thorgeir thorgeir commented Nov 7, 2017

Copy link
Copy Markdown

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

dnfclas commented Nov 7, 2017

Copy link
Copy Markdown

@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

kzu commented Nov 7, 2017

Copy link
Copy Markdown
Contributor

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

thorgeir commented Nov 8, 2017

Copy link
Copy Markdown
Author

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

kzu commented Nov 9, 2017 •

Copy link
Copy Markdown
Contributor

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

The updated implementation looks 👍

@thorgeir

thorgeir commented Nov 9, 2017 •

Copy link
Copy Markdown
Author

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

kzu commented Nov 9, 2017

Copy link
Copy Markdown
Contributor

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