- changed status to open
- assigned issue to
- removed comment
wrong usage of VERBOSE in Cactus make files
The file make.config.rules.in contains shell syntax errors, e.g. in this construct:
define NOTIFY_PREPROCESSING
- "test" doesn't know the "==" operator, it's "=" instead
- Commands within { } need to be terminated by a semicolon
Also:
- In other places, we allow both upper and lower case settings
And:
- The { } seem superfluous here; the if ... fi already brackets the statement
- The VERBOSE variable could be checked by make, not by the shell
Keyword:
Comments (19)
-
-
reporter - removed comment
The missing semicolon is required even for bash.
-
- removed comment
Is there a good reason not to require bash? If you set SHELL = /bin/bash, make will use bash. Is there really a system that we need to target these days that does not have bash installed? If we can assume bash, we don't have to worry about conforming to sh features.
-
reporter - removed comment
In this case it's only a question of writing = instead of ==.
-
- changed status to resolved
- removed comment
This should be fixed now. The == bashism is gone, the {} as well (although the semicolon was not necessary, as the statement inside ended with fi). upper case is implemented, and checking VERBOSE by make itself would require a larger patch, so I don't do this right now.
-
- removed comment
As to why we should not assume bash: we should ask the other way around: unless we really have to use bash we should stick to sh, because it is more portable. I agree that bash is probably around on almost every machine you can think of, but there might be some where it is not, is in a non-standard place, not in the path, or such. Also, bash can be a tiny bit slower than a light replacement, which is why some linux distributions change their standard shell away from bash (but still install it).
-
reporter - changed status to open
- removed comment
This statement
VERBOSE=`echo $VERBOSE | tr '[:upper:]' '[:lower:]'`; \ if test "$(VERBOSE)" = "yes"; then echo Preprocessing $<; fi
checks an empty shell variable $VERBOSE first, and the make variable $(VERBOSE) later. That is, the conversion to lower case doesn't happen.
-
- removed comment
Please explain further.
1. 'make sim VERBOSE=YES' does the same as 'make sim VERBOSE=yes' for me: producing verbose output. What does it do for you? 2. grep for 'upper' in lib/make and you will find this kind of conversion for other variables as well. What is special about VERBOSE? Or do you imply that this is wrong for all of these variables?
-
reporter - removed comment
The first line sets a shell variable VERBOSE, the second line checks a make variable VERBOSE. These are different variables. The first line is therefore inconsequential. These line should instead read
VERBOSE=`echo $(VERBOSE) | ... if test "$$VERBOSE" ...
If you say VERBOSE=YES, then the lines "Preprocessing" are never output.
Another way of implementing this would be
ifneq ($(shell echo $(VERBOSE) | tr '[:upper:]' '[:lower:]'),yes) define NOTIFY_PREPROCESSING endef ... other definitions else define NOTIFY_PREPROCESSING echo "Preprocessing $<..." endef ... other definitions endif
-
- changed title to wrong usage of VERBOSE in Cactus make files
- changed status to open
- marked as
- changed milestone to ET_2011_10
- removed comment
Thanks Erik. I attached a patch which implements your suggestion.
-
reporter - removed comment
I see lines containing "-n" being output. This is probably due to the line
echo -n ""
Just saying "echo" instead should also work. On the other hand, there seem to be too many such empty lines, and outputting nothing instead may be better.
-
- removed comment
echo "" without the -n option produces an empty line (a newline is output). The -n option prevents this newline, at least the echo command I know does have that option. The "-n" isn't output on my system. Can you check if your echo command lacks it? That would explain why lines containing "-n" are output there. If there are systems with the echo command lacking this option we would have to do something else instead. This line is supposed to simply do nothing; we should be able to do come up with another command for that, maybe a single ":" might work just as well, at least when I test it in sh, tcsh and bash.
-
reporter - removed comment
Yes, this seems to be the case on my system (Mac OSX). There are several echos available; there is /bin/echo, the bash builtin echo, and likely also a dash builtin echo, with dash being used as /bin/sh. My echo man page states that -n is available, but is not standard, and recommends printf instead.
I would use : for a no-op command.
-
- removed comment
I added a new patch which implements the : as no-op command. It also addresses the VERBOSE-issue in the Makefile itself, and sets VERBOSE=yes if SILENT is set to no for backwards-compatibility. I believe all issues raised so far are now addressed. Please review.
-
reporter - removed comment
NOTIFY_DIVIDER in lib/make/make.configuration has a colon preceded by a space; that space should instead be a tab.
-
- changed status to open
- removed comment
This is fixed in a new patch.
-
reporter - removed comment
Please apply.
-
- changed status to resolved
- removed comment
Applied, plus an updated documentation in this regard.
-
- changed status to closed
- edited description
- Log in to comment
Right, I should have thought of that. It runs on machines which have bash as default shell, since this is valid bash syntax, but it's not valid sh syntax. I will fix this.