1. Einstein Toolkit Avatar Einstein Toolkit
  2. Einstein Toolkit
  3. tickets
  4. Issues

wrong usage of VERBOSE in Cactus make files

Issue #586 closed
Erik Schnetter created an issue

The file make.config.rules.in contains shell syntax errors, e.g. in this construct:

define NOTIFY_PREPROCESSING

  1. "test" doesn't know the "==" operator, it's "=" instead
  2. Commands within { } need to be terminated by a semicolon

Also:

  1. In other places, we allow both upper and lower case settings

And:

  1. The { } seem superfluous here; the if ... fi already brackets the statement
  2. The VERBOSE variable could be checked by make, not by the shell

Keyword:

Comments (19)

  1. Frank Löffler
    • changed status to open
    • assigned issue to
    • removed 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.

  3. Ian Hinder
    • 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.

  5. Frank Löffler
    • 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.

  6. Frank Löffler
    • 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).

  7. Erik Schnetter 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.

  8. Frank Löffler
    • 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?

  9. Erik Schnetter 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

  11. Erik Schnetter 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.

  12. Frank Löffler
    • 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.

  13. Erik Schnetter 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.

  14. Frank Löffler
    • 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.

  15. Erik Schnetter reporter
    • removed comment

    NOTIFY_DIVIDER in lib/make/make.configuration has a colon preceded by a space; that space should instead be a tab.

  20. Log in to comment
Assignee
Type
bug
Priority
minor
Status
closed
Component
Cactus
Milestone
ET_2011_10
Version
–
Votes
0
Watchers
0
Jira Software: the preferred issue tracker for Bitbucket. Join the team!