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 wrapper issues on darwin. #172407

Closed
wants to merge 1 commit into from
Closed

Conversation

zaphar
Copy link
Contributor

@zaphar zaphar commented May 10, 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 #171473 and #23018 for more information. This
uses that binary wrapper to fix packages like sile.

cc: @doronbehar, alerque

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@zaphar
Copy link
Contributor Author

zaphar commented May 10, 2022

I didn't see a maintainers list for the lua interpreters package so if there is someone else I should be cc-ing on this let me know.

I'm not sure if it's worth having this be conditionally used on darwin as opposed to all platforms but that would also be an option.

@zaphar
Copy link
Contributor Author

zaphar commented May 10, 2022

I have tested on a darwin M1 macbook and it seems to work. I have not tested on linux or x86 platforms.

@zaphar
Copy link
Contributor Author

zaphar commented May 10, 2022

This cannot be considered complete yet but it appears to work for
sile at least. There is an issue in the checkPhase that has something to do with libtexpdf that seems unrelated but I'm still digging into it.

/nix/store/kmhw99yxbwgqai9v8b3rwb4nf31z1cml-lua-5.2.4/bin/lua: error loading module 'justenoughlibtexpdf' from file './core/justenoughlibtexpdf.so':
	dlopen(./core/justenoughlibtexpdf.so, 0x0006): Library not loaded: /nix/store/q97r3hiic0jg6ks95xylazf9m21hd1wh-sile-0.12.5/lib/libtexpdf.0.dylib
  Referenced from: /Users/zaphar/projects/nixpkgs/sile-0.12.5/core/justenoughlibtexpdf.so
  Reason: tried: '/nix/store/q97r3hiic0jg6ks95xylazf9m21hd1wh-sile-0.12.5/lib/libtexpdf.0.dylib' (no such file), '/usr/local/lib/libtexpdf.0.dylib' (no such file), '/usr/lib/libtexpdf.0.dylib' (no such file)
stack traceback:
	[C]: in ?
	[C]: in function 'require'
	./core/libtexpdf-output.lua:1: in main chunk
	[C]: in function 'require'
	./core/sile.lua:78: in function 'init'
	./sile:44: in main chunk
	[C]: in ?
Syntax Error: Document stream is empty
make: *** [Makefile:1381: selfcheck] Error 1

@doronbehar
Copy link
Contributor

Hello!

  1. Diff looks pretty good, though I doubt there aren't any other small fixes for individual packages this affects, that need to be taken care of.
  2. Could you please not cc me in the commit message? If that commit would have been merged, every user pushing that commit to their fork will send me an email notification.
  3. cc @alerque for the libtexpdf issue.
  4. This should target branch staging, as it triggers too many rebuilds.
  5. It'd be nice to see a few examples of packages that build fine with this patch, just to see that it doesn't ruin every package that uses lua. Try packages from the luaPackages attribute set, such as luacheck (it has a CLI interface) and check whether it's wrapper works as expected. It'd be also nice to test packages that plainly use lua as a dependency but don't use luaEnv = lua.withPackages like sile - check whether this patch affects them (it will most likely not, if the lua setup-hook works just like before.

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 12, 2022

I closed and reopened a different one to target staging at: #172749

@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants