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

Bugfix: CPPCHECK causes the Travis CI build to fail #668

Merged
merged 8 commits into from Jun 12, 2017

Conversation

@gtrensch
Copy link
Collaborator

gtrensch commented Mar 2, 2017

This PR fixes a bug introduced with PR #546 commit bf46de3.

This change activates CPPCHECK again. All static code analysis messages will appear in the log and be collected by the parsing script. The result is "correctly" ignored now. Additionally, duplicate lines have been removed from build.sh.

- Remove duplicate lines in build.sh.
Copy link
Contributor

heplesser left a comment

@gtrensch I think we need to think more about how we handle activation/inactivation of tests, see my comments below.

#
# (status_cppcheck_init is None or status_cppcheck_init) and \
# (number_of_msgs_in_summary(summary_cppcheck) == 0) and \

This comment has been minimized.

@heplesser

heplesser Mar 2, 2017 Contributor

It is dangerous to have on the one side the "PERFORM_CPPCHECK=true" flag and then hide here in a comment that the check is not actually active. The PERFORM_CPPCHECK flag should fully control whether CPPCHECK is performed, displayed and taken into account when deciding whether the build succeeded or not (and similarly for the other checks).

I know we did it differently before, but running a check that no-one really looks at (who reads Travis logs except to find the reason of failure?) seems rather like a waste of resources. So I think off should be fully off.

This comment has been minimized.

@jougs

jougs Mar 3, 2017 Contributor

I actually think that there is a benefit in running checks and having their output in the log but ignoring it for determining the build status.

However, I agree with @heplesser that hiding this in a comment might not be the best solution. Another approach would be to explicitly have skipping_<checker> flags for all style checkers and use those in l.902ff. The (deactivated) printout could then also be be added automatically (and called ignored).

This comment has been minimized.

@gtrensch

gtrensch Mar 3, 2017 Author Collaborator

@heplesser, @jougs I go along with your arguments but disagree slightly with the conclusion.

In my opinion, the best solution is to have no switches at all and this should be the goal. For the command-line tool they are useful but not on Travis. We have strengthened the static code analysis and put this now "in production". We fully turned on VERA, wich was not the case before, and found that CPPCHECK is very critical. We do not fully oversee the impact. I see this as a transision phase. The current, simle implementation helps to handle this without doing major code changes.

I think for CPPCHECK we need to collect information on what the impact would be. This PR therefore activates it and we run through all the logic needed for the evaluation. Just the exit code is skipped. If we turn it fully off, it probbaly will never be turned on and we have dead code.

I would volunteer watching the builds to get a clearer picture of the impact CPPCHECK has. Later, at some point in time, we need to make a descision whether we activate or stay away from it and remove the code. If a descission has once been made and the code analysis runs stable also the switches are not needed any longer. Therefore, I am not sure if it is worth the work adding additional logic to pass arguments through shell scripts and logs to Python.

This comment has been minimized.

@heplesser

heplesser Mar 3, 2017 Contributor

@gtrensch I see your point, and you are probably right that change will only happen one way here, cppcheck being included when evaluating whether a build was successful at some point. I still find this not very tidy. The main issue is how well the code will be understood by someone who was not involved in writing it, when code needs to be maintained later. In spite of our best intentions, cppcheck may still be deactivated five years from now ...

@@ -899,7 +900,6 @@ def build_return_code(status_vera_init,
(status_make_install) and \
(status_tests) and \
(number_of_msgs_in_summary(summary_vera) == 0) and \
(number_of_msgs_in_summary(summary_cppcheck) == 0) and \

This comment has been minimized.

@heplesser

heplesser Mar 2, 2017 Contributor

This should be something like

( not perform_cppcheck or number_of_msgs_in_summary(summary_cppcheck) == 0 )

Also, PEP8 prefers parentheses to continuation lines.

This comment has been minimized.

@jougs

jougs Mar 3, 2017 Contributor

This would become

( skipping_cppcheck or number_of_msgs_in_summary(summary_cppcheck) == 0 )

with my suggestion above. Something similar would happen for the lines below.

(and I would rename the looooong function number_of_msgs_in_summary to simply msg_count)

This comment has been minimized.

@heplesser

heplesser Mar 3, 2017 Contributor

If we decided no to use flags, then this line should stay in place, commented out, and with an accompanying comment explaining why the line is commented out and when it should be activated again.

This comment has been minimized.

@gtrensch

gtrensch Mar 3, 2017 Author Collaborator

@heplesser, @jougs

Fundamentally, I vote for following clean coding guidelines, and having a styleguide is definitely a must. Since you spent lot more time reading code than writing, we want the reading of code to be easy. Therefore, we should be careful in going too far and limit ourselves where it is not needed. For example: The 79 characters per line limitation makes it more difficult to produce good readable code. This discussion is very very old. My personal experience is, that sooner or later people switch to 100 ... 120.

Some of the backslashes are a result of that restriction. I need to change that at several places in the code.
Question: Is there a way to force PEP8 to complain about that ? If so, we should add that to the code analysis.

Concerning the looong function name: It seems that I became a victim of my own naming convention here. A shorter name would still be descriptive enough. I will replace the prefix "number_of_msgs_" by something shorter and also have a look a the names again.

This comment has been minimized.

@jougs

jougs Mar 4, 2017 Contributor

@gtrensch: you convinced me! I agree that we should rather fix the code instead of having an advanced system to disable checks. I also agree with @heplesser's suggestion to leave the line that disables cppcheck for now in as a comment with some explanatory text.

This comment has been minimized.

@gtrensch

gtrensch Apr 5, 2017 Author Collaborator

@heplesser, @jougs finally, I ended up with adding additional logic to be able to ignore certain static code analysis messages or skip the analysis. (5be3fda) This will then be reported with 'Ignored' or 'Skipped', respectively. I found this is the best solution and provides a central point of configuration in 'build.sh'. This information is passed through the log and parsed by the Python parser script.

The current setting is:

  • all checks are turned on
  • CPPCHECK messages are ignored

I also refactored some of the function names and removed the bracktes in the return statements where not needed again.

Copy link
Contributor

jougs left a comment

Thanks for fixing this. I've added some comments and suggestions.

build.sh Outdated
@@ -161,7 +155,7 @@ if [ "$xSTATIC_ANALYSIS" = "1" ] ; then
CLANG_FORMAT=clang-format
PEP8=pep8
PERFORM_VERA=true

This comment has been minimized.

@jougs

jougs Mar 3, 2017 Contributor

I would add an empty newline here to separate the section that defines program names from one that defines flags. Also some comments on that the variables are would be helpful.

This comment has been minimized.

@gtrensch

gtrensch Mar 3, 2017 Author Collaborator

I will add some comments.

This comment has been minimized.

@jougs

jougs Mar 4, 2017 Contributor

Thanks!

This comment has been minimized.

@gtrensch

gtrensch Apr 5, 2017 Author Collaborator

@jougs I have improved this. (08d69f6)

#
# (status_cppcheck_init is None or status_cppcheck_init) and \
# (number_of_msgs_in_summary(summary_cppcheck) == 0) and \

This comment has been minimized.

@jougs

jougs Mar 3, 2017 Contributor

I actually think that there is a benefit in running checks and having their output in the log but ignoring it for determining the build status.

However, I agree with @heplesser that hiding this in a comment might not be the best solution. Another approach would be to explicitly have skipping_<checker> flags for all style checkers and use those in l.902ff. The (deactivated) printout could then also be be added automatically (and called ignored).

@@ -899,7 +900,6 @@ def build_return_code(status_vera_init,
(status_make_install) and \
(status_tests) and \
(number_of_msgs_in_summary(summary_vera) == 0) and \
(number_of_msgs_in_summary(summary_cppcheck) == 0) and \

This comment has been minimized.

@jougs

jougs Mar 3, 2017 Contributor

This would become

( skipping_cppcheck or number_of_msgs_in_summary(summary_cppcheck) == 0 )

with my suggestion above. Something similar would happen for the lines below.

(and I would rename the looooong function number_of_msgs_in_summary to simply msg_count)

@heplesser
Copy link
Contributor

heplesser commented Mar 6, 2017

@gtrensch Could you take a look at Travis build 1716.7? The build fails in that case, but the report then states

| Make                   | Skipped                                       |
|                        |                                               |
|                        | Errors  : None                                |
|                        | Warnings: None                                |
+------------------------+-----------------------------------------------+

Here, Make should report FAILED, and report that there was an error. Could you check the log-parser? I am also a bit surprised that the log reports S3-upload

+------------------------+-----------------------------------------------+
| Amazon S3 upload       | Yes                                           |
+------------------------+-----------------------------------------------+

but this may be correct.

@gtrensch
Copy link
Collaborator Author

gtrensch commented Mar 6, 2017

You are right. There is a compile error in node_manager.cpp and the report should state failed. I will run the parsing script "offline" against that log.

@gtrensch
Copy link
Collaborator Author

gtrensch commented Apr 5, 2017

@heplesser the 'Make status' issue has been fixed with code change 098a686.
I also improved the reporting on the Amazon S3 deployment status (cb1f701). Since the upload is triggert by Travis after a successful build, it cannot be evaluated whether the upload was successful or not. Thus, the log parser script checks for the conditions only which lead to an upload. But I believe Travis itself reports on the deployment at the end of Travis run.

Copy link
Contributor

heplesser left a comment

Just one little improvement left :).

if $IGNORE_MSG_CLANG_FORMAT; then
print_msg "MSGBLD1030: " "IGNORE_MSG_CLANG_FORMAT is set. CLANG_FORMAT messages will not cause the build to fail."
fi
if $RUNS_ON_TRAVIS && $IGNORE_MSG_PEP8; then

This comment has been minimized.

@heplesser

heplesser Apr 5, 2017 Contributor

The $RUNS_ON_TRAVIS is not needed here, it is take care of by the outer if.

This comment has been minimized.

@gtrensch

gtrensch Apr 6, 2017 Author Collaborator

@heplesser yes and no. I added the 'if $RUNS_ON_TRAVES' for 3 reasons.

  • clarity
  • robustness
    If the script runs locally, executed by 'check_code_style.sh', and someone changes its parametrization, for whatever reason, it will still behave reasonable. The ignore flags do not make sense outside Travis.
  • testing
    I used the 'check_code_style.sh' script for testing. Means, you can do the above without getting the second string, e.g. 'IGNORE_MSG_VERA is set. VERA++ .....'.
@gtrensch
Copy link
Collaborator Author

gtrensch commented Apr 6, 2017

@heplesser, @jougs
Important note !

Before merging this PR the "curly braces PR" #691 should be merged. This PR will turn on the VERA check again. CPPCHECK is also on. CPPCHECK messages will appear in the report but they are not taken into account for the build result. This PR does also not add the exclusion list.

gtr
@gtrensch
Copy link
Collaborator Author

gtrensch commented Apr 6, 2017

Set the IGNORE_MSG_VERA flag. See also PR #691.

@heplesser
Copy link
Contributor

heplesser commented Jun 12, 2017

This looks fine now and since #691 has been merged, I will merge this as well.

@heplesser heplesser merged commit 079581a into nest:master Jun 12, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.