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

Softer checks for compiler warnings in CI #1842

Closed
wants to merge 1 commit into from

Conversation

heplesser
Copy link
Contributor

The recently introduced check for the number of compiler warnings appears too strict, since in particular the inclusion of external libraries can lead to large numbers of warnings with the exact number depending on details of header file inclusion. This leads to failing CI tests for rather obscure reasons and is thus counterproductive.

This PR addresses this in two ways:

  1. The number of warnings is only checked when building without external dependencies. In this case we have 8 warnings at present; the test now accepts any number of warnings <= 8.
  2. NEST is built (on Travis) with warnings=OFF if built with MPI, MUSIC or libneurosim as these cases trigger a large number of warnings, leading to unnecessarily bloated build logs.

@lekshmideepu This will solve the problem causing #5437.5 to fail for PR #1839.

…xternal libraries.

Test number of warnings only for cases without such libraries and then check upper bound.
@heplesser heplesser added T: Bug Wrong statements in the code or documentation S: Critical Needs to be addressed immediately I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Nov 12, 2020
@heplesser heplesser requested review from jougs and hakonsbm November 12, 2020 21:28
@heplesser heplesser self-assigned this Nov 12, 2020
@jougs
Copy link
Contributor

jougs commented Nov 12, 2020

@heplesser: thanks for the fix.

We discussed this issue extensively during the hackathon today and came to a similar but still somewhat different conclusion. The approach we agreed on is to adapt the detection of warnings even if using external libraries so that we only catch those coming from the NEST source code proper. I have already started to implement this. I will now rebase my work onto your branch, so your efforts are not completely lost. Once I'm ready, I'll be sending a PR towards your branch.

In order to prevent a premature merge of this PR, I'm converting to draft for now.

@jougs jougs marked this pull request as draft November 12, 2020 21:54
@heplesser
Copy link
Contributor Author

@jougs I did wonder if you had not discussed it already ;). Turing warnings off for the full build, the log shrinks from 30000 to 10000 lines and test pass, although one does get the slightly confusing message

Warnings: 289 (1000000000 expected)

BTW, I accidentally restarted the .5 build on Travis. Until that is finished, you can see the result in the build against my repo (https://travis-ci.org/github/heplesser/nest-simulator/jobs/743277720).

@heplesser
Copy link
Contributor Author

Closing this one, #1852 is a much better solution.

@heplesser heplesser closed this Nov 16, 2020
@heplesser heplesser deleted the reduce-warning-sensitivity branch February 25, 2021 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Critical Needs to be addressed immediately T: Bug Wrong statements in the code or documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants