-
Notifications
You must be signed in to change notification settings - Fork 20
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 from source & enable aarch64 build #55
base: master
Are you sure you want to change the base?
Conversation
Started test build 41986 |
Build 41986 failed |
Is this blocked upstream? |
com.bitwarden.desktop.yaml
Outdated
- --share=ipc | ||
# TODO: add wayland socket when electron supports wayland | ||
# https://github.com/electron/electron/issues/10915 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this issue has been closed and Electron (~)supports Wayland since V12. Since Bitwarden is using V19 currently is it worth adding this now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be tackled in #99
4abc460
to
fe94682
Compare
Started test build 19150 |
Build 19150 failed |
Started test build 19151 |
Build 19151 failed |
95071bc
to
086cea9
Compare
Started test build 19152 |
Build 19152 failed |
Started test build 19157 |
Started test build 19158 |
Build 19157 failed |
Build 19158 failed |
620e6df
to
1a5cd5f
Compare
Started test build 19164 |
Build 19164 failed |
1a5cd5f
to
50c884b
Compare
What's blocking this from being merged? |
The latest aarch64 release (1.7.0) doesn't work for me. I'm able to launch the app, but if I try to login, I just get a 400 with the error description: Since the links above expired, I tried to build and install the branch directly on a device running postmarketOS. Unfortunately, I ran into errors during build, though. 🤔 # Install git and flatpak-builder
sudo apk update
sudo apk add \
flatpak-builder \
git
# Clone the forked repo and switch to the relevent feature branch
git clone https://github.com/proletarius101/com.bitwarden.desktop.git
cd com.bitwarden.desktop
git submodule update --init --recursive
git switch build-from-source
# Install dependencies to build this flatpak
sudo flatpak install \
org.freedesktop.Platform//22.08 \
org.freedesktop.Sdk//22.08 \
org.electronjs.Electron2.BaseApp//22.08 \
org.freedesktop.Sdk.Extension.node16/aarch64/22.08 \
org.freedesktop.Sdk.Extension.rust-stable/aarch64/22.08
# Build it
flatpak-builder build-dir com.bitwarden.desktop.yaml
Any chance we could get a build from CI again so we can test this? |
Building on Asahi Linux on an M1 MacBook Air was successful. However running it afterwards resulted in the following error:
With the following patch I was able to build and run the flatpak.
But I am not really sure if that breaks compatibility on other aarch64 platforms? |
@proletarius101 Can you take care of those merge conflicts? I'd love to see this be merged in. |
I'd like to see it merged as well. But first I need the support of the maintainers of this repository. The only thing left to do after that is to update the version tags. |
Who are the maintainers? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why what this added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the original build script doesn't produce a desktop entry?
i386: | ||
env: | ||
npm_config_arch: ia32 | ||
npm_config_target_arch: ia32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i386: | |
env: | |
npm_config_arch: ia32 | |
npm_config_target_arch: ia32 |
arm: | ||
env: | ||
npm_config_arch: armv7l | ||
npm_config_target_arch: armv7l |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arm: | |
env: | |
npm_config_arch: armv7l | |
npm_config_target_arch: armv7l |
i386: | ||
env: | ||
npm_config_arch: ia32 | ||
npm_config_target_arch: ia32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i386: | |
env: | |
npm_config_arch: ia32 | |
npm_config_target_arch: ia32 |
arm: | ||
env: | ||
npm_config_arch: armv7l | ||
npm_config_target_arch: armv7l |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arm: | |
env: | |
npm_config_arch: armv7l | |
npm_config_target_arch: armv7l |
url-template: https://github.com/bitwarden/clients/releases/download/cli-v$version/bw-linux-$version.zip | ||
tag-template: cli-v$version | ||
|
||
# `node` binary with patches for `pkg`: https://www.npmjs.com/package/pkg#build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't these in the base app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove the whole file. automerge is being phased out for that use case.
@@ -5,10 +5,13 @@ sdk: org.freedesktop.Sdk | |||
base: org.electronjs.Electron2.BaseApp | |||
base-version: '22.08' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
23.08 is current now.
finish-args: | ||
- --share=ipc | ||
- --share=network | ||
- --socket=wayland | ||
- --socket=x11 | ||
- --device=dri | ||
- --env=XDG_CURRENT_DESKTOP=Unity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we are at it, why this?
@hfiguiere Since you've done an extensive review, are you interested in merging this PR if the issues are addressed? |
I tried to help and build this for arm but it fails due to "sdk not specified" but the sdk is specified. |
bot, build |
Queued test build for com.bitwarden.desktop. |
Started test build 119236 |
Don't put the cart before the horse.
|
Build 119236 failed |
finish-args: | ||
- --share=ipc | ||
- --share=network | ||
- --socket=wayland |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think removing wayland support is desirable, especially in this day and age, but maybe I'm misunderstanding what you're removing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it has both x11 ansd wayland which is wrong. So something must be changed anyway.
I built it and it succeeded. the bundle does not start on my arm device though. This is the error: any ideas? �[1m�[47m�[31mA JavaScript error occurred in the main process
|
It's missing @bitwarden/desktop-native-linux-arm64-musl, so it sounds like you're building outside of Flatpak — I assume you're using postmarketOS? Either way, building it outside of this Flatpak package is off-topic and is best discussed elsewhere. |
I am using qemu, flatpak-builder and flatpak build bundle on an x68_64 machine. The target device is a OP6 with pmOS indeed but I fail to understand why this happens and also why it would be bad to troubleshoot it since succeeding would help everyone using arm, no? And if it is indeed not possible to troubleshoot it here, can you point me somewhere? pmOS will likely tell me its a flatpak issue. If you want to take a look at the file: https://files.giftedmc.com/6kaKnS17jzbE.flatpak This is the command I used to build it: |
FYI, took @proletarius101's awesome work, and bringing it up to speed here: #222 |
Fixes #16 #63
The update generated sources action is tested: https://github.com/proletarius101/com.bitwarden.desktop/actions/runs/3997870343/jobs/6859757368
bitwarden-cli
should be handled in another PR to make this PR smaller.