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

appimageTools: use buildFHSUserEnvBubblewrap #97264

Merged
merged 1 commit into from Sep 6, 2020

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented Sep 5, 2020

I tested the execution of all dependent packages and all of them launch to the same screen as before.

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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Result of nixpkgs-review 1

24 packages built:
- Sylk
- deltachat-electron
- devdocs-desktop
- electronplayer
- irccloud
- joplin-desktop
- ledger-live-desktop
- marktext
- minetime
- molotov
- mycrypto
- notable
- obsidian
- plexamp
- runwayml
- ssb-patchwork
- standardnotes
- station
- timeular
- tusk
- unityhub
- wootility
- zettlr
- zulip

I tested the execution of all dependent packages and all of them launch to the same screen as before.
@Atemu Atemu requested a review from Mic92 September 5, 2020 21:45
@Mic92 Mic92 merged commit 86b973e into NixOS:master Sep 6, 2020
@Atemu Atemu deleted the appimageTools-bubblewrap branch September 6, 2020 21:45
@shlevy
Copy link
Member

shlevy commented Oct 6, 2020

@Atemu This broke zulip. Any advice on debugging?

@Atemu
Copy link
Member Author

Atemu commented Oct 6, 2020

Not too familiar with Electron, sorry.

I noticed that AppImage was using Chrootenv and found out that all our packaged AppImage apps work with the new bubblewrap FHSEnv -or so I thought.

I must've missed Zulip somehow. It's the last one alphabetically which might explain it but my bash_history tells me that I did in-fact execute it when drafting this PR.
I tried Zulip from this PR's HEAD and even had it run in a VM of my config to rule out other environmental factors but it must have already been broken back then.

The immediate fix would be to return zulip's appImageTools' FHSEnv builder to Chrootenv. That's fine as we're in the process of transitioning anyways but we should obviously also find out why it doesn't work under Bubblewrap in NixOS (The Flatpak uses the same tech and works fine, that's not it).

I found the -d flag for appimage-run and appRun reads the DEBUG env variable to print its env. They're ENVs are identical though and the debug output isn't very insightful.

What I've also found out is that the app still seems to function: You can open the dev console with C-I and use the "File" "Edit" etc. buttons in the top (e.g. click top left corner and then press q); it just doesn't display anything.
Speaking of which, is your blank screen darkish-grey too or is it white?

The biggest difference between Chrootenv and bubblewrap is how the actual FHSEnv is constructed; the directory structures (especially /etc) are very different.
I have a hunch the issue lies here.

@shlevy
Copy link
Member

shlevy commented Oct 7, 2020

@Atemu For now then #99930

shlevy added a commit that referenced this pull request Oct 7, 2020
For as-yet unknown reasons, this causes zulip to launch with a grey screen.

See #97264 (comment)
shlevy added a commit that referenced this pull request Oct 7, 2020
For as-yet unknown reasons, this causes zulip to launch with a grey screen.

See #97264 (comment)

(cherry picked from commit c800d20)
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.

None yet

3 participants