roslint icon indicating copy to clipboard operation
roslint copied to clipboard

avoid printing on stderr if linter successful

Open mikaelarguedas opened this issue 11 months ago • 3 comments

Hey there :wave:

Thanks for this utility! I'm experimenting it to invoke catkin_lint as described in https://discourse.ros.org/t/ros1-now-is-a-great-time-to-add-catkin-lint-to-your-packages/36521 To be precise I'm invoking it like this roslint_custom(catkin_lint -W2 --strict -q .)) An issue I face is that it is generating a catkin_tools warning on each package it is used even if the linter passes.

This PR is a workaround for this issue. I guess it doesnt have a strong impact as this pipe was not present in earlier version of ROS 1 (https://github.com/ros/roslint/pull/79/files#diff-75b2bc3dc9e3948395dbe27abe404923f62e0fbf445bdada52b0ce5264b4b186) but I actually have no idea of the potential impact.

Catkin build logs without and with this change

Without this change:

==> Expanding alias 'run_tests' from 'catkin run_tests test_pkg' to 'catkin test test_pkg'
Starting  >>> test_pkg                                                                                                                                                                              
__________________________________________________________________________________________________________________________________________________________________________________________________________
Warnings   << test_pkg:make /home/mikael/dev/catkin_ws/logs/test_pkg/test.make.014.log                                                                                                        
-- run_tests.py: execute commands with working directory "/home/mikael/dev/catkin_ws/build/test_pkg"
  /opt/ros/noetic/share/roslint/cmake/../../../lib/roslint/test_wrapper /home/mikael/dev/catkin_ws/build/test_pkg/test_results/test_pkg/roslint-test_pkg.xml make roslint_test_pkg
Built target roslint_test_pkg
cd /home/mikael/dev/catkin_ws/build/test_pkg; catkin test --get-env test_pkg | catkin env -si  /usr/bin/make run_tests; cd -

..........................................................................................................................................................................................................
Output     << test_pkg:results /home/mikael/dev/catkin_ws/logs/test_pkg/test.results.014.log                                                                                                  
Summary: 1 tests, 0 errors, 0 failures, 0 skipped
cd /home/mikael/dev/catkin_ws/build/test_pkg; catkin test --get-env test_pkg | catkin env -si  catkin_test_results; cd -
                   
Finished  <<< test_pkg          [ 0.7 seconds ]                                                                                                                                                     
[test] Summary: All 1 packages succeeded!                                                                                                                                                                 
[test]   Ignored:   None.                                                                                                                                                                                 
[test]   Warnings:  1 packages succeeded with warnings.                                                                                                                                                   
[test]   Abandoned: None.                                                                                                                                                                                 
[test]   Failed:    None.                                                                                                                                                                                 
[test] Runtime: 0.7 seconds total.                        

With this change

==> Expanding alias 'run_tests' from 'catkin run_tests test_pkg' to 'catkin test test_pkg'
Starting  >>> test_pkg                                                                       
Output     << test_pkg:make /home/mikael/dev/catkin_ws/logs/test_pkg/test.make.013.log 
-- run_tests.py: execute commands with working directory "/home/mikael/dev/catkin_ws/build/test_pkg"
  /opt/ros/noetic/share/roslint/cmake/../../../lib/roslint/test_wrapper /home/mikael/dev/catkin_ws/build/test_pkg/test_results/test_pkg/roslint-test_pkg.xml make roslint_test_pkg
cd /home/mikael/dev/catkin_ws/build/test_pkg; catkin test --get-env test_pkg | catkin env -si  /usr/bin/make run_tests; cd -

Output     << test_pkg:results /home/mikael/dev/catkin_ws/logs/test_pkg/test.results.013.log
Summary: 1 tests, 0 errors, 0 failures, 0 skipped
cd /home/mikael/dev/catkin_ws/build/test_pkg; catkin test --get-env test_pkg | catkin env -si  catkin_test_results; cd -

Finished  <<< test_pkg          [ 0.3 seconds ]                                              
[test] Summary: All 1 packages succeeded!                                                          
[test]   Ignored:   None.                                                                          
[test]   Warnings:  None.                                                                          
[test]   Abandoned: None.                                                                          
[test]   Failed:    None.                                                                          
[test] Runtime: 0.3 seconds total.   

@peci1 FYI as I may be using this wrong and maybe there is a better way around it ?

mikaelarguedas avatar Mar 08 '24 13:03 mikaelarguedas

This fix is wrong. The $@ part is what is actually calling the linter. If you move it to a later part, then you're basically testing the success of some previous command called before.

I think it might be better to write the output to a temporary file. When the linter is done, check if the file is nonempty. If it is, write it to stderr, otherwise do nothing.

peci1 avatar Mar 08 '24 13:03 peci1

This fix is wrong. The $@ part is what is actually calling the linter. If you move it to a later part, then you're basically testing the success of some previous command called before.

oh wow, of course, you're right

mikaelarguedas avatar Mar 08 '24 14:03 mikaelarguedas