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

[bug]: gh-r installs linux binary on a may when there is no mac binary #295

Open
jankatins opened this issue Jun 1, 2022 · 9 comments
Open
Labels
bug Something isn't working enhancement New feature or request feature request Add new functionality good first issue Issue that are great for first-time contributors help wanted Extra attention is needed

Comments

@jankatins
Copy link
Contributor

Describe the bug

Running the following on a mac will download the wrong (=linux) binary, as there is only a linux build available (git-absorb release page). I would have expected an error upon installation as the binary is clearly a linux one:

[11:21:41] λ  zinit  as'null' from"gh-r" nocompile nocompletions lbin"git-absorb-* -> git-absorb" for @tummychow/git-absorb

Downloading tummychow/git-absorb…
(Requesting `git-absorb-linux-x86_64'…)
#################################################################################################################################################################################################################### 100.0%
ziextract: Successfully extracted and assigned +x chmod to the file: `git-absorb-linux-x86_64'.
linkbin annex: Created the git-absorb hard link and set +x on the git-absorb-linux-x86_64 binary

[11:21:52] λ  git absorb
/Users/jankatins/.zinit/polaris/bin/git-absorb: /Users/jankatins/.zinit/polaris/bin/git-absorb: cannot execute binary file

[11:22:14] [126] ✗  uname -a
Darwin jankatins.fritz.box 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:22 PDT 2022; root:xnu-8020.121.3~4/RELEASE_X86_64 x86_64

(zinit is a recent version after the xtrace changes -> my exclamation mark branch on top of main/bcedc9ff947319a5a979e5d9dfc61e7d9f39cfbd)

Steps to reproduce

  1. Be on a mac, not a linux computer
  2. enable lbin annex
  3. run zinit as'null' from"gh-r" nocompile nocompletions lbin"git-absorb-* -> git-absorb" for @tummychow/git-absorb on mac
  4. run git absorb and observe that it fails

Expected behavior

An error during installation as there is no mac binary

Screenshots and recordings

No response

Operating System & Version

darwin21.3.0 | apple | x86_64 | x86_64 | x86_64 i386

Zsh version

zsh 5.9 (x86_64-apple-darwin21.3.0)

Terminal emulator

xterm-256color (wezterm)

If using WSL on Windows, which version of WSL

No response

Additional context

No response

@jankatins jankatins added the bug Something isn't working label Jun 1, 2022
@vladdoster
Copy link
Member

I think that knowing whether or not a project has a release asset for a respective system falls squarely on the user. I can't think of a situation where you'd add a program and not know if they support your OS.

@vladdoster
Copy link
Member

At which point use the if'' ice, rust annex, or build it from the source.

@vladdoster
Copy link
Member

It opens a huge can of worms in terms of complexity...

Such as whats the logic for:

  • only has FreeBSD Linux binary
  • bandwhich only has linux-musl binary, so it would pass linux test, but could fail when musl is absent or ARM
  • There is a linux binary named lnx
  • tianon/gosu linux binary is called gosu_amd64

@jankatins
Copy link
Contributor Author

jankatins commented Jun 1, 2022

I was thinking more of an exclusion pattern: if it contains linux, don't install on Mac/windows and similar for the other OS. At least I would never download anything with linux in the filename if i want to run it on a Mac. So first remove known bad/wrong ones, then pick the best of the rest for download and install.

An ice to verify something might also be nice: better an error in install than later when you want to use something...

@jankatins
Copy link
Contributor Author

FYI: git absorb 0.6.7 has now a linux + mac + windows binary (release workflow similar to ripgrep ones), so the current release cannot be used to run tests against it... 0.6.6 is the last one with an linux only binary.

@vladdoster
Copy link
Member

vladdoster commented Jun 8, 2022

@jankatins It can be still tested against using ver"0.6.6" ice, right?

@vladdoster
Copy link
Member

vladdoster commented Jan 15, 2023

@jankatins,

I've actually come to think this would be a very nice QoL improvement. Thanks for suggesting it, and I apologize for writing it off so quickly.

E.g., exa is unavailable for arm64 but currently selects armv7. Causing you to comment it out or add dumb workarounds.

So... were you volunteering to implement it ;).

I joke.

In all seriousness, what would you expect the behavior to be?

  1. Absolutely no possible release asset match (i.e., wrong architecture)
  2. Possible release asset (i.e., no architecture in release asset name)

Screenshot 2023-01-15 at 12 27 11

@jankatins
Copy link
Contributor Author

jankatins commented Jan 15, 2023

My idea would be to write something like this (in pseudo-python code...)

os = ... whatever I am ...
filenames ["...mac...","...linux...", "...unknown..."]
possible_filenames = []

# first remove any filename where we are sure it's a different OS/Arch/...
for filename in all_filenames:
    if os != "linux" and linux_regex.match(filename):
        # We assume this is a linux, but we are not linux, so remove this filename
        continue
    ... more ifs which exclude filenames, e.g. based on OS or arch or ...
    possible_filenames.append(filename)

# Error out if nothing survives...
if possible_filenames = []: 
    print("Couldn't find a release for this platform, stopping...")
    sys.exit(1)

# ... and here the old logic which chooses the best filename, just now from possible_filenames...

E.g. in the volta example above and on macos, it would at least remove the filenames with windows and linux and probably end up with the two macosx files. And on x86, it would also remove the aarch64 one. On aarch64 it would result in two files and now the code has to choose the better one, which hopefully would result in the aarch64 one, as it is the more specific one.

@jankatins
Copy link
Contributor Author

jankatins commented Sep 8, 2023

Stumbled over it again: #575 (this time for eza, a replacement for the unmaintained exa...)

@vladdoster vladdoster added enhancement New feature or request good first issue Issue that are great for first-time contributors help wanted Extra attention is needed feature request Add new functionality labels Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request feature request Add new functionality good first issue Issue that are great for first-time contributors help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants