[bug]: zinit downloads wrong lnav binary on mac

jankatins commented 2 years ago

What happened?

I'm running zinit default-ice --quiet as'null' from"gh-r" lbin'!' lucid nocompile nocompletions && zinit lbin'!' for tstack/lnav and on my intel mac, it downloads the musl build instead of the mac build:

[09:18:15] [1] ✗  exec zsh

Downloading tstack/lnav…
(Requesting `lnav-0.11.0-musl-64bit.zip'…)
#################################################################################################################################################################################################################### 100.0%
ziextract: Unpacking the files from: `lnav-0.11.0-musl-64bit.zip'…
Archive:  lnav-0.11.0-musl-64bit.zip
   creating: lnav-0.11.0/
  inflating: lnav-0.11.0/lnav
  inflating: lnav-0.11.0/README
  inflating: lnav-0.11.0/NEWS
ziextract: Successfully extracted and assigned +x chmod to the file: `lnav-0.11.0/lnav'.
linkbin annex: Created the lnav soft link and set +x on the lnav binary

[09:46:52] λ  file /Users/jankatins/.local/share/zinit/plugins/tstack---lnav/lnav-0.11.0/lnav
/Users/jankatins/.local/share/zinit/plugins/tstack---lnav/lnav-0.11.0/lnav: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), statically linked, with debug_info, not stripped

Available downloads include a file named *os-x*:

Steps to reproduce

Run zinit default-ice --quiet as'null' from"gh-r" lbin'!' lucid nocompile nocompletions && zinit lbin'!' for tstack/lnav on a intel mac
Get a failure when trying to run lnav

Operating System & Version

Zsh version

zsh 5.9 (x86_64-apple-darwin21.3.0)

Terminal emulator

wezterm, xterm-256color

Code of Conduct

[X] I agree to follow this project's Code of Conduct

vladdoster commented 2 years ago

Shooting from the hip, but I assume the 64 is throwing it off.

Was this working at some point?

Tangential, but isn't your example missing a from"gh-r" ice?

jankatins commented 2 years ago

The gh-r is in a default ice, added it above. Sorry!

I just tried it after getting the release notification from lnav, so it never ran before on my system. :-)

I think I ~~will~~ also opened a issue/PR at the lnav repo to ask if some better names are possible.

I wonder if some negative rules would make the selection more stable: exclude known non-working items and only then pick the best. E.g.:

if 'musl|linux' in filename, then don't allow it for non-linux systems
if 'os{-,}x|mac' in filename, then don't use it for non-mac systems

vladdoster commented 2 years ago

@jankatins

Dum questions:

why the duplicate lbin'!' ice?
afaik, the for syntax is unnecessary

zi default-ice \
  as'null' \
  from'gh-r' \
  lbin'!' \
  lucid \
  nocompile \
  nocompletions \
  --quiet

zi light tstack/lnav

vladdoster commented 2 years ago

btw, try out the fix in PR 368.

vladdoster commented 2 years ago

~/.local/share/zinit/zinit.git fix/osx-ghr-pattern*
ᐳ zi delete tstack/lnav -y; zi ice \
  as'null' \
  from'gh-r' \
  lbin'!' \
  lucid \
  nocompile \
  nocompletions \
  --quiet; zi light @tstack/lnav; printf "\nlocation: $(which lnav)\nversion: $(lnav --version)"
linkbin annex: Removed lnav soft link

Done (action executed, exit code: 0)

Downloading tstack/lnav…
(Requesting `lnav-0.11.0-os-x.zip'…)
################################################################################################################################################################################# 100.0%
ziextract: Unpacking the files from: `lnav-0.11.0-os-x.zip'…
Archive:  lnav-0.11.0-os-x.zip
   creating: lnav-0.11.0/
  inflating: lnav-0.11.0/lnav
  inflating: lnav-0.11.0/README
  inflating: lnav-0.11.0/NEWS
ziextract: Successfully extracted and assigned +x chmod to the file: `lnav-0.11.0/lnav'.
linkbin annex: Created the lnav soft link and set +x on the lnav binary

location: /Users/null/.local/share/zinit/polaris/bin/lnav
version: lnav 0.11.0-dirty
~/.local/share/zinit/zinit.git fix/osx-ghr-pattern*
ᐳ

vladdoster commented 2 years ago

But I can't figure out where in the tstack/lnav gh actions the macOS binary gets built. I only see Linux and Windows-related jobs.

jankatins commented 2 years ago

why the duplicate lbin'!' ice?

Because I copied from the lines above and below where I changed the lbin because I had to adjust the path/names to the binary and the default ice line was out of the window :-)

afaik, the for syntax is unnecessary

Just tested it and it didn't work:

[18:55:43] λ  zinit default-ice --quiet as'null' from"gh-r" lbin'!' lucid nocompile nocompletions
[18:55:44] λ  zinit @imsnif/bandwhich
ERROR: Unknown subcommand: `@imsnif/bandwhich` (it should be one of, e.g.: `load`, `snippet`, `update`, `delete`, …, e.g.: `zinit load username/reponame`) or a for-based command body (i.e.: e.g.: `zinit …ice-spec… for …(plugin or snippet) ID-1 ID-2 ⋯⋯…`). See `help` for a more detailed usage information and the list of the subcommands.

Re builds for the release: yes, I also looked and the produced filenames were also not consistent with the ones in the release... Let's see what https://github.com/tstack/lnav/issues/1036 results in :-)

Just tested #368 on my linux system and there it works. Will test on Monday on the osx system.

vladdoster commented 2 years ago

why the duplicate lbin'!' ice?

Because I copied from the lines above and below where I changed the lbin because I had to adjust the path/names to the binary and the default ice line was out of the window :-)

afaik, the for syntax is unnecessary

Just tested it and it didn't work:
[18:55:43] λ  zinit default-ice --quiet as'null' from"gh-r" lbin'!' lucid nocompile nocompletions
[18:55:44] λ  zinit @imsnif/bandwhich
ERROR: Unknown subcommand: `@imsnif/bandwhich` (it should be one of, e.g.: `load`, `snippet`, `update`, `delete`, …, e.g.: `zinit load username/reponame`) or a for-based command body (i.e.: e.g.: `zinit …ice-spec… for …(plugin or snippet) ID-1 ID-2 ⋯⋯…`). See `help` for a more detailed usage information and the list of the subcommands.

You're missing a light or load ice, but it was a bad comment.

I didn't check, but I could be wrong due to differences in ice vs. default-ice logic.

$ zi delete tstack/lnav -y
$ zi ice \
    as'null' \
    from'gh-r' \
    lbin'!' \
    lucid \
    nocompile \
    nocompletions \
    --quiet
$ zi load @tstack/lnav;

Builds for the release: yes, I also looked, and the produced filenames were also not consistent with the ones in the release... Let's see what tstack/lnav#1036 results in :-)

Sounds good.

Just tested #368 on my linux system and there it works. Will test on Monday on the osx system.

I've tested on personal systems (Intel, M1, and M2), and the zunit tests pass. FYI, I think it won't work if you run macOS 11. Confirmed it works on macOS 12.x & 13.0 Beta.

jankatins commented 2 years ago

Just tested #368 on my mac and it works.

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 3.8.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

zdharma-continuum / zinit