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

Added message in setup_runtime scripts #1226

Merged
merged 5 commits into from Apr 23, 2015
Merged

Conversation

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Apr 20, 2015

Some users reported wondering if anything (good or bad) happened when sourcing these
This let them know what happened
Some users also thought this only needed to be done once for all and would be
persistent accross terminal/sessions/reboots, etc...

Some users reported wondering if anything (good or bad) happened when sourcing these
This let them know what happen
Some users also thought this only needed to be done once for all and would be
persistent accross terminal/sessions/reboots, etc...
@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Apr 20, 2015

@dlonie @aashish24 @chaosphere2112 @remram44 please review

@remram44
Copy link
Contributor

@remram44 remram44 commented Apr 20, 2015

Should probably also display feedback line 13.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Apr 20, 2015

You mean in cleanup? Or when it fails?

@remram44
Copy link
Contributor

@remram44 remram44 commented Apr 20, 2015

Exactly before line 13. In that branch, you cleanup and still return 0 without any output.

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Apr 21, 2015

Good idea, I'll add a line there.

adds a message when UVCDAT was already sourced at the current location
@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Apr 21, 2015

@remram44 done.

@@ -10,10 +10,11 @@ function cleanup {
# which can lead to errors.
if [ -n "${UVCDAT_SETUP_PATH}" ] ; then
if [ "${UVCDAT_SETUP_PATH}" = "${install_prefix}" ] ; then
echo "Nothing to do UVCDAT is already setup at "${UVCDAT_SETUP_PATH} 1>&2
Copy link
Contributor

@remram44 remram44 Apr 21, 2015

Quotes are wrong

Copy link
Contributor Author

@doutriaux1 doutriaux1 Apr 21, 2015

why can't the var be outside the quotes?

Copy link
Contributor Author

@doutriaux1 doutriaux1 Apr 21, 2015

actually per: http://wiki.bash-hackers.org/syntax/quoting the quotes are probably not even needed here...

Copy link
Contributor

@remram44 remram44 Apr 21, 2015

echo "Nothing to do UVCDAT is already setup at ${UVCDAT_SETUP_PATH}" 1>&2
or
echo "Nothing to do UVCDAT is already setup at \"${UVCDAT_SETUP_PATH}\"" 1>&2
are probably what you meant

Copy link
Contributor Author

@doutriaux1 doutriaux1 Apr 21, 2015

i definitely don't want the quotes in the output, and since we are doing an echo of strings the quotes are actually not necessary. What I have in there is right. But I really don't care and we want to keep @remram44 happy so I will push an update right away with the quotes around the var.

@@ -22,11 +22,12 @@ endif
# which can lead to errors.
if ( $?UVCDAT_SETUP_PATH ) then
if ( ${UVCDAT_SETUP_PATH} == ${install_prefix} ) then
echo 'UVCDAT setup already sourced for this install location.'
echo 'Nothing to do UVCDAT is already setup at '${UVCDAT_SETUP_PATH}
Copy link
Contributor

@aashish24 aashish24 Apr 22, 2015

How about:
'Nothing to do since UVCDAT is already setup at '${UVCDAT_SETUP_PATH}

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Apr 22, 2015

@aashish24 @remram44 I think we are good to go now. Right?

aashish24
Copy link
Contributor

aashish24 commented on 5731c43 Apr 22, 2015

+1

echo "ERROR: Open a new shell in order to use a different install location." 1>&2
echo "INFO: UVCDAT setup was previously sourced at: ${UVCDAT_SETUP_PATH}" 1>&2
echo "INFO: There is no need to run setup_runtime manually anymore." 1>&2
echo "INFO: Open a new shell in order to use a different install location." 1>&2
Copy link
Contributor

@remram44 remram44 Apr 22, 2015

I made this an error because this does NOT setup the runtime environment, it keeps the previous one (which is different if this branch is reached).

Copy link
Contributor

@aashish24 aashish24 Apr 22, 2015

I think INFO should be good enough. Its not an error if we kept the last one. The script executed fine it just that it didn't do anything.

Copy link
Contributor

@remram44 remram44 Apr 22, 2015

I disagree: the script failed to load the requested environment. It should return an error code.

You wouldn't make the UV-CDAT setup process return 0 if it failed but another UV-CDAT is already installed.

Copy link
Contributor Author

@doutriaux1 doutriaux1 Apr 22, 2015

actually @remram44 is right, it should be an error here, we failed to use/setup the UVCDAT that the user requested. reversing.

Copy link
Contributor

@aashish24 aashish24 Apr 22, 2015

@remram44 what's important here? Users get the information right? whether you and I tag it as INFO or ERROR they will see the message. If you want we can change it to WARNING but its not a system error by any means in my opinion.

Copy link
Contributor

@remram44 remram44 Apr 22, 2015

You run the script for one reason, that is to load that UV-CDAT version in your shell. If you can't load it, it is as fatal an error as it can be. While would you pretend otherwise?

The only place the error status matters is when run from another script. If you run this from another script and encounter the condition, something is very wrong. Why would you possibly want to hide that?

I get that you like picking on whatever I say, but breaking the current behavior on purpose just because I wrote it is childish. UNIX has a notion of error codes, why not use it? Sure, you can figure it out by reading the terminal messages, but seriously, why not report errors as errors?

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Apr 22, 2015

👍

@doutriaux1
Copy link
Contributor Author

@doutriaux1 doutriaux1 commented Apr 22, 2015

@aashish24 @remram44 I think we're good to go now.

@remram44
Copy link
Contributor

@remram44 remram44 commented Apr 22, 2015

🆗 👍

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Apr 23, 2015

@doutriaux1 for the sake or moving forward, I am okay with this change.

aashish24 added a commit that referenced this issue Apr 23, 2015
@aashish24 aashish24 merged commit baf7b8f into master Apr 23, 2015
0 of 3 checks passed
@aashish24 aashish24 deleted the issue_1224_setup_runtime_msg branch Apr 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants