Skip to content

Comments

legcord: add autoPatchElfHook for venmic module#388827

Closed
wrmilling wants to merge 1 commit intoNixOS:masterfrom
wrmilling:legcord-autoPatchElfHook
Closed

legcord: add autoPatchElfHook for venmic module#388827
wrmilling wants to merge 1 commit intoNixOS:masterfrom
wrmilling:legcord-autoPatchElfHook

Conversation

@wrmilling
Copy link
Member

Attempting to address issue as expressed in: #383696 (comment)

Things done

Added autoPatchElfHook so that the venmic node.js module can be patched at build time.

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

@wrmilling
Copy link
Member Author

@nyabinary Can you take a look as see if this fixes the issue you are having? I will convert it to a full PR if it does resolve the problem.

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Mar 10, 2025
@nyabinary
Copy link
Contributor

nyabinary commented Mar 10, 2025

@nyabinary Can you take a look as see if this fixes the issue you are having? I will convert it to a full PR if it does resolve the problem.

Nope, didn't work, outputs this when I screenshare (in terminal).

WebRTC Capturer detected, using native window picker.
'loop->recurse > 0' failed at ../src/pipewire/thread-loop.c:425 pw_thread_loop_wait()
trying to import /nix/store/f8x40cl8ghmzb4i6ddgs4n81bifq2dk2-legcord-1.1.0/share/lib/legcord/resources/app.asar/dist/venmic-x64.node
Failed to import Venmic module Error: Cannot find module '/nix/store/f8x40cl8ghmzb4i6ddgs4n81bifq2dk2-legcord-1.1.0/share/lib/legcord/resources/app.asar/dist/venmic-x64.node'
Require stack:
- /nix/store/f8x40cl8ghmzb4i6ddgs4n81bifq2dk2-legcord-1.1.0/share/lib/legcord/resources/app.asar/ts-out/main-Dy_lruhE.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1232:15)
    at s._resolveFilename (node:electron/js2c/browser_init:2:124485)
    at Module._load (node:internal/modules/cjs/loader:1058:27)
    at c._load (node:electron/js2c/node_init:2:16955)
    at Module.require (node:internal/modules/cjs/loader:1318:19)
    at require (node:internal/modules/helpers:179:18)
    at Qt (file:///nix/store/f8x40cl8ghmzb4i6ddgs4n81bifq2dk2-legcord-1.1.0/share/lib/legcord/resources/app.asar/ts-out/main-Dy_lruhE.js:48:645)
    at G (file:///nix/store/f8x40cl8ghmzb4i6ddgs4n81bifq2dk2-legcord-1.1.0/share/lib/legcord/resources/app.asar/ts-out/main-Dy_lruhE.js:48:891)
    at file:///nix/store/f8x40cl8ghmzb4i6ddgs4n81bifq2dk2-legcord-1.1.0/share/lib/legcord/resources/app.asar/ts-out/main-Dy_lruhE.js:48:1376
    at WebContents.<anonymous> (node:electron/js2c/browser_init:2:87415) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/nix/store/f8x40cl8ghmzb4i6ddgs4n81bifq2dk2-legcord-1.1.0/share/lib/legcord/resources/app.asar/ts-out/main-Dy_lruhE.js'
  ]
}
Obtained Venmic module

@wrmilling
Copy link
Member Author

Nope, didn't work, outputs this when I screenshare (in terminal).

Thank, will try to reproduce locally, may be a bit before I can dig into it though.

@nyabinary
Copy link
Contributor

The developer of Legcord suggests that you build the core Legcord first and then build Venmic separately and copy the built binary into the folder Legcord expects it to be in. So, maybe a new drv for Venmic?
https://github.com/Legcord/Legcord/blob/dev/scripts/copyVenmic.mts
image

@wrmilling
Copy link
Member Author

I have been swamped with work, but I can hopefully take a look later today / tomorrow.

@nyabinary
Copy link
Contributor

nyabinary commented Mar 16, 2025

Didn't fix the issue, but I cleaned up the package/refactor it here to follow more with other Discord packages like Vesktop and Goofcord
#390517

@water-sucks
Copy link
Member

I took a look, and autoPatchelfHook is managing to patch the binary for venmic correctly, but it's not put in the correct place. I see the actual venmic binaries for each platform in share/lib/legcord/resources/app.asar.unpacked/node_modules/@vencord/venmic/prebuilds, but they aren't copied by the pnpm build step for electron-builder for whatever reason.

I applied this patch to this PR, and I believe venmic can work with this, with no extra derivation needed.

diff --git a/pkgs/by-name/le/legcord/package.nix b/pkgs/by-name/le/legcord/package.nix
index 4fabf7b0be01..b8a3109d85a6 100644
--- a/pkgs/by-name/le/legcord/package.nix
+++ b/pkgs/by-name/le/legcord/package.nix
@@ -52,6 +52,15 @@ stdenv.mkDerivation rec {
 
     pnpm build
 
+    # Replicating the build step to copy venmic from the vendored node module manually,
+    # since the install script does not do this for whatever reason
+    mkdir ./dist
+    cp ./node_modules/@vencord/venmic/prebuilds/venmic-addon-linux-x64/node-napi-v7.node ./dist/venmic-x64.node
+    cp ./node_modules/@vencord/venmic/prebuilds/venmic-addon-linux-arm64/node-napi-v7.node ./dist/venmic-arm64.node
+
+    # Patch venmic before putting it into the ASAR archive
+    autoPatchelf ./dist
+
     npm exec electron-builder -- \
       --dir \
       -c.electronDist="${electron_34.dist}" \

@wrmilling wrmilling force-pushed the legcord-autoPatchElfHook branch from a0bdf54 to 319bcbb Compare March 17, 2025 16:00
@wrmilling
Copy link
Member Author

wrmilling commented Mar 17, 2025

I took a look, and autoPatchelfHook is managing to patch the binary for venmic correctly, but it's not put in the correct place. I see the actual venmic binaries for each platform in share/lib/legcord/resources/app.asar.unpacked/node_modules/@vencord/venmic/prebuilds, but they aren't copied by the pnpm build step for electron-builder for whatever reason.

I applied this patch to this PR, and I believe venmic can work with this, with no extra derivation needed.

Seems to work in my limited testing, converting to a non-draft PR so tests can run.

@wrmilling wrmilling marked this pull request as ready for review March 17, 2025 16:30
@nix-owners nix-owners bot requested a review from water-sucks March 17, 2025 16:38
@nyabinary
Copy link
Contributor

I mean autoPatchelfHook is not preferable, what about creating a separate drv for venmic then copying it to there, it would remove the need for autoPatchelfHook right?

@wrmilling
Copy link
Member Author

@ofborg eval

@water-sucks
Copy link
Member

I mean autoPatchelfHook is not preferable, what about creating a separate drv for venmic then copying it to there, it would remove the need for autoPatchelfHook right?

True, but the binaries for the venmic executables are already in there, so it can save effort for now.

I'll look into making a venmic drv tonight, but taking a cursory look, making a build with Nix is kind of cursed already, since when building, cmake-js tries to fetch stuff with the network by default.

@nyabinary
Copy link
Contributor

I mean autoPatchelfHook is not preferable, what about creating a separate drv for venmic then copying it to there, it would remove the need for autoPatchelfHook right?

True, but the binaries for the venmic executables are already in there, so it can save effort for now.

I'll look into making a venmic drv tonight, but taking a cursory look, making a build with Nix is kind of cursed already, since when building, cmake-js tries to fetch stuff with the network by default.

Hmm, is there an offline flag of some kind?

@water-sucks
Copy link
Member

I mean autoPatchelfHook is not preferable, what about creating a separate drv for venmic then copying it to there, it would remove the need for autoPatchelfHook right?

True, but the binaries for the venmic executables are already in there, so it can save effort for now.
I'll look into making a venmic drv tonight, but taking a cursory look, making a build with Nix is kind of cursed already, since when building, cmake-js tries to fetch stuff with the network by default.

Hmm, is there an offline flag of some kind?

Not as far as I know of. Since this uses CPM under the hood, it ends up using FetchContent at some point and we'd probably have to generate a FOD for the repos that are being fetched, which sucks. See relevant code: https://github.com/Vencord/venmic/blob/5481a9277da95656c79ae69c4ea1146ce10cc2d7/CMakeLists.txt#L88-L121

I attempted to fetch the CPM code itself and was successful (see code below), but any further past this when running pnpm install inside the buildPhase fails completely due to trying to fetch the files. This is an incomplete derivation btw, just me experimenting and realizing that it will always try to fetch the Git repos mentioned as part of the build process.

{
  stdenv,
  fetchFromGitHub,
  pnpm,
  cmake,
  cpm-cmake
}:

stdenv.mkDerivation (finalAttrs: {
  pname = "venmic";
  version = "6.1.0";

  src = fetchFromGitHub {
    owner = "Vencord";
    repo = "venmic";
    rev = "v${finalAttrs.version}";
    hash = "sha256-0UP8a2bfhWGsB2Lg/GeIBu4zw1zHnXbitT8vU+DLeEY=";
  };

  postPatch = ''
    install -D ${cpm-cmake}/share/cpm/CPM.cmake ./build/CPM.cmake

    substituteInPlace CMakeLists.txt \
      --replace-fail 'cmake/cpm.cmake' './build/CPM.cmake'
  '';

  nativeBuildInputs = [
    pnpm.configHook
    cmake
  ];

  pnpmDeps = pnpm.fetchDeps {
    inherit (finalAttrs) pname version src;
    hash = "sha256-3LKQcFzYoNAEmcu3YVEcuwSNZTimGooSKwzmGWK1txk=";
  };

  preBuild = ''
    # don't fetch node headers
    substituteInPlace node_modules/cmake-js/lib/dist.js \
      --replace-fail '!this.downloaded' 'false'
  '';

  buildPhase = ''
    runHook preBuild

    pnpm install

    runHook postBuild
  '';
})

@nyabinary
Copy link
Contributor

I mean autoPatchelfHook is not preferable, what about creating a separate drv for venmic then copying it to there, it would remove the need for autoPatchelfHook right?

True, but the binaries for the venmic executables are already in there, so it can save effort for now.
I'll look into making a venmic drv tonight, but taking a cursory look, making a build with Nix is kind of cursed already, since when building, cmake-js tries to fetch stuff with the network by default.

Hmm, is there an offline flag of some kind?

Not as far as I know of. Since this uses CPM under the hood, it ends up using FetchContent at some point and we'd probably have to generate a FOD for the repos that are being fetched, which sucks. See relevant code: https://github.com/Vencord/venmic/blob/5481a9277da95656c79ae69c4ea1146ce10cc2d7/CMakeLists.txt#L88-L121

I attempted to fetch the CPM code itself and was successful (see code below), but any further past this when running pnpm install inside the buildPhase fails completely due to trying to fetch the files. This is an incomplete derivation btw, just me experimenting and realizing that it will always try to fetch the Git repos mentioned as part of the build process.

{
  stdenv,
  fetchFromGitHub,
  pnpm,
  cmake,
  cpm-cmake
}:

stdenv.mkDerivation (finalAttrs: {
  pname = "venmic";
  version = "6.1.0";

  src = fetchFromGitHub {
    owner = "Vencord";
    repo = "venmic";
    rev = "v${finalAttrs.version}";
    hash = "sha256-0UP8a2bfhWGsB2Lg/GeIBu4zw1zHnXbitT8vU+DLeEY=";
  };

  postPatch = ''
    install -D ${cpm-cmake}/share/cpm/CPM.cmake ./build/CPM.cmake

    substituteInPlace CMakeLists.txt \
      --replace-fail 'cmake/cpm.cmake' './build/CPM.cmake'
  '';

  nativeBuildInputs = [
    pnpm.configHook
    cmake
  ];

  pnpmDeps = pnpm.fetchDeps {
    inherit (finalAttrs) pname version src;
    hash = "sha256-3LKQcFzYoNAEmcu3YVEcuwSNZTimGooSKwzmGWK1txk=";
  };

  preBuild = ''
    # don't fetch node headers
    substituteInPlace node_modules/cmake-js/lib/dist.js \
      --replace-fail '!this.downloaded' 'false'
  '';

  buildPhase = ''
    runHook preBuild

    pnpm install

    runHook postBuild
  '';
})

Can one just package those deps and then use them in the drv? tl-expected, range-v3, glaze, and spdlog are already packaged leaving just rohrkabel and channel needing to be packaged.

@wrmilling
Copy link
Member Author

I am happy for us to go either way on this (autoPatchElf or setting up packages for the dependencies), but given my limited time to manage packages right now, do we want to go ahead with this PR anyway as a stopgap until such time as someone can fully package the dependencies in another PR?

@water-sucks
Copy link
Member

I'm personally cool with merging it as-is. I haven't had time since I've been studying for and doing midterms this week, but I'll have time after Friday to restart the venmic packaging experimenting I've been doing.

Though there's the separate update PR that probably needs to be updated after this would be merged, or vice versa, we should keep that in mind.

@nyabinary
Copy link
Contributor

I can maybe just merge this into my PR if that's okay to you? @wrmilling

@wrmilling
Copy link
Member Author

I can maybe just merge this into my PR if that's okay to you? @wrmilling

Sure, just reference this PR so this conversation/decisions don't get disconnected from what gets merged.

@nyabinary nyabinary mentioned this pull request Mar 21, 2025
13 tasks
@nyabinary
Copy link
Contributor

I can maybe just merge this into my PR if that's okay to you? @wrmilling

Sure, just reference this PR so this conversation/decisions don't get disconnected from what gets merged.

Done #390517

@nyabinary
Copy link
Contributor

Closing this, since this is merged into the other PR now :3

@nyabinary nyabinary closed this Mar 21, 2025
@nyabinary
Copy link
Contributor

I'm personally cool with merging it as-is. I haven't had time since I've been studying for and doing midterms this week, but I'll have time after Friday to restart the venmic packaging experimenting I've been doing.

Though there's the separate update PR that probably needs to be updated after this would be merged, or vice versa, we should keep that in mind.

Any update on this by the way? :P

@wrmilling wrmilling deleted the legcord-autoPatchElfHook branch March 26, 2025 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants