nengo / nengo-loihi

Run Nengo models on Intel's Loihi chip
https://www.nengo.ai/nengo-loihi/
Other
35 stars 12 forks source link

Unset bash settings after scripts #178

Closed tbekolay closed 5 years ago

tbekolay commented 5 years ago

This is mainly relevant if these scripts are sourced, which happens for conda.sh on TravisCI. Once a script is sourced, the rest of the bash session will print all lines, which litters the TravsisCI log with internal commands that we can ignore like travis_nanoseconds.

This PR also gives a more informative error message if no command is passed to the script.

hunse commented 5 years ago

One thing that's a little yucky about printing lines in general is that it makes it hard to distinguish an error. For example, if I call conda.sh without a command, I get something like this:

    source activate test
elif [[ -z "$COMMAND" ]]; then
    echo "$NAME requires a command like 'install' or 'script'"
else
    echo "$NAME does not define $COMMAND"
fi
.ci/conda.sh requires a command like 'install' or 'script'

set +e +v  # reset options in case this is sourced

It's kind of hard to notice the error in there with all the other stuff that's printed.

The easiest solution I can think of is to just put "ERROR" at the start of these error messages, so that you get something like this:

    source activate test
elif [[ -z "$COMMAND" ]]; then
    echo "ERROR: $NAME requires a command like 'install' or 'script'"
else
    echo "ERROR: $NAME does not define $COMMAND"
fi
ERROR: .ci/conda.sh requires a command like 'install' or 'script'

set +e +v  # reset options in case this is sourced

Do others find this helpful?

Another option (either an alternative or a complement) would be to put the new set statements earlier:

    source activate test
    set +e +v  # reset options in case this is sourced
elif [[ -z "$COMMAND" ]]; then
    set +e +v  # reset options in case this is sourced
    echo "ERROR: $NAME requires a command like 'install' or 'script'"
else
    set +e +v  # reset options in case this is sourced
    echo "ERROR: $NAME does not define $COMMAND"
fi
ERROR: .ci/conda.sh requires a command like 'install' or 'script'

Then the error comes at the end. The downside is that you have to call this in every branch.

We could also do something fancier where in the error blocks, we set an error code, and then have another block after the set +e +v where we actually echo the error message.

tbekolay commented 5 years ago

Personally I don't find set -v all that helpful in the first place. One alternative that I'd be happy to implement is to explicitly echo the commands we want to echo instead of the whole script. It reduces the noise while still giving the context of what is being run. I'll push a quick commit to show what that looks like.

tbekolay commented 5 years ago

2bfd160f3854cc691eafc186bdb125ac463d4619 does the selective echoing and ensures that the scripts are run from the nengo-loihi root diretory (even if run from somewhere else)

1c6d06b87dc6d7687bf43459395d683b24075387 enables static checks on our CI scripts (I have these enabled locally and it informed some of the changes in these recent commits, so I figured it would be good to have enforced)

I am happy to pull one or both of these commits into separate PRs for consideration in future sprints, but making them fit into an hour with time to spare.

tbekolay commented 5 years ago

TravisCI does weird things with BASH_SOURCE (https://github.com/travis-ci/travis-ci/issues/8392) so this fails. I'll try to fix and if it takes more than ten minutes I'll move to a separate PR

tbekolay commented 5 years ago

Fixed by checking what directory you're running in rather than changing directories (e9e5a8ae19f357e1b957fa9836e6badaeed7b615). This will fail if you rename the root directory to something other than nengo-loihi, not sure if we really care about that situation since these files are mostly for TravisCI where the directory isn't renamed.

hunse commented 5 years ago

I do like to run the static.sh script sometimes, and my root is named nengo_loihi instead of nengo-loihi. Why do we need this check? If it's required, is there a way to only do it only on TravisCI?

tbekolay commented 5 years ago

Why do we need this check? If it's required, is there a way to only do it only on TravisCI?

There are a few spots in these scripts that we assume you're in the root folder, so while the check isn't strictly required, some of the scripts will fail if you're not in that folder, so the check is to enforce a certain workflow in case the user isn't doing that. You can verify this now if you're in master by running static.sh script from within the .ci folder.

If people are renaming the root folder, then we have to do this another way. The check is relevant both locally and on TravisCI. I'll try to think of another way.

tbekolay commented 5 years ago

@hunse suggested checking for a few things to exist as relative paths, so I did that in 4db7f3690217ddd01. This should work in 99.999% of cases, so I think we can safely round that to 100%.