-
Notifications
You must be signed in to change notification settings - Fork 124
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
Subtle fix for ament_libraries_deduplicate tests #516
Conversation
562e76a
to
e0ef5ce
Compare
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.
Is this configuration list something ament invented, or does it come from somewhere else?
I see the ament_libraries_pack_build_configuration
macro also looks for optimized
:
ament_cmake/ament_cmake_libraries/cmake/ament_libraries_pack_build_configuration.cmake
Line 33 in e0ef5ce
if("${_lib}" MATCHES "^(debug|optimized|general)$") |
Bread crumb for future readers: the keywords seem to come from the
|
Evidently 'release' isn't a config keyword, but 'general' is. I also modified the test to assert that the keyword is actually treated like a keyword. Signed-off-by: Scott K Logan <logans@cottsay.net>
e0ef5ce
to
8840e2e
Compare
Since "lib" = "general:lib", should the library deduplicate process transform "lib;general;lib" into "lib" (or "general;lib"?) I don't think it is the case at the moment |
I believe you're correct on both points. That additional deduplication could be viewed as a further optimization, separate from the additional tests and pending performance optimization. |
Evidently 'release' isn't a config keyword, but 'general' is. I also modified the test to assert that the keyword is actually treated like a keyword.
We just got lucky that putting 'release' in that particular spot and treating it like a library name resulted in the same behavior as if it were a build config keyword.