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

lua: fix on darwin by using makeBinaryWrapper #172749

Merged
merged 1 commit into from
May 17, 2022

Conversation

zaphar
Copy link
Contributor

@zaphar zaphar commented 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 #171473 and #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.

  • 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.

cc: @alerque

@zaphar
Copy link
Contributor Author

zaphar commented May 12, 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

cc: @alerque in case he knows the cause of the above.

@zaphar zaphar mentioned this pull request May 12, 2022
13 tasks
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label May 12, 2022
@zaphar
Copy link
Contributor Author

zaphar commented May 12, 2022

Another issue I'm working through is this one:

$> NIXPKGS_ALLOW_BROKEN=1 nix run --impure .#sile -- --help                                                                                                                                                                ~/projects/nixpkgs

error: builder for '/nix/store/d5s9q2hgpxp3qhwh4qys8mq344a46rrf-lua5.4-compat53-0.7-1.drv' failed with exit code 1;
       last 10 log lines:
       > updateAutotoolsGnuConfigScriptsPhase
       > configuring
       > building
       > installing
       > Missing dependencies for compat53 0.7-1:
       >    lua >= 5.1, < 5.4 (not installed)
       >
       > compat53 0.7-1 depends on lua >= 5.1, < 5.4 (not installed)
       >
       > Error: Could not satisfy dependency lua >= 5.1, < 5.4: Rock lua 5.4-1 is already provided by VM or via 'rocks_provided' in the config file.
       For full logs, run 'nix log /nix/store/d5s9q2hgpxp3qhwh4qys8mq344a46rrf-lua5.4-compat53-0.7-1.drv'.
error: 1 dependencies of derivation '/nix/store/0fykn71wjj4jk2mqjcg4khkwxjidpabq-lua-5.4.3-env.drv' failed to build
error: 1 dependencies of derivation '/nix/store/rg8m4ccf4yzb0i4rr4zp1hqypk64vqvs-sile-0.12.5.drv' failed to build

It appears to be unrelated to my change specifically and is perhaps due the compat version being incompatible with the lua version.

@ofborg ofborg bot requested a review from doronbehar May 12, 2022 20:32
@zaphar zaphar changed the title Fix sile wrapper issues on darwin. lua: fix on darwin by using makeBinaryWrapper May 12, 2022
@zaphar
Copy link
Contributor Author

zaphar commented May 12, 2022

Running luacheck succeeds with this change.

$> nix run --impure .#lua53Packages.luacheck -- --help                                                                                                                                              ~/projects/nixpkgs
Usage: luacheck ([--config <config>] | [--no-config])
       ([--default-config <default_config>] | [--no-default-config])
       [-h] [-g] [-u] [-r] [-a] [-s] [--no-self] [--std <std>] [-c]
       [-d] [-t] [-m] [--max-line-length <length>]
       [--no-max-line-length] [--max-code-line-length <length>]
       [--no-max-code-line-length] [--max-string-line-length <length>]
       [--no-max-string-line-length]
       [--max-comment-line-length <length>]
      ....

Copy link
Contributor

@thiagokokada thiagokokada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an unrelated cargo-feature commit here.

@zaphar
Copy link
Contributor Author

zaphar commented May 12, 2022

Whoops sorry about that cargo-feature commit. I'll remove it. I think a rebase went wrong on my end.

@zaphar
Copy link
Contributor Author

zaphar commented May 12, 2022

The only other package I can find with lua as a dependency that is also marked broken on darwin is pdns but it is marked broken on darwin anyway since it requires systemd.

@zaphar
Copy link
Contributor Author

zaphar commented May 12, 2022

The only remaining issue I have to work through is the libtexpdf problem when the checkPhase runs from above. But that's about all the time I have for today. I'll get back to it tomorrow if I can. @alerque , if you know what that is happening let me know.

@doronbehar
Copy link
Contributor

Quoting from #172341 :

I think it was marked as broken on Darwin due to wrapper issues (#171473), but it shouldn't be in general. I don't know what that is about.

@alerque I see now that lua5.4-zlib was broken and that's what caused sile to be marked as broken by nixpkgs-review.

As for the compat53 issue, perhaps @alerque you'd wish to open a new PR with this diff:

diff --git c/pkgs/tools/typesetting/sile/default.nix i/pkgs/tools/typesetting/sile/default.nix
index 4bff8ab3ae3..5e83cdd766f 100644
--- c/pkgs/tools/typesetting/sile/default.nix
+++ i/pkgs/tools/typesetting/sile/default.nix
@@ -18,7 +18,6 @@ let
   luaEnv = lua.withPackages(ps: with ps; [
     cassowary
     cosmo
-    compat53
     linenoise
     lpeg
     lua-zlib
@@ -33,6 +32,8 @@ let
     penlight
     stdlib
     vstruct
+  ] ++ lib.optionals (lib.versionOlder lua.luaversion "5.3") [
+    compat53
   ]);
 in

Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff looks pretty good, besides the libtexpdf issue and the doCheck now disabled on sile. cc @teto for a 2nd eye

@zaphar zaphar force-pushed the darwin-sile branch 2 times, most recently from 9728b75 to eeb8082 Compare May 13, 2022 19:08
@zaphar
Copy link
Contributor Author

zaphar commented May 13, 2022

I keep getting bogged down in figuring out the libtexpdf and compat53 stuff. I'm going to exclude them from this change for now. I can workaround the libtexpdf thing with a doCheck override for the moment and the compat53 thing should be excluded from this change anyway I think.

@zaphar zaphar force-pushed the darwin-sile branch 2 times, most recently from ff6b011 to c3ce148 Compare May 13, 2022 19:32
@zaphar
Copy link
Contributor Author

zaphar commented May 16, 2022

I've made the changes to leave the makeWrapper interface in place and tested the same way as above. I'll note that this diverges from the way python and ruby make this change but I don't think that's necessarily problematic. It just means lua is setup slightly different from the other interpreters.

@vcunat
Copy link
Member

vcunat commented May 21, 2022

Lua stuff now got very broken on aarch64-darwin, apparently. And this commit is the main suspect chosen from the range where it started. Log: https://hydra.nixos.org/build/177341824 PR: #173605.

@zaphar
Copy link
Contributor Author

zaphar commented May 21, 2022

That log doesn't give much to go on. Is it just timing out or is something else going on?

@vcunat
Copy link
Member

vcunat commented May 22, 2022

I don't know, I don't have any compatible machine. I at least forced Hydra to retry a few times, but it always failed.

@zaphar
Copy link
Contributor Author

zaphar commented May 22, 2022 via email

vcunat added a commit that referenced this pull request May 22, 2022
This reverts commit 92f4c6e.
On aarch64-darwin this completely broke lua instead of improving it;
let's revert at least until that's resolved.
#172749 (comment)
@zaphar
Copy link
Contributor Author

zaphar commented May 22, 2022

I was not able to replicate this on my my fix branch but I was able to replicate on staging. If it is this change then it's an interaction between this change and the other changes on staging. I may not be able to dig in any more till next week sometime though.

@vcunat
Copy link
Member

vcunat commented May 22, 2022

I noticed in the meantime that also some coqPackages.* also now regressed in a similar way; example: https://hydra.nixos.org/build/177668313 Still, this revert really did avoid the mass lua failure.

@zaphar
Copy link
Contributor Author

zaphar commented May 22, 2022

Digging in a little more it would appear that the lua binary is what is getting killed with prejudice. It's not clear why to me yet. Nor why it works in my branch but not staging. At a first guess perhaps something changed with the binaryWrapper that is causing it to get killed?

@zaphar
Copy link
Contributor Author

zaphar commented May 22, 2022

Here is why lua is getting killed. It looks like a codesigning issue with the binaryWrapper that was introduced in staging.

Translated Report (Full Report Below)
-------------------------------------

Incident Identifier: D3F22F82-1FD7-4F98-83C9-84216E3802B4
CrashReporter Key:   E2FFED06-E6C8-B5A0-2FB3-130A4763A6F2
Hardware Model:      MacBookPro17,1
Process:             lua [67977]
Path:                /Volumes/VOLUME/*/lua
Identifier:          lua
Version:             ???
Code Type:           ARM-64 (Native)
Role:                Unspecified
Parent Process:      bash [67945]
Coalition:           org.nixos.nix-daemon [79]
Responsible Process: nix [127]

Date/Time:           2022-05-17 21:10:01.4506 -0400
Launch Time:         2022-05-17 21:10:01.3506 -0400
OS Version:          macOS 12.3.1 (21E258)
Release Type:        User
Report Version:      104

Exception Type:  EXC_BAD_ACCESS (SIGKILL (Code Signature Invalid))
Exception Subtype: UNKNOWN_0x32 at 0x0000000100d64000
Exception Codes: 0x0000000000000032, 0x0000000100d64000
VM Region Info: 0x100d64000 is in 0x100d64000-0x100d6c000;  bytes after start: 0  bytes before end: 32767
      REGION TYPE                    START - END         [ VSIZE] PRT/MAX SHRMOD  REGION DETAIL
      UNUSED SPACE AT START
--->  mapped file                 100d64000-100d6c000    [   32K] r-x/r-x SM=COW  ...t_id=a65ec1eb
      mapped file                 100d6c000-100d70000    [   16K] rw-/rw- SM=COW  ...t_id=a65ec1eb
Exception Note:  EXC_CORPSE_NOTIFY
Termination Reason: CODESIGNING 2 

Highlighted by Thread:  0

Backtrace not available

No thread state (register information) available

Binary Images:
               0x0 - 0xffffffffffffffff ??? (*) <00000000-0000-0000-0000-000000000000> ???

Error Formulating Crash Report:
_dyld_process_info_create failed with 6
dyld_process_snapshot_get_shared_cache failed
Failed to create CSSymbolicatorRef - corpse still valid ¯\_(ツ)_/¯
thread_get_state(PAGEIN) returned 0x10000003: (ipc/send) invalid destination port
thread_get_state(EXCEPTION) returned 0x10000003: (ipc/send) invalid destination port
thread_get_state(FLAVOR) returned 0x10000003: (ipc/send) invalid destination port

EOF

-----------
Full Report
-----------

{"app_name":"lua","timestamp":"2022-05-17 21:10:08.00 -0400","app_version":"","slice_uuid":"00000000-0000-0000-0000-000000000000","build_version":"","platform":0,"share_with_app_devs":0,"is_first_party":1,"bug_type":"309","os_version":"macOS 12.3.1 (21E258)","incident_id":"D3F22F82-1FD7-4F98-83C9-84216E3802B4","name":"lua"}
{
  "uptime" : 130000,
  "procLaunch" : "2022-05-17 21:10:01.3506 -0400",
  "procRole" : "Unspecified",
  "version" : 2,
  "userID" : 302,
  "deployVersion" : 210,
  "modelCode" : "MacBookPro17,1",
  "procStartAbsTime" : 3314279617860,
  "coalitionID" : 79,
  "osVersion" : {
    "train" : "macOS 12.3.1",
    "build" : "21E258",
    "releaseType" : "User"
  },
  "captureTime" : "2022-05-17 21:10:01.4506 -0400",
  "incident" : "D3F22F82-1FD7-4F98-83C9-84216E3802B4",
  "bug_type" : "309",
  "pid" : 67977,
  "procExitAbsTime" : 3314282012480,
  "translated" : false,
  "cpuType" : "ARM-64",
  "procName" : "lua",
  "procPath" : "\/Volumes\/VOLUME\/*\/lua",
  "parentProc" : "bash",
  "parentPid" : 67945,
  "coalitionName" : "org.nixos.nix-daemon",
  "crashReporterKey" : "E2FFED06-E6C8-B5A0-2FB3-130A4763A6F2",
  "responsiblePid" : 127,
  "responsibleProc" : "nix",
  "wakeTime" : 1722,
  "sleepWakeUUID" : "4FBB9758-0D22-4F9E-B8B4-9F9324F63C59",
  "sip" : "enabled",
  "vmRegionInfo" : "0x100d64000 is in 0x100d64000-0x100d6c000;  bytes after start: 0  bytes before end: 32767\n      REGION TYPE                    START - END         [ VSIZE] PRT\/MAX SHRMOD  REGION DETAIL\n      UNUSED SPACE AT START\n--->  mapped file                 100d64000-100d6c000    [   32K] r-x\/r-x SM=COW  ...t_id=a65ec1eb\n      mapped file                 100d6c000-100d70000    [   16K] rw-\/rw- SM=COW  ...t_id=a65ec1eb",
  "isCorpse" : 1,
  "exception" : {"codes":"0x0000000000000032, 0x0000000100d64000","rawCodes":[50,4309008384],"type":"EXC_BAD_ACCESS","signal":"SIGKILL (Code Signature Invalid)","subtype":"UNKNOWN_0x32 at 0x0000000100d64000"},
  "termination" : {"namespace":"CODESIGNING","flags":0,"code":2},
  "vmregioninfo" : "0x100d64000 is in 0x100d64000-0x100d6c000;  bytes after start: 0  bytes before end: 32767\n      REGION TYPE                    START - END         [ VSIZE] PRT\/MAX SHRMOD  REGION DETAIL\n      UNUSED SPACE AT START\n--->  mapped file                 100d64000-100d6c000    [   32K] r-x\/r-x SM=COW  ...t_id=a65ec1eb\n      mapped file                 100d6c000-100d70000    [   16K] rw-\/rw- SM=COW  ...t_id=a65ec1eb",
  "extMods" : {"caller":{"thread_create":0,"thread_set_state":0,"task_for_pid":0},"system":{"thread_create":0,"thread_set_state":0,"task_for_pid":0},"targeted":{"thread_create":0,"thread_set_state":0,"task_for_pid":0},"warnings":0},
  "usedImages" : [
  {
    "size" : 0,
    "source" : "A",
    "base" : 0,
    "uuid" : "00000000-0000-0000-0000-000000000000"
  }
],
  "legacyInfo" : {
  "threadHighlighted" : 0
},
  "reportNotes" : [
  "_dyld_process_info_create failed with 6",
  "dyld_process_snapshot_get_shared_cache failed",
  "Failed to create CSSymbolicatorRef - corpse still valid ¯\\_(ツ)_\/¯",
  "thread_get_state(PAGEIN) returned 0x10000003: (ipc\/send) invalid destination port",
  "thread_get_state(EXCEPTION) returned 0x10000003: (ipc\/send) invalid destination port",
  "thread_get_state(FLAVOR) returned 0x10000003: (ipc\/send) invalid destination port"
]
}

@zaphar
Copy link
Contributor Author

zaphar commented May 22, 2022

@ncfavier, Is it possible that your changes here: #172366 broke the binaryWrapper on some platforms?

@ncfavier
Copy link
Member

ncfavier commented May 22, 2022

If there's a culprit in that PR it's most certainly 8b79ef2 (#172366). Maybe cctools wasn't enough after all, though it's strange that the makeBinaryWrapper tests succeeded on aarch64-darwin in my test PR.

@zaphar
Copy link
Contributor Author

zaphar commented May 22, 2022

If there's a culprit in that PR it's most certainly 8b79ef2 (#172366). Maybe cctools wasn't enough after all, though it's strange that the makeBinaryWrapper tests succeeded on aarch64-darwin in my test PR.

It seems to one of those things that is only showing up when both this, and that change both landed. Lua wasn't using the binaryWrapper until this was merged in. I don't know if the Coq tests referenced above are also related or not.

@ncfavier
Copy link
Member

Seems like coq_makefile is indeed a binary wrapper.

@ncfavier
Copy link
Member

@zaphar would you be able to test #174038 ?

@zaphar
Copy link
Contributor Author

zaphar commented May 22, 2022

@ncfavier, I'll give it a try tomorrow.

@ncfavier
Copy link
Member

I see what's happening now: stripping binaries invalidates the signature (see #105026 (comment)). This is supposed to be avoided by having a wrapped version of strip that recreates the signature, but that wrapped version lives in cc, and cctools contains the non-wrapped version.

Indeed, in a previous build of coq we can see this:

strip is /nix/store/qlicbgwj6mddbcdcfsqqp7g5fq3rphd1-clang-wrapper-11.1.0/bin/strip

whereas in the current build, it's

strip is /nix/store/bi8ya5scc3305x4wa4nay5gmvriq4pxv-cctools-port-949.0.1/bin/strip

I am now convinced that #174038 fixes this.

vcunat added a commit that referenced this pull request May 23, 2022
This reverts commit 9f4060c.
After the previous merge commit this should work now.
@vcunat
Copy link
Member

vcunat commented May 23, 2022

OK, looks good on Hydra: https://hydra.nixos.org/eval/1763210#tabs-now-succeed

dasJ added a commit that referenced this pull request May 23, 2022
* origin/staging-next: (62 commits)
  Re-Revert "lua: fix on darwin by using makeBinaryWrapper (#172749)"
  openldap: fix cross-compilation
  makeBinaryWrapper: fix codesign on aarch64-darwin
  python3Packages.ldap: fix linking with openldap 2.5+
  Revert "lua: fix on darwin by using makeBinaryWrapper (#172749)"
  wine: enable parallel build again
  pkgsi686Linux.gdb: fix formatting for 32-bit systems
  gtk4: Fix incorrect merge
  nixos/openldap: use upstream unit defaults
  openldap: update maintainers
  openldap: 2.4.58 -> 2.6.2
  Revert "Add mingwW64-llvm cross-system."
  lua: fix on darwin by using makeBinaryWrapper (#172749)
  python310Packages.python-mimeparse: execute tests
  pandas: fix darwin build
  gtk3: 3.24.33 -> 3.24.33-2022-03-11
  gtk4: patch fixing g-c-c crashes
  e2fsprogs: patch for CVE-2022-1304
  firefox-unwrapped: fix cross compilation
  rustc: expose correct llvmPackages for cross compile
  ...
@zaphar
Copy link
Contributor Author

zaphar commented May 23, 2022

@vcunat, @teto, @ncfavier, @doronbehar, and @alerque , Thanks for the quick action on these. As a nix user on darwin I really appreciate the patience for my first contribution to nixpkgs.

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.

9 participants