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

Filtering for duplicate content files with referencing projects #4931

Merged
merged 6 commits into from
Apr 28, 2020

Conversation

sfoslund
Copy link
Member

@sfoslund sfoslund commented Nov 18, 2019

Addressing the issue of dependency content files overwriting the referencing project's content files of the same name (see dotnet/sdk#3871).
Test coverage in the SDK here.

@rainersigwald
Copy link
Member

I think this will conflict with #4865. @cdmihai would you mind taking a look at this one with that in mind?

@cdmihai
Copy link
Contributor

cdmihai commented Nov 20, 2019

I think this will conflict with #4865. @cdmihai Mihai Codoban FTE would you mind taking a look at this one with that in mind?

Yes, it will. I propose reimplementing this over #4865, because #4865 nicely separates the content items coming the current project from the content items coming from references.

@sfoslund
Copy link
Member Author

@cdmihai Sounds good to me, I'll update this once #4865 is in.

@rainersigwald
Copy link
Member

@sfoslund We merged the other PR; can you reimplement this in the new approach? Let me and @cdmihai know if you have any questions.

@cdmihai
Copy link
Contributor

cdmihai commented Dec 18, 2019

This is a breaking change for legacy style projects.

Should this behaviour be under a flag that only the Microsoft.Net.Sdk turns on? Or is the actual intent to force this upon everyone by default?

@sfoslund
Copy link
Member Author

@cdmihai Yes, a flag is a good idea. Do you have a certain flag in mind?

@cdmihai
Copy link
Contributor

cdmihai commented Dec 18, 2019

Do you have a certain flag in mind?

How about IgnoreConflictingTransitiveContent

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

Tested that this resolves dotnet/sdk#3871 offline, so this is good pending merge of #4997.
@sfoslund

@sfoslund
Copy link
Member Author

Thanks @Forgind! Looking back on previous feedback, I'll also want to add a flag to enable this behavior before merge, as discussed with @cdmihai

@Forgind
Copy link
Member

Forgind commented Mar 12, 2020

So more like <ItemGroup Condition="'$(IgnoreConflictingTransitiveContent)' == 'false'>? Should the flag be on by default or off?

@sfoslund
Copy link
Member Author

This is a breaking change for legacy style projects so it should be disabled by default but enabled for sdk style projects.

Enabled by default on Core; otherwise disabled by default.
@sfoslund sfoslund force-pushed the CopyAppSettingsIssue branch from 7763cb4 to 91f2c8d Compare April 20, 2020 18:35
Co-Authored-By: Rainer Sigwald <raines@microsoft.com>
Co-Authored-By: Rainer Sigwald <raines@microsoft.com>
@Forgind Forgind merged commit 82eff43 into dotnet:master Apr 28, 2020
@sfoslund sfoslund deleted the CopyAppSettingsIssue branch April 28, 2020 21:13
Forgind added a commit that referenced this pull request May 4, 2020
* LOC CHECKIN | microsoft/msbuild vs16.6 | 20200420 (#5299)

* Final branding_16.6 (#5273)

* merge

* Enable handshake timeout on Mono (#5318)

I am seeing consistent test hangs on macOS Mono. A node is getting into a bad state and is failing to respond to handshakes. While I do not yet understand the root cause, it is clear that having a timeout on the handshake operation mitigates the issue. The test is now failing but does not hang, saving developer time as well as test pipeline resources.

* Revert "Reverted and rebuilt for localizable strings." (#5246)

This adds back support for logging an error when a task returns false
without logging an error. This was originally added in #4940 but was
reverted because of multiple difficulties.

* Changed error code

* Add escape hatch

* Fix typo

* Filtering for duplicate content files with referencing projects (#4931)

* Updating content filtering based on content copying changes

* Add a flag that is enabled by default on Core; otherwise disabled by default.

* Adding Shutdown Message to Worker Nodes (#5262)

* Changed where Trace is being called and removed old functionality.

* Updating .NET Core branding to .NET (#5286)

* Override default Arcade Xunit configuration (#5302)

* Update Directory.Build.targets

* prevent arcade from injecting its own xunit file

* Don't log @(ReferencePath) at the end of ImplicitlyExpandDesignTimeFacades (#5317)

The actual ItemGroups inside the target already do a good job of logging exactly what items were added and/or removed and in what order and with what metadata.

Emitting an extra low-pri message which is unstructured here just adds noise, slows the builds down, wastes binlog space and is otherwise redundant.

* Compute hashes in parallel in GetFileHash (#5303)

* Compute hashes in parallel. This scales better for larger number of files.

* Use a dedicated write lock

* Allow disabling logging of task parameters and item metadata (#5268)

This enables fine-grained control over whether:

 * to log log each parameter (whether input or output)
 * or whether to log item metadata for each ITaskItem[] parameter.

When LogTaskInputs is set the default behavior is still to log all parameters and all item metadata for ITaskItem[] parameters. Since this is very verbose and hurts performance without adding any useful information it is valuable to be able to turn this logging off in certain situations.

This approach allows controlling logging via setting simple properties or environment variables.

I've identified the specific tasks and parameters that we want to restrict logging for that would give us the most gains without losing any significant useful info:

https://github.com/KirillOsenkov/MSBuildStructuredLog/wiki/Task-Parameter-Logging

* Update dependencies from https://github.com/dotnet/arcade build 20200430.5 (#5325)

- Microsoft.DotNet.Arcade.Sdk: 1.0.0-beta.20221.2 -> 1.0.0-beta.20230.5

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Use environment variable for handshake Resolves #4961 (#5196)

* Use environment variable for handshake Resolves #4961

* Combine means of hashing

* Support transitive project references in static graph (#5326)

Transitive project references are a thing now. Added support to static graph, so that buildxl and qb can avoid adding the transitive refs.
This PR is independent of #5222. Ideally, review this one first, as QB has a stronger dependency on this PR than on #5222.

Design

- transitive references are opt-in, per project evaluation
- once a project opts-in, transitivity is applied for all ProjectReference items
- a project opt-ins by setting the property AddTransitiveProjectReferencesInStaticGraph to true. The sdk does this automatically in Microsoft.Managed.After.Targets.
- interaction with crosstargeting: transitive refs are added only to inner builds, not the outer builds. This mimics vanilla msbuild.

Co-authored-by: Rainer Sigwald <raines@microsoft.com>

Co-authored-by: Martin Chromecek (Moravia IT) <v-chmart@microsoft.com>
Co-authored-by: Ladi Prosek <laprosek@microsoft.com>
Co-authored-by: Sarah Oslund <sfoslund@microsoft.com>
Co-authored-by: Ben Villalobos <villalobosb93@gmail.com>
Co-authored-by: Mihai Codoban <codobanmihai@gmail.com>
Co-authored-by: Kirill Osenkov <KirillOsenkov@users.noreply.github.com>
Co-authored-by: Pranav K <prkrishn@hotmail.com>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Rainer Sigwald <raines@microsoft.com>
sfoslund added a commit to sfoslund/msbuild that referenced this pull request May 15, 2020
…et#4931)

* Updating content filtering based on content copying changes

* Add a flag that is enabled by default on Core; otherwise disabled by default.
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.

5 participants