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

Fix template name for mkdir #2650

Merged
merged 1 commit into from
Feb 10, 2021
Merged

Fix template name for mkdir #2650

merged 1 commit into from
Feb 10, 2021

Conversation

kellda
Copy link
Contributor

@kellda kellda commented Feb 5, 2021

Fix #2553

Is it really all what is needed ?

@kinnison
Copy link
Contributor

kinnison commented Feb 7, 2021

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.

@kellda
Copy link
Contributor Author

kellda commented Feb 7, 2021

I really don't see how to test this in CI, since this is only executed when mktemp -d fails. (Btw I don't see why/how mktemp -d can fails, but it seems it happened once)

@kellda
Copy link
Contributor Author

kellda commented Feb 9, 2021

@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)"
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're welcome :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@rbtcollins rbtcollins Feb 10, 2021

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...

Copy link
Contributor Author

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

@rbtcollins
Copy link
Contributor

Thank you.

@rbtcollins rbtcollins merged commit 9896911 into rust-lang:master Feb 10, 2021
@kellda kellda deleted the patch-1 branch February 10, 2021 09:50
AJ-Ianozi pushed a commit to AJ-Ianozi/getada-download that referenced this pull request Oct 12, 2023
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.

mktemp: too few X's in template ‘rustup’
3 participants