Lengthen string comparisons for arch names, so as to not skip arm_aarch64

[DeeDeeG]

Context

See #115.
(Fixes: #115)

Explanation

Strings starting with "arm[...?]" should not be checked before strings starting with "arm_aarch64[...?]", since the former will prematurely match, preventing the latter from ever matching.

[EDIT: a variant of one of the alternate solutions below was used instead.]

Available Alternate Solutions

Alternatively, the longer max comparison character count could be restored (was 20 chars before #114), so that the comparison is more "exact match" rather than "starts with," per documentation of this comparison function: https://docs.gtk.org/glib/func.ascii_strncasecmp.html. (I tested this and it works. I don't have a strong preference but I simply picked one of the available solutions to get a PR out quickly for your consideration.)

[EDIT: Lengthened all of the string comparisons to "the fixed string's non-null ASCII character count + 1" bytes, to include the fixed string's terminating NUL byte, so as to remove ambiguities between longer strings and shorter substrings that could otherwise be matched at the beginnings of those fixed strings. Particularly addresses the fact that the first three characters of "arm" == the first three characters of "arm_arch64" (bug).]

We could drop the check for the "arm" string entirely, which it seems may have been the intended solution in the review discussions of #114, but I'm not sure if anyone's scripts out there use that, or what, it would have been harder for me to verify that approach doesn't break anyone's stuff so I opted for this that doesn't remove any string checks, only reorders them.

There are likely even more solutions to the problem... But this PR is a simple one, and works for me in testing.

Verification Process

I ran a CI workflow with and without this change.

• With this change, the aarch64runtime is correctly used. CI workflow that needs this passes.
  • https://cirrus-ci.com/task/4607957775679488?logs=fix_linux_appimage#L8194
  • [EDIT: Updated solution also behaves correctly in CI: https://cirrus-ci.com/task/6069624871059456?logs=fix_linux_appimage#L8194 ]
• Without this change, the armhf runtime is erroneously used. CI workflow errors due to cannot execute binary file: Exec format error.
  • https://cirrus-ci.com/task/4806848068452352?logs=fix_linux_appimage#L8189

That shows some change from current main branch is indeed needed, and that this PR does indeed work as a fix.

[black-sliver]

I think a decision needs to be made if this is supposed to be "startswith" or "equals". For "startswith", this change looks correct, however that makes it easier to accidentally match something. Say a future architecture is called "aarch64v2", then this will possibly produce an invalid output instead of giving an error.

To change it back to "equals", you'd need to add +1 to each of the lengths because the comparison will then include the NUL byte.

[DeeDeeG]

Okay, along the lines of what @black-sliver just said, I can confirm all of the following approaches work, regardless of the order the various strings are checked, and should be basically equivalent to each-other in practice AFAICT:

• g_ascii_strncasecmp("arm", archname, 4) DeeDeeG@1dd660a
• g_ascii_strncasecmp("arm", archname, 20) DeeDeeG@5997837
• g_ascii_strncasecmp("arm", archname, sizeof("arm")) DeeDeeG@3cf1d45

Those all increase the string comparison to be long enough to include the NUL byte at the end of the hard-coded string side of the comparison.

(See docs for g_ascii_strncasecmp: https://gnome.pages.gitlab.gnome.org/libsoup/glib/glib-String-Utility-Functions.html#g-ascii-strncasecmp)

So, with any of the above, the comparison function is looking for "exact match," e.g. "starting and ending with arm[NUL]" rather than "starts with arm" where any trailing characters after that are ignored, so longer strings starting with arm would match undesirably, e.g. "first three chars of arm == first three chars of arm_aarch64" (overly broad comparison catching false matches).

Those are all viable solutions to #115, as is the current PR as already written. As long as it fixes the problem, I'm willing to post whatever solution maintainers here prefer, having just shown four of them that all work. 👍

[TheAssassin]

I think a decision needs to be made if this is supposed to be "startswith" or "equals". For "startswith", this change looks correct, however that makes it easier to accidentally match something. Say a future architecture is called "aarch64v2", then this will possibly produce an invalid output instead of giving an error.

In my opinion, we should just literally match the strings. Checking only the prefix makes no sense. Typically, however, someone will show up depending on the old behavior. I think it would be ok to break these people's workflows...

We had that docs page explaining our interpretation of ISAs and the identifier strings we use and how they compare to, e.g., Debian's packaging (and we may want to add others, like Docker platforms, GOARCH, ...).

[black-sliver]

I think that makes DeeDeeG@1dd660a the preferable solution then.

[TheAssassin]

Agreed. @DeeDeeG could you make sure that this solution is provided here?

(Side note: C string comparisons are so annoying...)

[DeeDeeG]

Hi again folks, I updated the PR with the solution from DeeDeeG@1dd660a as requested.

[black-sliver]

Change looks correct, have not tested it myself though.

[TheAssassin]

Thank you very much!