(cmake_xmllint) make extensions configurable #479
Conversation
|
Friendly ping @clalancette |
1 similar comment
|
Friendly ping @clalancette |
|
CI Issues are related to https://bugs.launchpad.net/ubuntu/+source/python-flake8/+bug/2012523 |
Signed-off-by: Matthijs van der Burgh <[email protected]>
| @@ -23,7 +23,7 @@ | |||
| # @public | |||
| # | |||
| function(ament_xmllint) | |||
| cmake_parse_arguments(ARG "" "MAX_LINE_LENGTH;TESTNAME" "" ${ARGN}) | |||
| cmake_parse_arguments(ARG "" "TESTNAME" "PATHS;EXCLUDE;EXTENSIONS" ${ARGN}) | |||
There was a problem hiding this comment.
It doesn't look like either PATHS nor EXCLUDE are being used here, so let's just drop them.
There was a problem hiding this comment.
I will check whether I want to add these as well or remove them.
There was a problem hiding this comment.
I have update the macro to also use the PATHS and EXCLUDE args. (Though these are not use in the ament_lint_auto hook. As it looks for files, which eventually will be used by ament_xmllint. But then you are recreating the logic from ament_xmllint in CMake, which is undesired. But I don't think you want to call the python function from the ament_xmllint library here.
| @@ -12,8 +12,26 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
|
|
|||
| file(GLOB_RECURSE _source_files FOLLOW_SYMLINKS "*.xml") | |||
| # Forces ament_xmllint to consider ament_cmake_xmllint_EXTENSIONS as the given extensions if defined | |||
| set(_extensions "xml") | |||
There was a problem hiding this comment.
This feels a little weird. If the user doesn't specify anything for EXTENSIONS, then we won't pass --extensions to ament_xmllint at all, and then that script will choose the xml extension itself. If the user specifies some EXTENSIONS, then we will always pass --extensions xml, plus whatever they choose. There is no way for the user to ask for extensions that doesn't include xml.
I think we should do one of two things here:
- If the user specifies EXTENSIONS, then make that the complete set. Don't silently add
.xml. - Rename the variable from EXTENSIONS to EXTENSIONS_APPEND or something like that, to make it clear that we will always do at least xml.
Either way, we should document what we are doing in the documentation to ament_xmllint.
There was a problem hiding this comment.
No, I checked the behaviour. When setting ament_cmake_xmllint_EXTENSIONS, it will completely overwrite the _extensions variable. As I use REGEX REPLACE.
Signed-off-by: Matthijs van der Burgh <[email protected]>
Signed-off-by: Matthijs van der Burgh <[email protected]>
|
@clalancette friendly ping ;) |
1 similar comment
|
@clalancette friendly ping ;) |
Make it possible to configure the file extensions to be checked by xmllint from CMake.
This can be done by setting the
ament_cmake_xmllint_EXTENSIONSas a space or comma separated string of extensions when usingament_lint_auto. Or by passing theEXTENSIONStoament_xmllintfunction directly.