-
Notifications
You must be signed in to change notification settings - Fork 18
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
make check: check for parallel/wget/curl, fix #157 #158
Conversation
.PHONY: all modules check clean help show always-update | ||
|
||
check: | ||
@sem --version >/dev/null && echo "ok - parallel installed" |
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.
Then an error would be reported like this:
/bin/sh: 1: sem: not found
.PHONY: all modules check clean help show always-update | |
check: | |
@sem --version >/dev/null && echo "ok - parallel installed" | |
.PHONY: all modules check clean help show always-update | |
check: | |
@sem --version >/dev/null 2>&1 || echo "error - please run 'sudo make deps-ubuntu' or install GNU parallel manually" |
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 have no strong opinion regarding the success message, so ok - parallel installed
could be added for the success case if desired.
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 don't see the point. You already get appropriate error messages now when one of these tools is missing. No need to duplicate that under a new target. It will only confuse users. Other missing dependencies will produce errors when due, respectively.
|
||
check: | ||
@sem --version >/dev/null && echo "ok - parallel installed" | ||
@{ wget --version || curl --version; } > /dev/null && echo "ok - wget/curl installed" |
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.
As we already have hard dependencies on wget
, cleaning the current code would be reasonable - no need to support curl
as well.
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 don't see the point. You already get appropriate error messages now when one of these tools is missing. No need to duplicate that under a new target. It will only confuse users. Other missing dependencies will produce errors when due, respectively.
I see this more as a documentation feature. If users do encounter problems, you could ask them: "What does make check
say?" to quickly exclude missing dependencies. I'd say that is easier than capturing make all
output and spot the error.
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.
> As we already have hard dependencies on
wget, cleaning the current code would be reasonable - no need to support
curl` as well.
Since the $(WGET)
macro checks wget --version
, then curl --version
, make check
should also check in that order. The ||
should stop early if wget is available and test whether curl is available only if wget isn't.
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 see this more as a documentation feature. If users do encounter problems, you could ask them: "What does
make check
say?" to quickly exclude missing dependencies. I'd say that is easier than capturingmake all
output and spot the error.
What do you gain? – Nothing. Whether wget and parallel is missing is already reported by make all
– at the very bottom (unless running parallel and for the first time).
What do you loose? – All the other possible sources of error. You'll usually have to ask for make all
output anyway.
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.
Cleaning the WGET
macro to only use wget
is a separate task which can be done before or after this PR (but please in a separate PR). I just wanted to say that there is no need to support curl
here and to remove that later again.
make deps-ubuntu
installs wget
.
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.
Error messages should be added / improved, and instead of checking for curl or wget, a check for wget only would be better.
Just to note that |
As far as I remember, a default installation of Ubuntu already includes |
We have since introduced a different |
No description provided.