pkg2appimage icon indicating copy to clipboard operation
pkg2appimage copied to clipboard

functions.sh using non-portable uname -i

Open krayon opened this issue 6 years ago • 11 comments

Similar to https://github.com/AppImage/AppImageKit/pull/650 functions.sh uses the non-portable command uname -i. Is there any reason why uname -m isn't suitable?

krayon avatar Mar 15 '18 02:03 krayon

I saw someone suggest the gnu coreutils arch command however this is not installed by default and thus also not portable ( at least according to https://www.gnu.org/software/coreutils/manual/html_node/arch-invocation.html ).

krayon avatar Mar 15 '18 02:03 krayon

Thank you.

Is there any reason why uname -m isn't suitable?

I don't know. What we have now is working for me. Does this fix a concrete issue someone is having (not just "in generally it would be better")?

probonopd avatar Mar 17 '18 11:03 probonopd

@probonopd I originally heard about this from Klaatu's podcast and I noticed he did raise an issue for it: #288

The solution there of adding AuthenticAMD to the fallback method of using the portable uname -m alternative is messy but does work... for now. It does mean however, that each time a new return value for uname -i is found, this whole process occurs again. I think it comes from the processor - and given that it's non-portable, there's probably more that haven't been discovered (or at least reported) yet). Each time one does:

  1. The bitten user will need to determine the cause;
  2. The user will then have to be bothered to raise an issue; and
  3. The new string will need to be added to the case statement.

Given the GenuineIntel in that statement also indicates this has so far happened twice. Using uname -m when that was found would have avoided #288.

krayon avatar Mar 18 '18 16:03 krayon

I guess if you wanted to be more cautious than my suggestion, you could have uname -m processed first, with uname -i a fallback if and only if uname -m didn't return a valid arch. You could even put a note in there requesting the user opens an issue reporting the event. At least then you're only using the non-portable uname -i in an extreme circumstance - most likely only on a system with some weird arch that's not supported anyway.

krayon avatar Mar 18 '18 16:03 krayon

you could have uname -m processed first, with uname -i a fallback if and only if uname -m didn't return a valid arch.

I really like that. Would you be willing to implement it? Thanks.

probonopd avatar Mar 18 '18 20:03 probonopd

Keep in mind that the commands only have to work on the target systems and not some exotic commercial Unices.

darealshinji avatar May 31 '18 13:05 darealshinji

Keep in mind that the commands only have to work on the target systems and not some exotic commercial Unices.

OK, but keep in mind too that AppImage had to already be modified (twice?) to handle edge cases with uname -i (Slackware for example - hardly an "exotic commercial Unix") and potentially will need to again in future due to uname -i not being portable. There's a reason it explicitly discourages the use of uname -i for this purpose.

Already the edge cases cause messy unnecessary matching of all the currently encountered possible return values of uname -i.

However, as stated above, if one really wants to be defensive, leave the messy edge case detections in and fall back to the proper method on failure.

IMHO one should not write code with a "it works for me" attitude. Where possible, standards or in this case, suggested usage, should be observed.

krayon avatar Jun 01 '18 12:06 krayon

@probonopd - As per your request, I have updated the branch to only fall back to uname -m when uname -i fails.

krayon avatar Jun 01 '18 15:06 krayon

This is causing issues for me on Arch Linux. uname -i and uname -p return unknown, but uname -m returns x86_64. This makes running recipes with scripts marked as executable kind of annoying (for some reason, it detects .js files as ARM, and requests me to set ARCH manually.

If this helps, my machine has an Intel Core i7-8565U.

capezotte avatar Nov 09 '20 16:11 capezotte

Would https://github.com/AppImage/pkg2appimage/pull/320 solve this for you?

Is there a way to get the architecture from the /sys tree (sysfs)?

probonopd avatar Nov 09 '20 18:11 probonopd

Is there a way to get the architecture from the /sys tree (sysfs)?

AFAIK, no.

I would reiterate if you're going to merge #320 , you're probably better off taking the earlier commit I originally suggested as it's cleaner (does away with uname -i)

krayon avatar Nov 16 '20 00:11 krayon