-
Notifications
You must be signed in to change notification settings - Fork 277
Don’t try to validate trace XML when xmllint not found #5452
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
Don’t try to validate trace XML when xmllint not found #5452
Conversation
This was the intended behaviour to begin with, I just forgot to put the ‘exit’ there.
@@ -13,6 +13,7 @@ fi | |||
xmllint="$(command -v xmllint)" | |||
if [ $? -ne 0 ] > /dev/null; then | |||
echo "xmllint not found, skipping XSD tests" | |||
exit |
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.
It seems to me that this should exit with an error code, as we haven't tested successfully if xmllint
is missing.
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.
I'm a fan of this idea - even though it looks to me like we have checked if xmllint
was found, which is what we care for here, isn't it?
@@ -13,6 +13,7 @@ fi | |||
xmllint="$(command -v xmllint)" |
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.
Also, for something unrelated to this PR, but why is command -v
used here instead of whereis
? Are there any benefits?
I don't mind it, it was just surprising to me - never seen this before, and for this functionality, I've always seen whereis
being used instead.
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.
command
is very simple. It just checks if something is on $PATH
, optionally returns the path to it and gives me an error code if can’t be found. whereis
seems to be a more complicated tool used for actually locating things, which is not what I’m trying to do.
It also doesn’t fail by default if it can’t find anything and gives me ton of different things if it does. For me the question is more the other way round, is there a reason to use whereis
when command
will do?
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.
Looks good overall, only would ask if it would be possible to get an error code returned before it gets merged.
@NlightNFotis I'd prefer to keep this consistent with the other cases right now. Having this error out if the prereqs aren't there would be a much bigger change, as it would involve basically changing most of our CI scripts. |
This was the intended behaviour to begin with, I just forgot
to put the ‘exit’ there.