Skip to content
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

fix: Correct tool existence checks #1145

Closed
wants to merge 1 commit into from

Conversation

pulecp
Copy link

@pulecp pulecp commented Nov 3, 2023

command -v <cmd> returns a path to a binary to stdout if it's found in PATH. The condition deciding if <cmd> is installed redirects stdout to /dev/null and so this check could never work.

This change redirects stderr to /dev/null instead of stdout to compare the output of command -v <cmd> properly with an empty string.

I unified the redirection to a file in whole file. The empty space behind redirection is not needed.

Signed-off-by: Pavel Pulec [email protected]

@pulecp pulecp changed the title fix: Correct yamllint's existence check fix: Correct cmd existence checks Nov 3, 2023
@pulecp pulecp changed the title fix: Correct cmd existence checks fix: Correct tool existence checks Nov 3, 2023
`command -v <cmd>` returns a path to a binary to stdout if it's found in PATH.
The condition deciding if `<cmd>` is installed redirects stdout to `/dev/null`
and so this check could never work.

This change redirects stderr to `/dev/null` instead of stdout to compare the
output of `command -v <cmd>` properly with an empty string.

I unified the redirection to a file in whole file. The empty space behind
redirection is not needed.

Signed-off-by: Pavel Pulec <[email protected]>
@electron0zero
Copy link
Member

hi @pulecp Makefile.common is managed in https://github.com/prometheus/prometheus repo, and synced here by automation.

please make this change in upstream. once merged in prometheus/prometheus, automation will sync it here.

@electron0zero
Copy link
Member

thanks for the PR but I am going to close this PR because we can't merge this change here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants