Skip to content

Conversation

@benvillalobos
Copy link
Member

Making debugging easier, one step at a time

@Forgind
Copy link
Contributor

Forgind commented Apr 9, 2020

How is this an improvement on the line immediately before?

@benvillalobos
Copy link
Member Author

The short answer is I didn't notice it, mainly because I never saw that log message or the files it's generating. Interesting that it creates a fresh file only to write that single line.

Looking at this now I think the logic should actually replace the line you highlighted, so instead of creating and writing to a file it just does a CommunicationsUtilities.Trace call.

@Forgind
Copy link
Contributor

Forgind commented Apr 9, 2020

I'll admit that's a little weird. Maybe replace their file write with CommunicationsUtilities.Trace? I'd like to keep the if (_debugCommunications) part.

@benvillalobos
Copy link
Member Author

Agreed, the if statement needs to stay and I'll replace the streamwriter logic with the Trace call.

Copy link
Contributor

@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.

LGTM

@Forgind Forgind merged commit c540980 into dotnet:master Apr 28, 2020
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 <[email protected]>

Co-authored-by: Martin Chromecek (Moravia IT) <[email protected]>
Co-authored-by: Ladi Prosek <[email protected]>
Co-authored-by: Sarah Oslund <[email protected]>
Co-authored-by: Ben Villalobos <[email protected]>
Co-authored-by: Mihai Codoban <[email protected]>
Co-authored-by: Kirill Osenkov <[email protected]>
Co-authored-by: Pranav K <[email protected]>
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 <[email protected]>
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.

3 participants