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

Add system make installation check #25

Closed
wants to merge 2 commits into from
Closed

Add system make installation check #25

wants to merge 2 commits into from

Conversation

dungru
Copy link

@dungru dungru commented May 9, 2021

Give a user a prompt when system make not install yet.

Give a user a prompt that system make not install yet.
scripts/init Outdated
# find make
MAKE=$(which make 2>/dev/null)

if (( $? == 1 )); then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like this to be fenced by an environment variable, like $DEVENVVERBOSE or $DEVENVDEBUG. I think most of the time we would like devenv to be lean.

@yungyuc yungyuc added the enhancement New feature or request label May 9, 2021
Copy link
Collaborator

@tai271828 tai271828 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some coding style bike-shedding for your reference.

MAKE=$(which make 2>/dev/null)

if (( $? == 1 )); then
echo " "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

echo " " echo an empty space is equivalent to echo "". Do you prefer which one?

if (( $? == 1 )); then
echo " "
echo "ERROR: make needs to be installed to run devenv. "
echo " Install make using the command such as ' apt install build-essential' as root and try again. "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure the leading and trailing spaces are meant to be. Besides, ' apt install build-essential' has a leading space as well. Are they typo or on purpose?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's on purpose. Actually the end of apt install build-essential needs space, too.
The space between command and quotes is avoiding the user copy the single quote
The purpose of the leading space that means this line is to follow the previous line error message.

echo " "
echo "ERROR: make needs to be installed to run devenv. "
echo " Install make using the command such as ' apt install build-essential' as root and try again. "
echo " "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

scripts/env.d/vars Outdated Show resolved Hide resolved
It is better put environment variable to the same file.
@dungru dungru closed this May 10, 2021
@yungyuc
Copy link
Member

yungyuc commented May 10, 2021

@ldotrg It seems to me that this PR still has unfinished work and can still be worked on. Why not keep working on it?

Copy link
Author

@dungru dungru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ldotrg It seems to me that this PR still has unfinished work and can still be worked on. Why not keep working on it?

Instead of adding the warining message and fixing the "space".
I will add building GNU make from source. It's better than obsessed with some spaces and learned nothing in the end.
Because these typo problem could add the .clang_format to auto fix it.

if (( $? == 1 )); then
echo " "
echo "ERROR: make needs to be installed to run devenv. "
echo " Install make using the command such as ' apt install build-essential' as root and try again. "
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's on purpose. Actually the end of apt install build-essential needs space, too.
The space between command and quotes is avoiding the user copy the single quote
The purpose of the leading space that means this line is to follow the previous line error message.

@yungyuc
Copy link
Member

yungyuc commented May 10, 2021

@ldotrg It seems to me that this PR still has unfinished work and can still be worked on. Why not keep working on it?

Instead of adding the warining message and fixing the "space".
I will add building GNU make from source. It's better than obsessed with some spaces and learned nothing in the end.
Because these typo problem could add the .clang_format to auto fix it.

Understood. It makes sense to build make from source. Look forward to your next PR.

It would be a good idea to add an issue for the make recipe, so that everyone knows about your plan.

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

Successfully merging this pull request may close these issues.

3 participants