Skip to content

Added message in setup_runtime scripts #1226

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.

Sign up for GitHub

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

Merged
aashish24 merged 5 commits into from
Apr 23, 2015
Merged

Added message in setup_runtime scripts #1226

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7b274e7
Added message in setup_runtime scripts
doutriaux1 Apr 20, 2015
e009857
addressed @remram44 suggestion
doutriaux1 Apr 21, 2015
7226d92
quotes moved to please @remram44
doutriaux1 Apr 21, 2015
5731c43
Addressed @aashish24 concerns
doutriaux1 Apr 22, 2015
aed46d1
reverted from INFO to ERROR because we fail to do what user asked in …
doutriaux1 Apr 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions CMake/cdat_modules_extra/setup_runtime.csh.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,13 @@ 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.'
exit 1
echo 'Nothing to do since UVCDAT is already setup at '${UVCDAT_SETUP_PATH}
exit 0
else
echo 'ERROR: UVCDAT setup was previously sourced at '${UVCDAT_SETUP_PATH}
echo 'Open a new shell in order to use a different install location.'
exit 1
echo 'INFO: UVCDAT setup was previously sourced at '${UVCDAT_SETUP_PATH}
echo 'INFO: There is no need to run setup_runtime manually anymore.'
echo 'INFO: Open a new shell in order to use a different install location.'
exit 0
endif
endif

Expand Down Expand Up @@ -91,3 +92,7 @@ setenv UVCDAT_SETUP_PATH ${install_prefix}

unset install_prefix

echo 'Successfully updated your environment to use UVCDAT'
echo '(changes are valid for this session/terminal only)'
echo 'Version: '${UVCDAT_PROMPT_STRING}
echo 'Location: '${UVCDAT_SETUP_PATH}
15 changes: 10 additions & 5 deletions CMake/cdat_modules_extra/setup_runtime.sh.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,21 @@ function cleanup {
# which can lead to errors.
if [ -n "${UVCDAT_SETUP_PATH}" ] ; then
if [ "${UVCDAT_SETUP_PATH}" = "${install_prefix}" ] ; then
echo "Nothing to do since UVCDAT is already setup at: ${UVCDAT_SETUP_PATH}" 1>&2
cleanup
return 0
else
echo "ERROR: UVCDAT setup was previously sourced at \"${UVCDAT_SETUP_PATH}\"" 1>&2
echo "ERROR: There is no need to run setup_runtime manually anymore." 1>&2
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
Copy Markdown
Contributor

@remram44 remram44 Apr 22, 2015

Choose a reason for hiding this comment

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

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
Copy Markdown
Contributor

@aashish24 aashish24 Apr 22, 2015

Choose a reason for hiding this comment

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

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
Copy Markdown
Contributor

@remram44 remram44 Apr 22, 2015

Choose a reason for hiding this comment

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

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
Copy Markdown
Contributor Author

@doutriaux1 doutriaux1 Apr 22, 2015

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@aashish24 aashish24 Apr 22, 2015

Choose a reason for hiding this comment

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

@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
Copy Markdown
Contributor

@remram44 remram44 Apr 22, 2015

Choose a reason for hiding this comment

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

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?

cleanup
return 1
return 0
fi
fi

# Check that the install prefix exists, otherwise stop.
if [ ! -d "${install_prefix}" ] ; then
echo "ERROR: \"${install_prefix}\" is not a directory." 1>&2
echo "ERROR: ${install_prefix} is not a directory." 1>&2
cleanup
return 1
fi
Expand Down Expand Up @@ -85,4 +86,8 @@ export PYTHONPATH

export UVCDAT_SETUP_PATH="${install_prefix}"
cleanup
echo "Successfully updated your environment to use UVCDAT" 1>&2
echo "(changes are valid for this session/terminal only)" 1>&2
echo "Version: ${UVCDAT_PROMPT_STRING}" 1>&2
echo "Location: ${UVCDAT_SETUP_PATH}" 1>&2
return 0
⚡