remove Xs from constructed mktemp templates #59
Conversation
jhoblitt
force-pushed
the
tickets/DM-11444-mktemp
branch
from
732f61d
to
34b40d2
Compare
|
Travis is having trouble again today, so we're still waiting on CI to know if this is good -- @jdswinbank do you want to give this concept a quick look-over? |
scripts/newinstall.sh
Outdated
| # | ||
| # XXX GNU mktemp will complain about extra Xs in the filename that are too | ||
| # small of a consecutive sequence to constitute a legal template. | ||
| tmpfile=$(mktemp -t "XXXXXXXX.${safe_miniconda_file_name/X/_}") |
There was a problem hiding this comment.
Should be miniconda_file_name?
Should be ${miniconda_file_name//X/_} (to guard against multiple occurrences of X in the filename)?
scripts/newinstall.sh
Outdated
| @@ -458,7 +461,7 @@ miniconda::lsst_env() { | |||
| conda_opts='--quiet' | |||
| fi | |||
|
|
|||
| tmpfile=$(mktemp -t "${conda_packages}.XXXXXXXX") | |||
| tmpfile=$(mktemp -t "${conda_packages/X/_}.XXXXXXXX") | |||
| # the miniconda installer seems to complains if the filename does not end | ||
| # with .sh | ||
| tmpfile=$(mktemp -t "XXXXXXXX.${miniconda_file_name}") | ||
| # XXX the miniconda installer seems to complains if the filename does not |
There was a problem hiding this comment.
What's the XXX business about?
There was a problem hiding this comment.
It is emphasis that we're working around what is arguably a bug in the miniconda installer.
|
|
||
| # "stub" out cmd to prevent curl from being run | ||
| # shellcheck disable=SC2034 | ||
| cmd="echo" |
There was a problem hiding this comment.
I realise you're not adding cmd to newinstall.sh here, just taking advantage of it. However, it did cause me a moment of confusion before I worked out that it is supposed to be undefined in normal operation. Worth a comment?
There was a problem hiding this comment.
It is commented in newinstall.sh: https://github.com/lsst/lsst/blob/master/scripts/newinstall.sh#L933-L939 -- I'm not sure what else could be noted here that would be helpful.
| cmd="echo" | ||
|
|
||
| # test expected usage | ||
| miniconda::install 3 4.2.12 /dne |
There was a problem hiding this comment.
/dne is just an (ignored) prefix, right? I first looked for it as some sort of argument to the miniconda installer. Worth a comment?
There was a problem hiding this comment.
It is a required argument to the miniconda::install function which, is passed on to the miniconda installer. The value doesn't matter at this point since cmd has been stubbed out.
GNU mktemp will complain about extra Xs in the filename that are too
small of a consecutive sequence constitute a legal template.
Eg.:
$ mktemp --version
mktemp (GNU coreutils) 8.27
Copyright (C) 2017 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Written by Jim Meyering and Eric Blake.
$ mktemp -t XXXXX.Miniconda2-4.2.12-MacOSX-x86_64.sh
mktemp: too few X's in template ‘XXXXX.Miniconda2-4.2.12-MacOSX-x86_64.sh’
jhoblitt
force-pushed
the
tickets/DM-11444-mktemp
branch
from
34b40d2
to
53b823a
Compare
No description provided.