-
Notifications
You must be signed in to change notification settings - Fork 43
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
linter invocation fixup #95
Conversation
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using just ament_lint_cmake
seems reasonable to me in this case.
sros2_cmake/CMakeLists.txt
Outdated
@@ -7,10 +7,8 @@ set(LIB_INSTALL_DIR lib/) | |||
set(INCLUDE_INSTALL_DIR include/) | |||
set(SYSCONFIG_INSTALL_DIR share/${PROJECT_NAME}) | |||
|
|||
find_package(ament_cmake_test REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command sets the build BUILD_TESTING
flag.
If we remove it, we should do something else to enable testing (I think enable_testing()
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is much more logic to it than just calling enable_testing()
: see https://github.com/ament/ament_cmake/blob/207a07b72af1e5a780887d7c2268df57ef456af3/ament_cmake_test/ament_cmake_test-extras.cmake#L17-L37 so I don't think duplicating that logic makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e4ee571 applies suggestion from #90 (comment)
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
* remove unecessary ament_cmake_test Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * remove ament_lint_auto dependency Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * add back ament_cmake_test with comment Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
* remove unecessary ament_cmake_test Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * remove ament_lint_auto dependency Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com> * add back ament_cmake_test with comment Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Follow up of #90
ament_cmake_test
find_package call.ament_lint_auto
altogether and only depends onament_cmake_lint_cmake
.Second commit is more subjective, as this is not an ament package, each linter has to be called explicitly. At that point it seems that relying on
ament_lint_auto
loses its benefits while adding dependencies and CMake code. As this package is meant to only provide pure cmake macros it looks likeament_lint_cmake
is the only linter it will need going forward .Happy to revert the second commit if the ament_lint_auto approach is preferred.