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.

Sign up for GitHub

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

Jump to bottom

avoid printing on stderr if linter successful #88

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

avoid printing on stderr if linter successful #88

wants to merge 3 commits into from

Conversation

mikaelarguedas
Copy link
Member

Hey there 👋

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 ?

@peci1
Copy link
Contributor

peci1 commented Mar 8, 2024

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.

@mikaelarguedas
dont do bash before coffee
2637886 
Signed-off-by: Mikael Arguedas <[email protected]>
@mikaelarguedas
Copy link
Member Author

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

peci1
peci1 reviewed Mar 8, 2024
View reviewed changes
scripts/test_wrapper Outdated Show resolved Hide resolved
peci1
peci1 reviewed Mar 8, 2024
View reviewed changes
scripts/test_wrapper Outdated Show resolved Hide resolved
peci1
peci1 suggested changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@peci1 peci1 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please add echo "$LINTER_OUTPUT" to the success branch. I think it's important to see any messages from the linter. Printing them to stdout is compatible with catkin_tools so that they don't treat it as a warning.

An example of useful logs from a succeeded linter can be found e.g. at the top of this build: https://build.ros.org/job/Ndev__cras_ros_utils__ubuntu_focal_amd64/86/console .

I haven't figured if printing to stdout could break something else, though.

@mikaelarguedas
Copy link
Member Author

@arne48 @mikepurvis friendly 🛎️ , WDYT about this workaround ? do you think it can make it in the codebase ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peci1 peci1 peci1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mikaelarguedas @peci1