Log an error when a Task returns false but didn't log an error#4940
Merged
benvillalobos merged 16 commits intodotnet:masterfrom Feb 24, 2020
Merged
Conversation
This doesn't apply to all tasks at the moment.
rainersigwald
requested changes
Nov 25, 2019
Member
rainersigwald
left a comment
rainersigwald
left a comment
There was a problem hiding this comment.
Can you add a test that this matches the behavior of normal task-emitted errors in the various ContinueOnError cases?
src/Build.UnitTests/BackEnd/ReturnFailureWithoutLoggingErrorTask.cs
Outdated
Show resolved
Hide resolved
Member
Author
|
A lead on what's causing tests to hang: |
Member
Author
|
The tests pass? I don't trust this, rerunning. |
Member
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Added copyright Added comments explaining: ReturnFailureWithoutLoggingErrorTask returning false Why MSBuild tasks are excluded from tasks returning false. Changed the .resx string
rainersigwald
approved these changes
Dec 12, 2019
| ITestOutputHelper _output; | ||
|
|
||
| public BuildResult_Tests() | ||
| public BuildResult_Tests(ITestOutputHelper output) |
Member
There was a problem hiding this comment.
This is a good change, but is the output used anywhere?
Member
Author
There was a problem hiding this comment.
No, this was more of a "I touched this file so might as well add this" change, despite undoing the need for the change now. I can remove it entirely at this point.
| } | ||
| } | ||
|
|
||
| internal bool HasLoggedErrors { get { return _hasLoggedErrors; } set { _hasLoggedErrors = value; } } |
Member
There was a problem hiding this comment.
Should we add some more scoped unit tests to LoggingContext_Tests? It doesn't look like there are any tests that cover the methods you're changing :(
Member
Author
There was a problem hiding this comment.
Agreed. Any suggestions for testing this specifically?
rainersigwald
changed the title
Member
Author
|
Testing azure pipeline syntax. /azp run |
rainersigwald
approved these changes
Feb 21, 2020
Member
rainersigwald
left a comment
rainersigwald
left a comment
There was a problem hiding this comment.
Pushed a very simple test that we'll call good enough for now.
Co-Authored-By: Rainer Sigwald <raines@microsoft.com>
Member
Author
|
I'm okay with that. Getting a build to run and having access to the taskloggingcontext to see if it logged any errors was somewhat troublesome. |
Forgind
added a commit
to Forgind/msbuild
that referenced
this pull request
Apr 28, 2020
This adds back support for logging an error when a task returns false without logging an error. This was originally added in dotnet#4940 but was reverted because of multiple difficulties.
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2036
In the off chance that a task returns false but does not log an error, the build will look like it succeeded despite a failure. This change raises an error displaying the task that returned false but did not log an error.