-
Notifications
You must be signed in to change notification settings - Fork 860
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(dist/linux): handle the possible unavailability of /proc
in rustup-init.sh
#3800
base: master
Are you sure you want to change the base?
Conversation
7ee4c2a
to
3043a95
Compare
/proc/self/exe
in rustup-init.sh
/proc
in rustup-init.sh
/proc
in rustup-init.sh
/proc
in rustup-init.sh
case "$_ostype" in | ||
|
||
Android) | ||
_ostype=linux-android | ||
;; | ||
|
||
Linux) | ||
check_proc | ||
get_current_exe | ||
_current_exe=$RETVAL |
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.
Any reason not to use
_current_exe=get_current_exe
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.
@rbtcollins I assume you meant _current_exe=$(get_current_exe)
?
The echo "Warning: ..."
calls in the function body will interfere with the function's output when it's being used like so (unless the 1>&2
trick is used, as I understand it). Looking at the surrounding code, it seems like the $RETVAL
trick is used exactly in this case to specify the actual value to be returned.
I referred to downloader()
as an example when writing this. This is how it calls other functions via $RETVAL
:
Lines 596 to 599 in 2a5a69e
check_curl_for_retry_support | |
_retry="$RETVAL" | |
get_ciphersuites_for_curl | |
_ciphersuites="$RETVAL" |
This is how it issues warnings:
Line 604 in 2a5a69e
echo "Warning: Not enforcing strong cipher suites for TLS, this is potentially less secure" |
Maybe we should use 1>&2
for echo
commands instead? But in that case we might hit #3350, right?
I think this looks reasonable. There are other probes we could use, such as uname and lsb_release and gcc but they all have edge cases and caveats. One question for you then GTG IMO. |
if test -L /proc/self/exe ; then | ||
_current_exe=/proc/self/exe | ||
else | ||
echo "Warning: Unable to find /proc/self/exe. System architecture detection might be inaccurate." |
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.
Oops, a tiny typo here...
echo "Warning: Unable to find /proc/self/exe. System architecture detection might be inaccurate." | |
echo "Warning: Unable to find /proc/self/exe. System architecture detection might be inaccurate." |
Closes #2700. I'm no shell expert, just trying to do my best...
This fix is based on @miigotu and @kerberjg's find in #2700 (comment) and #2700 (comment) respectively. It first refers to
/proc/self/exe
if it's accessible, then$SHELL
if it's set, and finally/bin/sh
. This executable is then explicitly passed to functions that previously relied on/proc/self/exe
.Concerns
What about theFixed.grep '^Features' /proc/cpuinfo
line which also relies on/proc
?grep
fails (because e.g./proc/cpuinfo
is unavailable or malformed), we still assumearmv7
. This doesn't work in the case described inrustup-init.sh
fails to detect platform correctly underdocker buildx
which lacks/proc
#2700, where we're using ARMv6. OTOH, falling back toarm
is much safer in this case and will resolve the aforementioned issue.TheI've tested the patch with a modifiedpodman
instance I use seems to have/proc
access./proc
path (/fakeproc
) and the logic seems to work.