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

element-desktop: build native modules #124907

Closed
wants to merge 1 commit into from
Closed

element-desktop: build native modules #124907

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented May 29, 2021

Fixes #87752

There might be room for improvement, but it works.

For example, there is a tradeoff to be made between ease of updating and the size of the files that have to be included in nixpkgs as well as the output size.
An alternative approach would be building the native extensions in their own derivations. This way we could avoid having a modified yarn.lock for element-desktop shipped in the repository. However the native extensions would have to be updated seperately, and we would have to implement some of the electron build environment logic ourselves, not much, ~20 lines, but it would have to be maintained.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Added a release notes entry if the change is major or breaking
  • Fits CONTRIBUTING.md.

@r-rmcgibbo
Copy link

r-rmcgibbo commented May 29, 2021

Result of nixpkgs-review pr 124907 at c8fb00b5 run on x86_64-linux 1

1 package built successfully:
  • element-desktop
3 suggestions:
  • warning: build-tools-in-build-inputs

    yarn is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

    Near pkgs/development/tools/yarn2nix-moretea/yarn2nix/default.nix:292:7:

        |
    292 |       buildInputs = [ yarn nodejs rsync ] ++ extraBuildInputs;
        |       ^
    
  • warning: build-tools-in-build-inputs

    rsync is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

    Near pkgs/development/tools/yarn2nix-moretea/yarn2nix/default.nix:292:7:

        |
    292 |       buildInputs = [ yarn nodejs rsync ] ++ extraBuildInputs;
        |       ^
    
  • warning: missing-phase-hooks

    installPhase should probably contain runHook preInstall and runHook postInstall.

    Near pkgs/development/tools/yarn2nix-moretea/yarn2nix/default.nix:332:7:

        |
    332 |       installPhase = attrs.installPhase or ''
        |       ^
    

Result of nixpkgs-review pr 124907 at c8fb00b5 run on aarch64-linux 1

1 package built successfully:
  • element-desktop

Copy link
Contributor

@sumnerevans sumnerevans left a comment

Choose a reason for hiding this comment

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

Thank you so much for doing this!

nativeBuildInputs = [
makeWrapper
python3 rustc cargo rustPlatform.cargoSetupHook pkg-config yarn
sqlcipher libsecret # FIXME this should be in buildInputs
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: which dependency is this comment referring to?

Copy link
Author

Choose a reason for hiding this comment

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

sqlcipher and libsecret

Copy link
Author

@ghost ghost Jun 17, 2021

Choose a reason for hiding this comment

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

What should I do about it? Theoretically they belong in buildInputs, but adding them to buildInputs breaks the build (I have no idea why).

@sumnerevans
Copy link
Contributor

So, after running nixpkgs-review pr 124907 and then running element-desktop from the resulting shell, I now have the option to enable message search, but doing so doesn't appear to work. It just keeps spinning the little loading spinner

image

@ghost
Copy link
Author

ghost commented May 30, 2021

This might fix the build with electron 13: neon-bindings/neon#715

@ghost
Copy link
Author

ghost commented May 30, 2021

Nope, can't find a solution for Electron 13. Let's wait until upstream switches to Electron 13.

@ghost ghost marked this pull request as draft May 30, 2021 06:38
@ghost ghost mentioned this pull request May 30, 2021
@Ma27 Ma27 requested a review from lheckemann May 30, 2021 16:21
@ghost ghost marked this pull request as ready for review June 17, 2021 22:50
@ghost
Copy link
Author

ghost commented Jun 17, 2021

This works fine for me, even when the state has been used with Electron 13.1.x before.

@wucke13
Copy link
Contributor

wucke13 commented Jun 30, 2021

This is not working for me. I get the error in the Security & Privacy/Search/Message Search menu:

Error opening the database: SqlCipherError("Sqlcipher support is missing")

Naturally, I clicked Reset. Now, when I reopen said settings dialog, I have only the option to press Enable und Message Search. Clicking it results in a spinner spinning, that's it. If I attempt to search in chats, I get the warning about the desktop app mentioned here element-hq/element-web#17051. For this I completely reset my $XDG_CONFIG_HOME/Element folder, e.g. I moved the old folder somewhere else and logged in again.

@ghost
Copy link
Author

ghost commented Jun 30, 2021

This solution works for me, I have put it in my local overrides together with source-built schildichat, and I don't know what else I could do to make it work for others.

@ghost ghost closed this Jun 30, 2021
@wucke13
Copy link
Contributor

wucke13 commented Jul 1, 2021

Oh no, why did you close the PR? I'd really like to have this working soon :) To document which exactly goes wrong, here is what I did:

  1. exited all remaining element processes
  2. run nix run 'github:petabyteboy/nixpkgs?ref=feature/seshat2#element-desktop'
  3. In the settings, I see this:
    image
  4. It looks like there is nothing obviously wrong but some language stuff in the output of the terminal:
Output of Element when launched ``` /home/wucke13/.config/Element exists: no /home/wucke13/.config/Riot exists: yes Using legacy user data path: /home/wucke13/.config/Riot No update_base_url is defined: auto update is disabled Fetching translation json for locale: en_EN Changing application language to en-us Fetching translation json for locale: en-us Could not fetch translation json for locale: 'en-us' Error: Cannot find module './i18n/strings/en-us.json' Require stack: - /nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/language-helper.js - /nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/tray.js - /nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/electron-main.js - /nix/store/nf30da68xp29rll1kccj3kbll5z8k3jr-electron-12.0.11/lib/electron/resources/default_app.asar/main.js - at Module._resolveFilename (internal/modules/cjs/loader.js:887:15) at Function.n._resolveFilename (electron/js2c/browser_init.js:261:1128) at Module._load (internal/modules/cjs/loader.js:732:27) at Function.f._load (electron/js2c/asar_bundle.js:5:12684) at Module.require (internal/modules/cjs/loader.js:959:19) at require (internal/modules/cjs/helpers.js:88:18) at AppLocalization.fetchTranslationJson (/nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/language-helper.js:83:20) at /nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/language-helper.js:102:39 at Array.forEach () at AppLocalization.setAppLocale (/nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/language-helper.js:101:17) { code: 'MODULE_NOT_FOUND', requireStack: [ '/nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/language-helper.js', '/nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/tray.js', '/nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/electron-main.js', '/nix/store/nf30da68xp29rll1kccj3kbll5z8k3jr-electron-12.0.11/lib/electron/resources/default_app.asar/main.js', undefined ] } Resetting the UI components after locale change Resetting the UI components after locale change Changing application language to en-us Fetching translation json for locale: en-us Could not fetch translation json for locale: 'en-us' Error: Cannot find module './i18n/strings/en-us.json' Require stack: - /nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/language-helper.js - /nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/tray.js - /nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/electron-main.js - /nix/store/nf30da68xp29rll1kccj3kbll5z8k3jr-electron-12.0.11/lib/electron/resources/default_app.asar/main.js - at Module._resolveFilename (internal/modules/cjs/loader.js:887:15) at Function.n._resolveFilename (electron/js2c/browser_init.js:261:1128) at Module._load (internal/modules/cjs/loader.js:732:27) at Function.f._load (electron/js2c/asar_bundle.js:5:12684) at Module.require (internal/modules/cjs/loader.js:959:19) at require (internal/modules/cjs/helpers.js:88:18) at AppLocalization.fetchTranslationJson (/nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/language-helper.js:83:20) at /nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/language-helper.js:102:39 at Array.forEach () at AppLocalization.setAppLocale (/nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/language-helper.js:101:17) { code: 'MODULE_NOT_FOUND', requireStack: [ '/nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/language-helper.js', '/nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/tray.js', '/nix/store/sznd4g1rqqvclvmvl7c613ha9g4rviq9-element-desktop-1.7.30/share/element/electron/src/electron-main.js', '/nix/store/nf30da68xp29rll1kccj3kbll5z8k3jr-electron-12.0.11/lib/electron/resources/default_app.asar/main.js', undefined ] } Resetting the UI components after locale change ```
  1. When I click through the Reset Dialog, I'll end up with a spinner:
    image
  2. Despite the spinner, the database $XDG_CONFIG_HOME/Riot/EventStore/events.db (which AFAIU holds the data for the search) was created but is not growing in size, to be specific, it's size is 0.
  3. The normal electron process for element has no considerable cpu load, while the renderer process is about 2% load. From this I would infer that no hefty, lengthy, compute heavy decrypting of my chat's is running in background, thus I expect the spinner to be just a UI gimmick?
  4. Now when I start a completely fresh login, so remove the remainings of Riot/Element in $XDG_CONFIG_HOME and login again, I find this in the settings:
    image
  5. If I again click on Reset, I get the spinner again. However, if I now close the settings and reopen the settings, something has changed:
    image
  6. If I now press Enable, I get the spinner again, no significant CPU load. If I reopen the settings menu, the spinner is gone and I can again click on Enable for the search cache, again yielding back my old friend, the spinner. At least no more complaint about SQLite.

So, this is where I'm now. I hope this lengthy comment serves well to document what exactly I did and how the "It does not work" manifest.

@ghost
Copy link
Author

ghost commented Jul 1, 2021

Thanks for the comment. Doing the exact same steps on my machine results in this:

screenshot1
screenshot1

I see no way to debug this if I can't reproduce the issue.
Feel free to open a new PR and figure out why it fails in some cases.

@wucke13
Copy link
Contributor

wucke13 commented Jul 1, 2021

Weird. I can reproduce the same issue, now even with a fresh account, less than 1 week old and less than 1000 messages in total. So this is not an artifact of my account I hope. Again I get the

Message search initialization failed

Advanced
Error opening the database: SqlCipherError("Sqlcipher support is missing")

Error. So I searched a bit. This issue makes me think that there is a high chance that the SQLCipher error is related to the search not working. So I try to investigate that a bit more. I'm quite sure that the error originates from here: https://github.com/matrix-org/seshat/blob/d43e278d65e656a895cec73ec951f044e0de2c7b/seshat-node/native/src/lib.rs#L157. That functions implementation is over here. Now, for SQLCipher support, the following crate is used: https://crates.io/crates/rusqlite . This on then uses https://crates.io/crates/libsqlite3-sys to bind to bind the sqlite C API. So there we are, I believe somehow the sqlite in this build ends up without SQLCipher support SQLCipher is not used/detected by the rusql.

Edit: Oh and this is where the very error string seems to originate from: https://github.com/matrix-org/seshat/blob/7930413986f15225dc7d27848e8b60fea94bb226/src/database/mod.rs#L212 .

@sumnerevans
Copy link
Contributor

In the most recent TWIM update: https://matrix.to/#/!xYvNcQPhnkrdUmYczI:matrix.org/$GGkvPA3b6FuXSquNBq0O9w3ntFnhjcUHsevegeWhc7s?via=matrix.org&via=maunium.net&via=envs.net under "Web"

  • Updated desktop to electron 13, complete with new n-api build of seshat (thanks, Poljar!)

May be worth making another stab at this since I think that Electron 13 compatibility was one of the issues that @petabyteboy was having?

@ghost
Copy link
Author

ghost commented Jul 28, 2021

I implemented the n-api based seshat interface, so I am very aware that it exists, but I'm not interested in spending any more time on this, since my current solution works fine for me and other people.

Also the same things still apply:

I see no way to debug this if I can't reproduce the issue.
Feel free to open a new PR and figure out why it fails in some cases.

@wucke13
Copy link
Contributor

wucke13 commented Aug 6, 2021

The issue I had is resolved by #132601 .

This pull request was closed.
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.

Our element-desktop is not built with encrypted chat search support
5 participants