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

Minor improvements to perlbrew-install #568

Merged
merged 7 commits into from Oct 16, 2017
Merged

Minor improvements to perlbrew-install #568

merged 7 commits into from Oct 16, 2017

Conversation

fluca1978
Copy link
Contributor

A set of minor shell improvements to perlbrew-install in order to make the shell script more coherent.
Tested on bash 4.4.7 and zsh 5.2 on ubuntu linux.

If the perl executable is not found into one of the well known places
during the loop the error message is shown, otherwise the first
executable found is used as system-wide Perl.
This is coherent with the same usage of -z a few lines under this code.
It does not make sense to change dir to '.'.
Make the clean_exit function callable from the very beginning of the script
and use it instead of "plain" exit.
@coveralls
Copy link

coveralls commented Oct 5, 2017

Coverage Status

Coverage decreased (-0.02%) to 84.594% when pulling 15e3633 on fluca1978:sh into 7096e91 on gugod:develop.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 84.594% when pulling 15e3633 on fluca1978:sh into 7096e91 on gugod:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 84.594% when pulling 15e3633 on fluca1978:sh into 7096e91 on gugod:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 84.594% when pulling 15e3633 on fluca1978:sh into 7096e91 on gugod:develop.

Copy link
Owner

@gugod gugod left a comment

Choose a reason for hiding this comment

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

While it looks OK to altering exit() status, I'm curious: do you plan to depen on those values for some application ?

@gugod gugod merged commit 15e3633 into gugod:develop Oct 16, 2017
PERLBREWURL=https://raw.githubusercontent.com/gugod/App-perlbrew/master/perlbrew
fi

if [ -z "$TMPDIR" ]; then
if [ -d "/tmp" ]; then
TMPDIR="/tmp"
cd $TMPDIR || clean_exit 1

Choose a reason for hiding this comment

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

Only cd $TMPDIR if $TMPDIR is not the current directory, but what happens if $TMPDIR is set in the environment? Then no cd happens. Is that correct?

exit $1
}

LOCALINSTALLER=$(mktemp perlbrew.XXXXXX)

Choose a reason for hiding this comment

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

This creates a file in the current directory (at least in GNU mktemp); see https://www.gnu.org/software/coreutils/manual/html_node/mktemp-invocation.html#mktemp-invocation .

@jonathancast
Copy link

I commented on two lines that are causing me weird behavior: empty perlbrew.XXXXXX files showing up in the current directory when installing perlbrew. (This is noticeable for me because I'm writing an INSTALL script that calls out to the perlbrew installation procedure, which means I'm running it over and over). I think the issue for me is:

  • I don't have $TMPDIR set.
  • I do have a /tmp directory.
  • Therefore, perlbrew-install is creating a temp file in my current directory, then cding to /tmp and using (and then deleting) a file of that name in /tmp.
  • The file in my current directory is never used or deleted.

My guess is that the original author of this PR had $TMPDIR in h{is,er} environment, which means the cd never happens and the original file in the original current directory gets cleaned up at the end of the script.

fluca1978 added a commit to fluca1978/App-perlbrew that referenced this pull request Mar 13, 2018
As per discussion in
<gugod#568 (review)>
the TMPDIR variable could have been set from the
environment and in such case no 'cd' happens leaving the installer
in the current directory.

Thanks @jonathancast
fluca1978 added a commit to fluca1978/App-perlbrew that referenced this pull request Mar 13, 2018
Since mktemp is not called with '-p' the temporary file
is created relatively to the current directory. Since the
directory is changed after the temporary file is created, this
could leave the installer with a dangling file.

See discussion
<gugod#568 (review)>

Thanks to @jonathancast.
fluca1978 added a commit to fluca1978/App-perlbrew that referenced this pull request Mar 13, 2018
Now the installation script has a few more extra checks for
existance of the directories and installation file since
the script could die in the early stage and leave some
dangling file.

see discussion
<gugod#568 (review)>
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.

None yet

4 participants