-
Notifications
You must be signed in to change notification settings - Fork 107
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
add ament_(cmake_)xmllint packages #104
Conversation
@ros-pull-request-builder retest this please |
Edit: scratch this: I just saw https://github.com/ament/ament_lint/pull/104/files#diff-2b64320c40b951cc04ac6618030de634R89 |
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.
code change LGTM, with one request for additional doc.
Could you add to the documentation what xml syntaxes are supported for defining schemas?
The DDS-Security xml files are currently not being validated against the schema.
If it's useful:
DDS-Security permission xsd: https://www.omg.org/spec/DDS-SECURITY/20170901/omg_shared_ca_permissions.xsd
DDS-Security permission xml example: https://www.omg.org/spec/DDS-SECURITY/20170901/omg_shared_ca_permissions_example.xml
Issue tracking the fact that the generated XMLs are not valid: ros2/sros2#49
find_program(xmllint_BIN NAMES "xmllint") | ||
|
||
if(NOT xmllint_BIN) | ||
if(${CMAKE_VERSION} VERSION_LESS "3.8.0") |
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.
what is the cmake 3.8 feature being used here ? From @dirk-thomas DISABLED
used L66 is the answer
The commit d874409 adds support for specifying the schema in the |
Arg that doesn't look good. I suggest to revert / remove the last commit and merge this as is. A follow-up ticket / PR can be opened with a reference to the commit so that we can look into reenabling it when we find time to look into it. |
The url |
This was just copied from the CI job output. Just tried locally and the error is the same with the https URL:
|
Can you try it with |
With the downloaded schema it works for me: |
Fails: with
But this works:
I observe the same behavior with their new schema: https://www.omg.org/spec/DDS-SECURITY/20170901/omg_shared_ca_governance.xsd |
The problem is actually not the redirect. Do you want to find the same schema on a http url and use that, or host the schema on http://download.ros.org/schema/ or should we disable the linter for that package? |
Let's disable that package for now as it would fail the test even if it was finding the schema properly. We can revisit later, either use a linter that supports https or a non-secured hosting solution for these xsds |
Done in ros2/system_tests#287. |
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.
👍
Follow up commit 0ad4e0f to avoid stderr output. |
@dirk-thomas These linter tests ran on MacOS last night (I was not expecting it based on the PR description). |
|
Does this mean that the package I'm asking because we will need this information to update the node setup instructions and eventually user install instructions |
I have not touched any macOS node. It seems the executable was already available.
I didn't see a choco package but didn't look for long since it should be sufficient if one platform runs the linter. |
https://ci.ros2.org/job/ci_linux/4876/testReport/ as an example of failed linting.
If
xmllint
is not available (as it is the case on the non-Linux Jenkins nodes) the tests are being skipped: https://ci.ros2.org/job/ci_linux/4875/testReport/junit/(root)/projectroot/With other PRs merged this passes CI: