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.

Sign up for GitHub

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

Jump to bottom

fix(nix/modules/launcher): add wrapGAppsHook #1976

Merged
merged 8 commits into from
Mar 25, 2023
Merged

fix(nix/modules/launcher): add wrapGAppsHook #1976

merged 8 commits into from
Mar 25, 2023

Conversation

steveej
Copy link
Member

@steveej steveej commented Feb 20, 2023 •
edited
Loading

this fixes an issue where the filechooser doesn't work

Summary

can be tried out using nix develop github:holochain/holochain/pr_hc-launch_fix_filechooser#holonix

TODO:

  • CHANGELOG(s) updated with appropriate info
  • make proper use of wrapGAppsHooks
  • @matthme how can we verify the other workarounds (WEBKIT_DISABLE_COMPOSITING_MODE and GIO_MODULE_DIR) are still functionaL?
  • Just before pressing the merge button, ensure new entries to CHANGELOG(s) are still under the UNRELEASED heading

@steveej steveej requested a review from matthme February 20, 2023 20:54
@steveej steveej added the autorebase:opt-in Apply this label to enable automatic rebasing label Feb 21, 2023
@steveej steveej force-pushed the pr_hc-launch_fix_filechooser branch from 722fa74 to 7f42db0 Compare February 21, 2023 18:52
@holochain-release-automation2 holochain-release-automation2 force-pushed the pr_hc-launch_fix_filechooser branch 5 times, most recently from 70d2ea6 to 918038f Compare February 22, 2023 06:21
@steveej steveej changed the title fix(nix/modules/launcher): add wrappGAppsHook fix(nix/modules/launcher): add wrapGAppsHook Feb 22, 2023
@steveej steveej marked this pull request as draft February 22, 2023 09:58
@steveej steveej force-pushed the pr_hc-launch_fix_filechooser branch from 918038f to 053e7d0 Compare March 2, 2023 17:42
@DavHau
Copy link
Contributor

DavHau commented Mar 3, 2023

Tests failed first on a missing socket error. Re-ran them and now it's all green

@steveej
Copy link
Member Author

steveej commented Mar 3, 2023

@DavHau i'm not sure whether or not the wrapGAppsHook clobbers the custom wrap arguments here. none of our tests cover this code as it's functionality inside the wrapped binary that relies on it, and since the executable is a binary it's difficult to verify statically.

@steveej steveej force-pushed the pr_hc-launch_fix_filechooser branch from 053e7d0 to 1375758 Compare March 7, 2023 10:05
@DavHau
Copy link
Contributor

DavHau commented Mar 7, 2023

Wrapping something two times shouldn't be an issue, right? We will just end up with two layers of wrapping which is fine.
So I'd say, we go ahead and merge it.

I don't quite understand what makes it hard to test. I guess the custom wrapper has been introduced to fix some breakage. Can't we just test for that breakage?

@steveej steveej force-pushed the pr_hc-launch_fix_filechooser branch from bc60a69 to b441fa9 Compare March 10, 2023 08:46
@steveej steveej force-pushed the pr_hc-launch_fix_filechooser branch from b441fa9 to 886dacd Compare March 17, 2023 10:15
@steveej steveej force-pushed the pr_hc-launch_fix_filechooser branch from 9059dcb to 53b411a Compare March 24, 2023 10:08
@steveej steveej marked this pull request as ready for review March 24, 2023 14:17
matthme
matthme approved these changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@matthme matthme left a comment

Choose a reason for hiding this comment

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

Approved and tested on my Ubuntu 22.04.

@steveej steveej merged commit 003039a into develop Mar 25, 2023
@steveej steveej deleted the pr_hc-launch_fix_filechooser branch March 25, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavHau DavHau DavHau approved these changes

@matthme matthme matthme approved these changes

Assignees
No one assigned
Labels
autorebase:opt-in Apply this label to enable automatic rebasing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@steveej @DavHau @matthme