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

zed-editor: shrink darwin closure #369399

Merged
merged 2 commits into from
Dec 31, 2024
Merged

Conversation

niklaskorz
Copy link
Contributor

@niklaskorz niklaskorz commented Dec 30, 2024

Zed-editor's closure currently depends on three versions of apple-sdk, at least two of which are not needed at runtime.
These are determined as dependencies because (a) Zed's own binary references headers from apple-sdk_15 because of #367169 and (b) because we include args.gn and webrtc.ninja, which reference apple-sdk_14, in the output of livekit-libwebrtc.
We fix (a) by adding the app directory to stripDebugList in the zed-editor derivation.
We fix (b) by splitting livekit-libwebrtc into out and dev outputs, where out contains the libraries and dev contains everything else (headers and other artifacts required by the livekit Rust crate at build-time).
As livekit's webrtc crate expects both libraries and build-time artifacts to be in the same directory, we symlink them using pkgs.buildEnv.
Because the symlinks resolve to the split outputs, the dev output will not be considered a dependency of the final closure.

For livekit-libwebrtc, $out/lib is symlinked into $dev/lib, so it's easily found by the crate's build script but still resolves to the location in $out.
The solution to (b) might also shrink the closure of zed-editor on Linux, though I did not specifically check for it.

Furthermore, I removed the WebRTC.framework for Darwin from livekit-libwebrtc. We originally added it because libwebrtc.dylib was missing symbols that were included in WebRTC.framework.
As both the framework's contained binary and libwebrtc.dylib are shared mach objects, it was possible to move the framework's binary in the dylib's stead.
This also has the positive effect of not having any headers inside the out output, only in dev.
Also, the linker command substitution in zed-editor now looks the same for Linux and Darwin as both link to dynamic libraries.

Diff of nix-store -qR $(nix-build -A zed-editor)
@@ -6,7 +6,6 @@
 /nix/store/0q652rqzd86cnllgmkx2n7b3c7434jx8-ncurses-6.4.20221231
 /nix/store/0v8fw166lwzfvaczrzbhhpq1kz9bvdkm-zvbi-0.2.43
 /nix/store/1bdb707v8jph12q0b3rjqwn1xwjkynp2-gnugrep-3.11
-/nix/store/1i0ri6dfxqqw42bxqyxjn4m5j4ryfcs9-cups-headers-2.4.11
 /nix/store/1q5kzfka1jc0xpmykyl34zcvlaiw577r-libgit2-1.8.4-lib
 /nix/store/1qw1mgw2ql5hvvd48lp7bs86cr7v1am3-perl5.40.0-HTTP-Date-6.06
 /nix/store/1rc4m04p36j8rhzkx7y5zzawlfv1l91n-perl5.40.0-Try-Tiny-0.31
@@ -16,12 +15,10 @@
 /nix/store/2hzpfj0iq63yrsnsb8826izw7r3457qp-fontconfig-2.15.0-lib
 /nix/store/2n30zp09iizmhw74srp7ndyib2kg0xar-perl5.40.0-Digest-HMAC-1.04
 /nix/store/2qjlmgs596j29p56l049akf89hkp85x7-readline-8.2p13
-/nix/store/2r0sf27ybwdi55s86w5drfik0b6s6i7c-livekit-libwebrtc-m114
 /nix/store/2y71rl5mnazwnpwpcq07xx97jl7sy5lh-apple-sdk-11.3
 /nix/store/38pgm27l3gvjw9w8llkj8n7k41g3hxs6-nghttp2-1.64.0-lib
 /nix/store/3rm748j9y1k596rq4rm1g49cm2lc9n0y-openmp-19.1.5
 /nix/store/42cbzwc93i3vnv339dx66nrgznspq8h6-perl5.40.0-libnet-3.15
-/nix/store/42pzdm5r1px15z4g1xyz878cz0ymy8fp-libSystem-B
 /nix/store/45f6w6lb62hhq64lgj3ninvdwxwgr1zx-libpng-apng-1.6.43
 /nix/store/4hk24pjm7zhzf14510xng602lkxmmr7d-perl5.40.0-HTTP-Negotiate-6.01
 /nix/store/4i20lpy0vwdg32hvlqnk861lfwcxvvng-perl5.40.0-IO-Socket-SSL-2.083
@@ -60,6 +57,7 @@
 /nix/store/afm1lk6939qifg2z1vmfwyjc8xmry2ij-libutil-72
 /nix/store/azg3i5v5i675924xb66mhxz2q1xsjgb6-perl5.40.0-Mozilla-CA-20230821
 /nix/store/b6vhxijkxw06znzvvzb6a1bn3jffz8rq-x265-3.6
+/nix/store/b98ygnz1ahgjf6ipa42m90cbl8xpi5k4-livekit-libwebrtc-m114
 /nix/store/bi58hj6aqbswrwngsdzszzdf35vzsl36-git-2.47.0
 /nix/store/bkqrnyz9az7040ws3sv7n2hlnnvr0lj9-perl5.40.0-Clone-0.46
 /nix/store/bzx98yb86791m7jbwinz23m0xjn2inag-pcre2-10.44
@@ -80,7 +78,6 @@
 /nix/store/dmk8pjlgslv6di92i8ca54l4sigrh9zp-libsbuf-14.1.0-dev
 /nix/store/dzbxz3pglzhwnz01475xqj6p8y0rdm78-gnused-4.9
 /nix/store/fandazhpczvsp04d025ky56fkyy3k5gr-dejavu-fonts-minimal-2.37
-/nix/store/g2ys0z8vdlw845izq6454kc2z2jpcdmr-expand-response-params
 /nix/store/galb9cxwnm2z3kdnv7wwki1wl3mc6gh7-librist-0.2.11
 /nix/store/gd2546zkaizg9cfgq3rfawvr2d9xxfk5-libresolv-83
 /nix/store/gnm2kjc7jbghb4napf2m4q6lb5ivlhsq-libbluray-1.3.4
@@ -101,6 +98,7 @@
 /nix/store/ixvnjsa3vf9plg0zqpkch02pq79nj0df-mailcap-2.1.54
 /nix/store/j046ap9wagpc74xmxa8m5hfa8cf42kwi-perl5.40.0-HTML-Tagset-3.20
 /nix/store/j18mgkpi7vax928fhgj3yg6ifg05hsx6-libopenmpt-0.7.11
+/nix/store/j8262czylskmrzv56fmw8j565jyms6ib-zed-editor-0.166.2
 /nix/store/jgmpmh6dkp4hrm1i2p0n8a6i0s1cxq45-x264-0-unstable-2023-10-01-lib
 /nix/store/k89nbm81ng303b5mknxzgv0aasjidzlg-fontconfig-2.15.0
 /nix/store/kaplvrb0niv3cfq12npgjnyy7p3jw4bb-perl5.40.0-Authen-SASL-2.1700
@@ -113,7 +111,6 @@
 /nix/store/lkwx6c5nn06nvxjldw79qx7n57pby00q-krb5-1.21.3-lib
 /nix/store/lm60b0gdds255849nyncy0pdcsfic9dn-clang-19.1.5
 /nix/store/lzma48wdzmhhdsnkw48c8lql84m1msv9-perl5.40.0-FCGI-ProcManager-0.28
-/nix/store/m26mkb9aa2bihkkrxl3bl43ik4fc6wq1-apple-sdk-14.4
 /nix/store/m3jzhwg8h7fhy913lh27rmlpp36szg8v-libiconv-107
 /nix/store/m6k870ya87a61ki602i8dg69bx8fqva1-libX11-1.8.10
 /nix/store/m79p6m0y4na9synf7yv4y4bad3c0jcjc-dav1d-1.5.0
@@ -121,7 +118,6 @@
 /nix/store/mk02p01jfb2yk86qmlgchn9a8j142mc2-libidn2-2.3.7
 /nix/store/mpf9klxlhxfq4571q4vsgbsi1jczqfgr-cups-headers-2.4.11
 /nix/store/mphsyr8h04vf2b3cmc8kccs533kpn6rp-brotli-1.1.0-lib
-/nix/store/mrqw4zq2wl0y7wrj2fazns5i9c4sc2g1-cctools-binutils-darwin-wrapper-1010.6
 /nix/store/n71549v1h1xc1blvd4g7bc00qyk46m8d-perl5.40.0-FCGI-0.82
 /nix/store/ncqqqxrccy0dasj3hy2fg81pgghpmdmx-glib-2.82.1
 /nix/store/nkf2zpjwmwssq22799xjwr15vsl9gdbb-libvmaf-3.0.0
@@ -147,13 +143,11 @@
 /nix/store/s1c6cl7rg3fjhdk97l7kccvk3gfqhcaq-perl5.40.0-libwww-perl-6.72
 /nix/store/s7y8k2drsg9ags6y9cqyfk1h1ddwbrwg-perl5.40.0-HTML-Parser-3.81
 /nix/store/sbvhcndavnzsm5wm9awzwjh211ldns08-mpg123-1.32.9
-/nix/store/sf4yiq6n0y6fhlfsl335fygylsasg2dd-zed-editor-0.166.2
 /nix/store/sycll6rh17sm0a59xb0l9m0grnv0yadf-perl5.40.0-File-Listing-6.16
 /nix/store/v9f8cn00m8ymvc0c5v4lsary7adb615n-libvpx-1.15.0
 /nix/store/vgds0yvnrbvi3c79g7jf8mzlass0bb28-libssh2-1.11.1
 /nix/store/vm7vsn05c3915y1a8va32jjgif6dv79x-ffmpeg-6.1.2-data
 /nix/store/vy90nwv7qy5agm9iwdksj45mm33kj8ml-libwebp-1.4.0
-/nix/store/wkadfwbhn9b4kdih88j7izzy20qw79bk-clang-wrapper-19.1.5
 /nix/store/xkrsn3afmdcf1pfc5vrjf47dxd6c7mwi-lerc-4.0.0
 /nix/store/xln1n349s5idq6ilwvcpabwpyw0mw0ps-llvm-19.1.5-lib
 /nix/store/xsc6bq51if8vzc1z50qhi03as0hnhblr-ld64-951.9
@@ -163,7 +157,6 @@
 /nix/store/xzmm3ikv0pinxspy0jwz4jkbjvl7815q-bzip2-1.0.8
 /nix/store/y6q3p5rxx42i9869w4a04wmhm5kgsqbr-ld64-951.9-lib
 /nix/store/y7i6q4zdljjdlyk8a020l7wmsvcbgv4q-perl5.40.0-HTTP-Cookies-6.10
-/nix/store/z12iccmc0xvvlf5l4s76fl34sbmlad54-apple-sdk-15.0
 /nix/store/z8780my4hw9aicrj7whlcy24znpsarn4-gnutls-3.8.6
 /nix/store/zdvzrlc9c6bk5krm6g0m2xva5536kqx0-freetype-2.13.3
 /nix/store/zkdm5ai2wr0jikq2yr54bdyqvv3i0hyb-perl5.40.0-CGI-Fast-2.16

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@niklaskorz
Copy link
Contributor Author

niklaskorz commented Dec 30, 2024

The buildEnv could probably be removed by symlinking livekit-libwebrtc's $out/lib into $dev in the install phase, any thoughts on that?

Edit: did that and moved dev to be the first in the outputs list, so the dev output does not have to be addressed explicitly when used in zed-editor

@emilazy
Copy link
Member

emilazy commented Dec 30, 2024

Zed-editor's closure currently depends on three versions of apple-sdk, at least two of which are not needed at runtime.

FWIW, no apple-sdk package should ever be required at runtime. They contain purely compile‐time stubs. If there’s any remaining it should be possible to remove.

Mild preference to omit (a) in favour of waiting for #369094 to make its way through the cycle.

@niklaskorz
Copy link
Contributor Author

niklaskorz commented Dec 30, 2024

Zed-editor's closure currently depends on three versions of apple-sdk, at least two of which are not needed at runtime.

FWIW, no apple-sdk package should ever be required at runtime. They contain purely compile‐time stubs. If there’s any remaining it should be possible to remove.

Mild preference to omit (a) in favour of waiting for #369094 to make its way through the cycle.

The remaining apple-sdk runtime dependency is caused by perl (which is needed by git). So that'd be the topic for a separate staging PR, as perl afaik is a transitive build dependency of almost all packages.

As for the strip debug list, I'll make sure to remove it again once the strip hook change has made its way into master. :)

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 369399


x86_64-linux

✅ 3 packages built:
  • livekit-libwebrtc
  • livekit-libwebrtc.dev
  • zed-editor

aarch64-linux

✅ 3 packages built:
  • livekit-libwebrtc
  • livekit-libwebrtc.dev
  • zed-editor

x86_64-darwin

✅ 3 packages built:
  • livekit-libwebrtc
  • livekit-libwebrtc.dev
  • zed-editor

aarch64-darwin

✅ 3 packages built:
  • livekit-libwebrtc
  • livekit-libwebrtc.dev
  • zed-editor

Copy link
Contributor

@WeetHet WeetHet left a comment

Choose a reason for hiding this comment

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

LGTM

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Dec 30, 2024
@ofborg ofborg bot requested review from WeetHet and GaetanLepage December 30, 2024 23:50
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Dec 30, 2024
Copy link
Contributor

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

LGTM !

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 369399


x86_64-linux

✅ 3 packages built:
  • livekit-libwebrtc
  • livekit-libwebrtc.dev
  • zed-editor

aarch64-linux

✅ 3 packages built:
  • livekit-libwebrtc
  • livekit-libwebrtc.dev
  • zed-editor

x86_64-darwin

✅ 3 packages built:
  • livekit-libwebrtc
  • livekit-libwebrtc.dev
  • zed-editor

aarch64-darwin

✅ 3 packages built:
  • livekit-libwebrtc
  • livekit-libwebrtc.dev
  • zed-editor

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Dec 31, 2024
@GaetanLepage GaetanLepage merged commit 8da05cf into NixOS:master Dec 31, 2024
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants