Remove deprecated Warnings plugin

[rotu]

Fixes #315

[dirk-thomas]

With this patch applied how are we being informed if there are compiler warnings, CMake warnings, etc?

[rotu]

By looking at the output and the failure status of CI. If it's severe enough to treat it as an error (like a missing space via ament_uncrustify), it's certainly worth adding it in test coverage.

It would also be reasonable to add support in a future PR for another plugin such as https://github.com/jenkinsci/warnings-ng-plugin/blob/master/doc/Documentation.md

[dirk-thomas]

By looking at the output

That doesn't sounds like a reasonable substitution to me. It is unlikely that someone reads the full output if the build comes back green.

the failure status of CI

Without this plugin I would expect the build to be green even if it has CMake / compiler warnings? Not sure I understand this correctly.

it's certainly worth adding it in test coverage.

How would this be actually done?

It would also be reasonable to add support in a future PR for another plugin

Atm the warning plugins gives us visibility into CMake / compiler warnings. Without it we don't have any notification if those occur. Without a proper working replacement I don't think this should be removed.

[rotu]

1. It’s totally reasonable to look at your build output. Especially since these are not issues that we haven’t deemed worthy of being build errors.

2. Compiler issues can and should be caught by clang format, clang tidy, and other such tools.

3. Okay. How could I test such a PR?

[dirk-thomas]

It’s totally reasonable to look at your build output.

At the scale of PRs / CI builds we are facing I disagree on that. I have to be able to rely on the CI status to tell if there is anything wrong I should look into.

Compiler issues can and should be caught by clang format, clang tidy, and other such tools.

I don't think that is true for CMake warnings.

[rotu]

I think you’re right this bears more thought. I transitioned things over basically as-is to warnings-ng in #359.

[wjwwood]

Closing in favor of #359

[rotu]

@wjwwood why did you close this? This PR is still needed

[wjwwood]

Unless I misunderstand, we won't be separately removing and adding the other warnings plugin. We would only remove and replace it all at once. Please consolidate this to one pull request, i.e. remove the current plugin in the other pull request.

If @dirk-thomas disagrees, I can reopen this.

[rotu]

Then merge them at the same time. I personally would prefer to have overlapping coverage for a short period.

[dirk-thomas]

We can't run CI for a change which spreads across two separate branches / PRs.

[rotu]

Ah that makes sense. Merged into #359 and rebased