Fix template name for mkdir #2650
Conversation
|
I agree that this looks like the right fix, but I wonder if there's any way we can test this somehow. If you can think of a way to add a test to our CI which validates this then that'd be cool, if you don't think you can, given the intermittent behaviour the bug reporters have had, then we can merge this since I can't see at all how it could be a bad thing regardless. |
|
I really don't see how to test this in CI, since this is only executed when |
|
@kinnison Do you have any idea on how to test this in CI ? Is there anything else I have to do before this can be merged ? |
rustup-init.sh
Outdated
| @@ -72,7 72,7 @@ main() { | |||
| local _url="${RUSTUP_UPDATE_ROOT}/dist/${_arch}/rustup-init${_ext}" | |||
|
|
|||
| local _dir | |||
| _dir="$(mktemp -d 2>/dev/null || ensure mktemp -d -t rustup)" | |||
| _dir="$(mktemp -d 2>/dev/null || ensure mktemp -d -t rustup-XXX)" | |||
There was a problem hiding this comment.
lets just remove the || path here - even OpenBSD mktemp supports -d as-is. https://man.openbsd.org/mktemp.1 : there's no possible case I can see where we're making things better with this.
#183 is the bug that this was meant to fix initially. mktemp on OS X - 7719d91
Now, here's the man page for OS X mktemp: https://ss64.com/osx/mktemp.html
again, -d is supported.
So, I don't know why it was failing for rustup-setup as reported in #183, but the fix was not correctly diagnosed: the original command was valid, and the fix was invalid: note the language from the man page "The template may be any file name with some number of `Xs' appended to it, for example /tmp/temp.XXXX. ".
So - lets:
- go back to a single mktemp -d
- remove 2>/dev/null so that we can actually find out what goes wrong in the long tail here and fix the real bug in future
There was a problem hiding this comment.
This makes a lot of sense. I wasn't in the mood to go digging in other OS manpages, so thanks for that Robert.
There was a problem hiding this comment.
We want
local _dir="$(ensure mktemp -d)"
Your commit looks like
_dir="$(mktemp -d 2>/dev/null || ensure mktemp -d -t rustup-XXX)"
to me - this might be github glitching though, since its been acting odd overnight...
There was a problem hiding this comment.
Right, I forgot to stage my changes before committing... should be good now
|
Thank you. |
Fix template name for mkdir
Fix #2553
Is it really all what is needed ?