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

Build statically-linked sqlcipher for Unix #334

Merged
merged 5 commits into from
Apr 19, 2022

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Mar 29, 2022

This PR is a direct application of this patch by the Arch Linux community to help fix incompatibilities between sqlcipher and electron 15+. Full context is in #691.

I've just copy/pasted the patch to this PR for easier review, but from what I can see, this changeset is:

  • Compiling sqlcipher.so on Linux, whereas before we relied on it being provided by a separate distro package.
  • Setting the --enable-tcl=no configure flag when compiling sqlcipher for Mac and Unix.
  • Setting the --with-pic=yes configure flag for Linux only.
  • Setting RUSTFLAGS=-Clink-arg=-Wl,-Bsymbolic -Clink-arg=-Wl,--exclude-libs,ALL for Linux only.
  • Running tclsh on all platforms (previously Linux was excluded).

The implication is that Linux builds will now receive and use a statically-linked version of sqlite instead of relying on distro packages. It may be worth checking with downstream packagers whether this change is OK.

It should also be ensured that the Element Desktop build machines have the appropriate packages installed to handle this new configuration (tclsh is needed now, for instance).


This PR currently has none of the required changelog labels.

Add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is plus X-Breaking-Change if it's a breaking change.

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

I'm not super familiar with a lot of this code, but at a glance it appears it won't work on Windows - can we get some assurances that it appears to build fine for the supported environments?

hak/matrix-seshat/build.ts Show resolved Hide resolved
hak/matrix-seshat/check.ts Show resolved Hide resolved
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Looks fine, but this isn't behind an option or anything so this will make our debian build ship with a statically linked sqlite. If we want to do this, we should remove it as a dependency from the debian package.

hak/matrix-seshat/build.ts Show resolved Hide resolved
hak/matrix-seshat/check.ts Show resolved Hide resolved
@t3chguy
Copy link
Member

t3chguy commented Apr 12, 2022

Looks fine, but this isn't behind an option or anything so this will make our debian build ship with a statically linked sqlite. If we want to do this, we should remove it as a dependency from the debian package.

We'd also need to communicate to all other re-packagers to not depend on sqlcipher

@anoadragon453
Copy link
Member Author

I've now removed libsqlcipher0 as a dependency from the nightly and release Debian packages.

@anoadragon453 anoadragon453 requested review from dbkr and turt2live April 13, 2022 00:02
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

I defer to Dave on this one

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Seems fine from my PoV then. We should check with whoever has the job of releasing currently.

@t3chguy
Copy link
Member

t3chguy commented Apr 19, 2022

Landing this, it won't be in a release for nearly 3 weeks, have notified known packagers

@HarHarLinks
Copy link

I just updated the first time to a nightly that I think has this patch: Element Nightly version: 2022042401 Olm version: 3.2.8 from aur/element-desktop-nightly-bin.

I launched using the desktop file, which does LD_PRELOAD=/usr/lib/libsqlcipher.so exec "/opt/Element-Nightly/element-desktop-nightly". It did not crash instantly, which is good, but upon checking the settings for search indexing, I found:

Error opening the database: DatabaseUnlockError("Invalid passphrase")

I reset the event store, it started indexing, and crashed after ~1 minute.

@anoadragon453
Copy link
Member Author

anoadragon453 commented Apr 25, 2022

@HarHarLinks Try removing the LD_PRELOAD= env var. That will force Element Desktop to use the system libsqlcipher again, bypassing the statically-linked one added in this PR, thus causing crashes.

Note that as of about 4 hours ago, this env var has been removed in the element-desktop-nightly-bin AUR package: https://aur.archlinux.org/cgit/aur.git/commit/?h=element-desktop-nightly-bin&id=f2d409a3fee6bdb63ef5fedf122b0e34de78ca96

So you should just be able to update and have it work :)

Edit: And now I realise you just noticed that on the linked issue. Ah well :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants