-
Notifications
You must be signed in to change notification settings - Fork 877
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're welcome :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?