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

Fix transitive content conflicts Fixes #3871 #11352

Merged
merged 6 commits into from
May 12, 2020

Conversation

Forgind
Copy link
Member

@Forgind Forgind commented Apr 20, 2020

Changes this to opt-out for sdk-style projects. (See discussion in microsoft/msbuild#4931 and microsoft/msbuild#4997.)

Fixes #3871.

Changes this to opt-out for sdk-style projects.
@sfoslund sfoslund requested a review from a team April 20, 2020 23:15
@dsplaisted
Copy link
Member

This also fixes #3864, correct?

@sfoslund
Copy link
Member

@dsplaisted I don't think so, this ensures that transitive content files don't overwrite other content files of the same name. If I'm reading #3864 correctly, that issue refers to copying all transitive content, regardless of name.

@sfoslund
Copy link
Member

sfoslund commented May 4, 2020

@Forgind @dsplaisted the corresponding MSBuild PR was merged, right? So this can be merged too?

@Forgind
Copy link
Member Author

Forgind commented May 4, 2020

Can confirm:
dotnet/msbuild#4931

@sfoslund
Copy link
Member

sfoslund commented May 5, 2020

@Forgind cool, I'm merging with master to get the new version of MSBuild that has the change, just to get CI run with it enabled.

@rainersigwald
Copy link
Member

Should this have a test and an inverse test for when it's explicitly disabled?

@sfoslund
Copy link
Member

sfoslund commented May 5, 2020

@rainersigwald sure, I just pushed the test I wrote back in November 😆. It's a little messy because this issue is hard to repro/ detect consistently.

@sfoslund
Copy link
Member

sfoslund commented May 6, 2020

Weird, this tests is reliable on my local machine but it looks like it's really flaky in general. @Forgind were you able to get a consistent repro when you were testing that could be put into this testing format?

@Forgind
Copy link
Member Author

Forgind commented May 7, 2020

It seemed to repro reasonably consistently if I remember correctly, but I just manually tested it, and unfortunately, this isn't very high on my priority list right now. I can try to get back to it in another month or so.

@sfoslund sfoslund marked this pull request as draft May 7, 2020 18:46
@sfoslund sfoslund force-pushed the conflicting-transitive-content-fix branch from ab6bb05 to 25335a3 Compare May 8, 2020 19:55
@sfoslund
Copy link
Member

sfoslund commented May 8, 2020

I've added a new test that asserts the target output instead of the file that ends up in the output folder, which is unreliable when there are conflicting content files. This one should be much more reliable and doesn't require adding a new test asset.

@dsplaisted can you review, since I've added code?

@sfoslund sfoslund marked this pull request as ready for review May 11, 2020 21:00
@sfoslund sfoslund merged commit 7fb2def into master May 12, 2020
@sfoslund sfoslund deleted the conflicting-transitive-content-fix branch May 12, 2020 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dotnet build copies appsettings.json from referenced project.
4 participants