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

Fix sile for darwin platforms now that there is a makeBinaryWrapper solution #171473

Closed
zaphar opened this issue May 4, 2022 · 9 comments
Closed
Labels
0.kind: bug 6.topic: darwin Running or building packages on Darwin 6.topic: lua

Comments

@zaphar
Copy link
Contributor

zaphar commented May 4, 2022

Describe the bug

The sile is currently broken on darwin platforms due to issue #23018. The solution for that is now fixed however. It would be nice if we could get sile deployable on darwin again using that solution.

Steps To Reproduce

Steps to reproduce the behavior:

  1. Use a darwin based platform (MacOS)
  2. run nix-shell -p sile
  3. See an error that sile is broken on the darwin platform

Expected behavior

I should get a shell with sile available

Notify maintainers

@doronbehar @alerque

@doronbehar
Copy link
Contributor

It would be nice if we could get sile deployable on darwin again using that solution.

cc @teto as the Lua eco system maintainer. Do you think we can make all Lua environment wrappers use makeBinaryWrapper?

@teto
Copy link
Member

teto commented May 4, 2022

is it easy to toggle between the plaintext and binary versions ?
I think the lua wrapping system is currently broken (at least some stuff is suboptimal) and I rely on the plaintext wrappers a lot to fix the issues.

I am ok with the idea but I need to learn about implications/how it affects the code. A PR would be nice

@zaphar
Copy link
Contributor Author

zaphar commented May 4, 2022

It would take me a while to put together a PR. I'm not familiar with the lua nixpkg ecosystem that much. I'm experimenting locally to see if I can fix it but it is going to take me a while to figure out how everything wires together and I'm a little short on time right now :-D

@veprbl veprbl added the 6.topic: darwin Running or building packages on Darwin label May 4, 2022
@FRidh
Copy link
Member

FRidh commented May 4, 2022

override your package or package set and pass in the other makeWrapper package. They are compatible, the binary just misses a few hardly used arguments if I am correct.

@zaphar
Copy link
Contributor Author

zaphar commented May 4, 2022

I have naively tried an override in a nix flake and a local copy of the sile package as an attempt to fix this:

{
  inputs = {
    nixpkgs.url = "nixpkgs";
    flake-utils.url = "github:numtide/flake-utils";
  };

  outputs = {self, nixpkgs, flake-utils}:
  flake-utils.lib.eachDefaultSystem (system:
  let
    pkgs = import nixpkgs { inherit system; };
    sile = with pkgs; (callPackage ./package.nix { makeWrapper = pkgs.makeBinaryWrapper; });
  in {
      defaultPackage = sile;
  });
}

and I still get this kind of error:

/nix/store/ysiz8w1ga4mb3psw2hiphyvjswyaa7bm-sile-0.12.5/bin/sile: line 2: SYSTEM_SILE_PATH: command not found
/nix/store/ysiz8w1ga4mb3psw2hiphyvjswyaa7bm-sile-0.12.5/bin/sile: line 3: SHARED_LIB_EXT: command not found
/nix/store/ysiz8w1ga4mb3psw2hiphyvjswyaa7bm-sile-0.12.5/bin/sile: line 5: syntax error near unexpected token `('
/nix/store/ysiz8w1ga4mb3psw2hiphyvjswyaa7bm-sile-0.12.5/bin/sile: line 5: `local executable = debug.getinfo(1, "S").short_src'

Which indicates that it isn't using the binary wrapper? I'm not sure if I'm doing something wrong or if it's less simple than just passing in the binary wrapper instead of the regular wrapper.

@Artturin
Copy link
Member

Artturin commented May 6, 2022

is it easy to toggle between the plaintext and binary versions ? I think the lua wrapping system is currently broken (at least some stuff is suboptimal) and I rely on the plaintext wrappers a lot to fix the issues.

I am ok with the idea but I need to learn about implications/how it affects the code. A PR would be nice

makeBinaryWrapper contains these lines in the binary which can be viewed just by opening it in vim for example

# ------------------------------------------------------------------------------------
# The C-code for this binary wrapper has been generated using the following command:


makeCWrapper /nix/store/jp1y8aw7i87dxqp34wk9caxav179s8li-any-nix-shell-1.2.1/bin/.any-nix-shell-wrapped \
    --inherit-argv0 \
    --prefix 'PATH' ':' '/nix/store/jp1y8aw7i87dxqp34wk9caxav179s8li-any-nix-shell-1.2.1/bin'


# (Use `nix-shell -p makeBinaryWrapper` to get access to makeCWrapper in your shell)
# ------------------------------------------------------------------------------------

@alerque
Copy link
Contributor

alerque commented May 7, 2022

I don't have access to anything where I could even test on Darwin, so I'm not much use as a reviewer here, sorry.

@zaphar
Copy link
Contributor Author

zaphar commented May 10, 2022

Did some more digging and it looks like the issue is that we actually need the luaEnv wrapper to be a binary wrapper but I'm out of my depth with that package and it will take me some time to figure it out. If someone wanted to assist I can provide a Darwin M1 platform to test on if they can guide me through where I should be making changes.

zaphar added a commit to zaphar/nixpkgs that referenced this issue May 12, 2022
Darwin execve has issues with shebang lines that point to other scripts
with shebang lines. A new makeBinaryWrapper hook was added to workaround
the issue on darwin. See NixOS#171473 and NixOS#23018 for more information. This
uses that binary wrapper to fix packages like sile.

I'm not sure this can be considered complete but it appears to work for
sile at least.
zaphar added a commit to zaphar/nixpkgs that referenced this issue May 16, 2022
Darwin execve has issues with shebang lines that point to other scripts
with shebang lines. A new makeBinaryWrapper hook was added to workaround
the issue on darwin. See NixOS#171473 and NixOS#23018 for more information. This
uses that binary wrapper to fix packages like sile.

I'm not sure this can be considered complete but it appears to work for
sile at least.
@zaphar
Copy link
Contributor Author

zaphar commented May 24, 2022

I believe now that my fix has landed in master that this can be considered fixed. But I'm not sure what rules govern closing these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug 6.topic: darwin Running or building packages on Darwin 6.topic: lua
Projects
None yet
Development

No branches or pull requests

7 participants