MicrosoftPremier / VstsExtensions

Documentation and issue tracking for Microsoft Premier Services Visual Studio Team Services Extensions
MIT License
57 stars 14 forks source link

No warnings reported for .NET projects built with Shell Script tasks and Cake Build #2

Closed cosminstirbu closed 6 years ago

cosminstirbu commented 6 years ago

Hi,

For some reason no warnings are reported for .NET projects built with a Shell Script task that invokes Cake Build bash script.

The build reports general MSBuild warnings such as:

2018-05-15T18:08:38.3790300Z Pages/HomePage.cs(51,50): warning CS0169: The field 'HomePage._headerMessage' is never used [/Users/vsts/agent/2.133.3/work/1/s/SampleProject.csproj]

or StyleCop warnings such as:

2018-05-15T18:08:38.3620380Z /Users/vsts/agent/2.133.3/work/1/s/SampleProject/Views/SampleView.designer.cs(60,40,60,40): warning : SA1008 : CSharp.Spacing : Invalid spacing around the opening parenthesis. [/Users/vsts/agent/2.133.3/work/1/s/SampleProject.csproj]

Is this something related to the Tasks regex?

Thank you, Cosmin

almtcger commented 6 years ago

Hi Cosmin,

could you first please make sure that those warnings appear as issues in your build's summary? Our task does not directly inspect the log output of other tasks but simply reads the number of issues from the build itself. Thus, the task only sees warnings if they were correctly logged to the build.

I assume that your cake script prevents the MSBuild warnings to be reported directly to the build system. Could you run your MSBuild project file directly using the MSBuild task? This should make sure that issues are reported correctly and can be picked up by our task.

We had a similar report from a customer using FAKE scripts started from a batch file. Even though FAKE just ran MSBuild, issues were not logged. This customer then created a MSBuild wrapper project that simply started the FAKE script and used the MSBuild task to run that project file. This solved the issue for them.

Just let me know if this helped.

René

cosminstirbu commented 6 years ago

Hi @almtcger,

Thank you for your reply.

Is there a way that the extension can be modified to read from the console log as well? Doing the MSBuild wrapper project is a painful wokraround.

Thank you, Cosmin

almtcger commented 6 years ago

Hi Cosmin,

the extension already can read the log output of other tasks. However, it doesn't know what kind of output is a warning and what isn't. You could use the warning filters parameter (see https://github.com/MicrosoftPremier/VstsExtensions/blob/master/BuildQualityChecks/en-US/WarningsPolicy.md#warnFilters) to tell the task what to count as a warning, but that might be tedious work.

Your two examples from the initial post would be picked up by an expression like this: ^.+\(\d+(,\d+)*\):\s+warning

This would match all log lines that start with some arbitrary output followed by one or more position indicators in parentheses, a colon, and the word warning. Other warnings might look different, but this is a good start.

The issue you linked explains quite well why the task doesn't pick up the warnings by default: neither your CAKE script nor MSBuild uses the logging commands. The MSBuild task actually contains the logic to pick up warnings and log them using the logging commands. Since you're not using the MSBuild task, those warnings are just treated as regular log output.

Hope this helps, René

cosminstirbu commented 6 years ago

I did use the regex above for the warnings regex but it still doesn't report any issues.

In the console log I can see that The specified task filters did not match any build task.

The regex for tasks is the default one.

The name of the tasks invoking Cake is

2018-05-17T07:00:25.0982120Z ==============================================================================
2018-05-17T07:00:25.0996010Z Task         : Shell Script
2018-05-17T07:00:25.1010600Z Description  : Run a shell script using bash
2018-05-17T07:00:25.1024110Z Version      : 2.1.3
2018-05-17T07:00:25.1037600Z Author       : Microsoft Corporation
2018-05-17T07:00:25.1051230Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkID=613738)

Should I change the tasks regex to match "Shell Script"?

Thank you, Cosmin

almtcger commented 6 years ago

Good morning!

Yes, please do so. I totally forgot to mention that.

René

cosminstirbu commented 6 years ago

I've used

Warning Filters: /^.+\(\d+(,\d+)*\):\s+warning Task Filters: /(Shell Script|Invoke Cake)

In the build logs for Build Warnings I can see:

2018-05-17T07:14:53.0859950Z ##[section]Starting: Build Warnings
2018-05-17T07:14:53.0921500Z ==============================================================================
2018-05-17T07:14:53.0935540Z Task         : Build Quality Checks
2018-05-17T07:14:53.0949410Z Description  : Breaks a build based on quality metrics like number of warnings or code coverage.
2018-05-17T07:14:53.0963350Z Version      : 4.0.3
2018-05-17T07:14:53.0977020Z Author       : Microsoft Premier Services
2018-05-17T07:14:53.0993770Z Help         : [[Docs]](https://github.com/MicrosoftPremier/VstsExtensions/blob/master/BuildQualityChecks/en-US/overview.md)
2018-05-17T07:14:53.1008340Z ==============================================================================
2018-05-17T07:14:53.5968420Z SystemVssConnection exists true
2018-05-17T07:14:54.2714230Z Validating build warnings policy...
2018-05-17T07:14:54.4302500Z Counting warnings from tasks:
2018-05-17T07:14:54.4316750Z - Invoke Cake
2018-05-17T07:14:55.0596170Z Total warnings: NaN
2018-05-17T07:14:55.0609740Z Filtered warnings: NaN
2018-05-17T07:14:55.3622430Z Found baseline build with ID 376.
2018-05-17T07:14:55.5240840Z Counting warnings from tasks:
2018-05-17T07:14:55.5256080Z - Invoke Cake
2018-05-17T07:14:56.2392740Z Total warnings: NaN
2018-05-17T07:14:56.2406730Z Filtered warnings: NaN
2018-05-17T07:14:56.4000060Z ##[section]Finishing: Build Warnings

So I'm not sure what I'm missing.

almtcger commented 6 years ago

Hi Cosmin,

I think I made a mistake and there is a small bug in the task that we're now running into. My warning filter expression contains a capturing group and we have a special logic that evaluates capturing groups in order to read the total number of warnings from a log file. When that capturing group captures anything but a number, the warning analysis should ignore it and here is where the bug comes into play. It simply evaluates to NaN, which doesn't help at all. I'll fix the bug with the next update.

To get you unblocked, please change the expression to a non-capturing group like this: /^.+\(\d(?:,\d+)*\):\s+warning/i

That should fix the issue. René

cosminstirbu commented 6 years ago

I think we're making progress, but still something doesn't add up

2018-05-17T14:43:36.0866720Z ##[section]Starting: Build Warnings
2018-05-17T14:43:36.0926630Z ==============================================================================
2018-05-17T14:43:36.0940160Z Task         : Build Quality Checks
2018-05-17T14:43:36.0953550Z Description  : Breaks a build based on quality metrics like number of warnings or code coverage.
2018-05-17T14:43:36.0970300Z Version      : 4.0.3
2018-05-17T14:43:36.0991820Z Author       : Microsoft Premier Services
2018-05-17T14:43:36.1009470Z Help         : [[Docs]](https://github.com/MicrosoftPremier/VstsExtensions/blob/master/BuildQualityChecks/en-US/overview.md)
2018-05-17T14:43:36.1024250Z ==============================================================================
2018-05-17T14:43:36.6414110Z SystemVssConnection exists true
2018-05-17T14:43:39.0434370Z Validating build warnings policy...
2018-05-17T14:43:39.5088610Z Counting warnings from tasks:
2018-05-17T14:43:39.5108820Z - Invoke Cake
2018-05-17T14:43:39.9344320Z Total warnings: 0
2018-05-17T14:43:39.9357890Z Filtered warnings: 272
2018-05-17T14:43:40.3825220Z Found baseline build with ID 377.
2018-05-17T14:43:40.8091190Z Counting warnings from tasks:
2018-05-17T14:43:40.8106250Z - Invoke Cake
2018-05-17T14:43:42.4689930Z Total warnings: 0
2018-05-17T14:43:42.4703660Z Filtered warnings: 272
2018-05-17T14:43:42.5056050Z ##[section]Finishing: Build Warnings
cosminstirbu commented 6 years ago

screen shot 2018-05-17 at 5 59 54 pm

almtcger commented 6 years ago

Hi Cosmin,

I know it looks strange, but this is actually by design. As mentioned, the task usually picks up the warnings that have been logged as build issues. If you specify warning filters, you usually only want to monitor specific warnings (e.g., just code analysis warnings). Thus, the result should show something like 5 out of 10 warnings.

In your case there are no build issues and the total number is always zero. Therefore, you see something like 5 out of 0 warnings. We cannot simply add the filtered warnings to the total warnings, as this would count warnings multiple times if they have been logged as build issues.

I still have an item on our backlog to fix this and create a result that appears to make more sense :) Perhaps we simply use the number of filtered warnings as the total if total is lower than filtered. Might not be correct, but seems better than the current solution.

That being said, the policy should still work for you now, even if the output looks a bit weird.

René

cosminstirbu commented 6 years ago

Thank you for your response.

However it's not clear why it says that Warnings haven't changed since last build.

Also, is there a way to see the warnings themselves, similar to:

https://brakemanpro.com/images/jenkins/warning_file.png (taken from https://brakemanpro.com/docs/ci/jenkins)

Thank you, Cosmin

almtcger commented 6 years ago

Hey,

currently, we only support warnings analysis for MSBuild and VSBuild tasks (see https://github.com/MicrosoftPremier/VstsExtensions/blob/master/BuildQualityChecks/en-US/WarningsPolicy.md#statistics), since we have to fully understand the structure of the log file to analyze the warnings. When we do a generic analysis, we simply count warnings but do not collect any additional information about the warnings. Thus, the analysis always reports that warnings haven't changed. If you send me the log file of your CAKE build, I can see if we might add a bit more logic to fully analyze the warnings issued by the internal MSBuild call as well. Since it is just MSBuild it shouldn't be that hard to do it. The log format seems to be just slightly different from what our MSBuild and VSBuild tasks emit.

The answer to your second question is yes and no. If you used the MSBuild/VSBuild task to build your project file and warnings had been logged as build issues, the build system itself generates links to those warnings in source control. The result looks similar to your screenshot. There are two caveats, though:

  1. The issue list only contains a limited number of warnings, thus, you may not see all of them as links to source control.
  2. It only works for a few technologies.

Our task itself does not offer this feature. We are thinking about extending our warning analysis in the future to at least list the actual warnings that are new since the last build. Those warnings would have links to version control as well. I cannot give you an estimate about when we will have this, though, as we haven't really planned that work yet. The next features will target the pull request process and having our task emit pull request comments about warnings and code coverage. The work we do for that will be the foundation for a better warning analysis. At the moment all I can say is: stay tuned for the upcoming changes.

René

cosminstirbu commented 6 years ago

Hi,

Thank you for your in depth answer. What would be a good non public way for me to share the CAKE logs with you?

As for question number two.

If we would go down the initial suggested way, of wrapping Cake using a .NET project so we can build it with MSBuild task, would we get the source linking and the new warnings diff out of the box?

Thank you, Cosmin

almtcger commented 6 years ago

You can send in the logs using our official support address PSGerExtSupport@microsoft.com.

And yes, I believe that our MSBuild task would simply pick up all warnings logged by the internally called MSBuild process and publish them as build issues. I've never worked with CAKE but perhaps you can create a small sample with an MSBuild wrapper to make sure it works before you invest time into your actual builds. If it works, you should also get our MSBuild warning analysis as well and see statistics based on code files.

René

cosminstirbu commented 6 years ago

I've sent out an email with the Cake Build Logs subject.

In the meanwhile I'll have a look at wrapping the cake build invocation with a .NET project.

Thank you, Cosmin

cosminstirbu commented 6 years ago

Hi @almtcger,

I've ended up using https://github.com/cake-contrib/Cake.Issues.MsBuild to parse the warnings and manually report them to VSTS so they are picked up by your extension.

Things work out nicely, warnings are now picked up and my builds fail if I introduce new ones.

Not sure if there is a way that the extension can also provide some sort of Source Code navigation, but if not, that's fine, as what I'm actually interested in is failing the build if a new warning is introduced.

You can follow the conversation that lead me leveraging both the extension and the Cake Addin here - https://github.com/cake-contrib/Cake.Issues.MsBuild/issues/20

Thank you for your help.

Regards, Cosmin

almtcger commented 6 years ago

Thank you, Cosmin, for the update!

I'll look into the conversation you mentioned and also take a look at the Cake.Issues.MSBuild project. I promise to still check if some tweaks to our MSBuild log parsing logic might capture issues in you scenario without the need for additional tools.

I'm closing this issue for now. If you have any additional feedback, questions, or issues, please feel free to open another one or contact us via email.

René