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

Check for spurious output at bash login shell #2132

Conversation

giovannipizzi
Copy link
Member

@giovannipizzi giovannipizzi commented Oct 26, 2018

This fixes #1890 and fixes #1887.
This is a different solution than PR #1888 and superses it.
We both document what the user shell should NOT do (produce
output if in interactive mode), and explain how to keep this
but guard it so that no output is produced at least in
non-interactive mode.
Moreover, we add a test in verdi computer test that has pointers
to the documentation and the issue, making it (hopefully) clear
early on to the user on how to solve the issue.

This fixes aiidateam#1890 and fixes aiidateam#1887.
This is a different solution than PR aiidateam#1880 and superses it.
We both document what the user shell should NOT do (produce
output if in interactive mode), and explain how to keep this
but guard it so that no output is produced at least in
non-interactive mode.
Moreover, we add a test in `verdi computer test` that has pointers
to the documentation and the issue, making it (hopefully) clear
early on to the user on how to solve the issue.
clear
echo "Hi! This is a new shell"
;;
esac
Copy link
Contributor

Choose a reason for hiding this comment

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

the guard usually looks as follows:

if [[ $- != *i* ]] ; then
    # Shell is non-interactive.  Be done now!
    return
fi

dev-zero
dev-zero previously approved these changes Oct 26, 2018
Copy link
Contributor

@dev-zero dev-zero left a comment

Choose a reason for hiding this comment

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

The guard usually looks a bit different as stated in the comment, but if you want to keep it like this, it is ok.

@espenfl
Copy link
Contributor

espenfl commented Oct 26, 2018

I would suggest to follow @dev-zero on the formatting. This is what most people expect to see. Nice to get this documented for new and possibly inexperienced users.

@coveralls
Copy link

Coverage Status

Coverage decreased (-7.7%) to 60.872% when pulling 83b1306 on giovannipizzi:fix_1890_document_bash_profile_restrictions into cc90894 on aiidateam:develop.

@coveralls
Copy link

coveralls commented Oct 26, 2018

Coverage Status

Coverage decreased (-7.9%) to 60.779% when pulling 20d3e60 on giovannipizzi:fix_1890_document_bash_profile_restrictions into d0a8db5 on aiidateam:develop.

As suggested by @dev-zero.
Also, removing a sentence that would apply
only if we were using non-login shells.
@giovannipizzi
Copy link
Member Author

Done. Happy to get an approval again if now it's ok.

@sphuber sphuber merged commit 8e9a1ca into aiidateam:develop Oct 28, 2018
@giovannipizzi giovannipizzi deleted the fix_1890_document_bash_profile_restrictions branch October 28, 2018 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Complaints about TERM environment variable not set in tests Handle ANSI escape and csi characters
5 participants