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

nixos/flatpak: pass system icons and fonts #262462

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

io12
Copy link
Contributor

@io12 io12 commented Oct 21, 2023

Fixes #119433

Description of changes

Add another patch to Flatpak that mounts the NixOS system font and icon directories into the sandbox if the standard FHS ones don't exist. The FHS paths are still checked so that the Flatpak build still works on non-NixOS systems. Since these directories contain symlinks into /nix/store, the patch also walks the tree of symlinks to collect a list of /nix/store paths to expose to the sandbox.

Also, change the default for fonts.fontDir.enable from false to services.flatpak.enable.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 21, 2023
@io12 io12 force-pushed the flatpak-fix-fonts-icons branch from a340b18 to 0194ccc Compare October 21, 2023 05:18
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Oct 21, 2023
Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

This will not handle symlinks pointing to trees of symlinks (e.g. when using font collections produced with buildEnv) but I am not sure how common that is.

I also wonder about performance with large font libraries. I thought about using nix-store --query --references but that will not work for the /run/current-system paths directly, since they are not in Nix store. It would also pull in all dependencies, not just those referenced by icons/fonts. But I guess we can just address it by having NixOS generate /run/current-system/sw/share/nix-flatpak/fonts.closure if/when someone notices a performance degradataion.

pkgs/development/libraries/flatpak/fix-fonts-icons.patch Outdated Show resolved Hide resolved
pkgs/development/libraries/flatpak/fix-fonts-icons.patch Outdated Show resolved Hide resolved
pkgs/development/libraries/flatpak/fix-fonts-icons.patch Outdated Show resolved Hide resolved
nixos/modules/config/fonts/fontdir.nix Outdated Show resolved Hide resolved
@io12
Copy link
Contributor Author

io12 commented Nov 12, 2023

Thanks for the review! I applied the suggestions.

This will not handle symlinks pointing to trees of symlinks (e.g. when using font collections produced with buildEnv) but I am not sure how common that is.

I think it does, because get_nix_closure() is recursive. I think I remember this being a requirement for icons.

@andrevmatos
Copy link
Member

I'm running this locally, it seems to work fine, at least for fonts (I can see my system fonts in Inkscape without the bindfs workaround).
cursors though seems to work in some apps, and fallback to a black cursor in Gtk3+ ones, not sure if something with this patch or my local config (KDE, cursors set in Plasma and Gtk configs to Oxygen White)

@io12
Copy link
Contributor Author

io12 commented Nov 13, 2023

I'm running this locally, it seems to work fine, at least for fonts (I can see my system fonts in Inkscape without the bindfs workaround). cursors though seems to work in some apps, and fallback to a black cursor in Gtk3+ ones, not sure if something with this patch or my local config (KDE, cursors set in Plasma and Gtk configs to Oxygen White)

Does the bindfs workaround have the same behavior? If no, what is the output of flatpak --user override --show?

@io12 io12 force-pushed the flatpak-fix-fonts-icons branch from b0fec33 to b333f26 Compare November 19, 2023 00:06
Copy link
Member

@andrevmatos andrevmatos left a comment

Choose a reason for hiding this comment

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

After some restarts (maybe cleaning up the old /usr bindfs mounts), and checking again, this patch seems to be working fine. I can see fonts everywhere, and proper cursors in most apps (but a few stubborn gtk ones, but would not make an issue for them). LGTM

@AndreiSva
Copy link

will this fix the mouse cursor not being the adwaita theme in flatpak apps?

@io12
Copy link
Contributor Author

io12 commented Nov 24, 2023

will this fix the mouse cursor not being the adwaita theme in flatpak apps?

Yes

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3158

@beh-10257
Copy link

since well this is still an open issue is passing themes as well out of the question

@io12
Copy link
Contributor Author

io12 commented Jan 4, 2024

since well this is still an open issue is passing themes as well out of the question

I don't think flatpak tries to pass themes by default, just fonts and icons. I also don't know if nixpkgs would be willing to accept a patch to add a feature that doesn't exist on other distros like that?

@beh-10257
Copy link

This is what I used to do on arch symlink th theme I want to use from /usr/share/.../themes
To .local/share/themes
And yeah that's enough obv after allowing flatpaks to read
.local/share/themes directory

it seems just installing the package isn't enough in nixos or home-manager since I have no idea where are the themes even stored
Also since well I don't mean make it by default but having the option would just be nice

Also so adding icon packs to .local/share/icons manually isn't gonna pass them ??
Even with this pull request

@AndreiSva
Copy link

what is blocking this pr?

Copy link
Contributor

@alois31 alois31 left a comment

Choose a reason for hiding this comment

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

I can confirm this fixes icons and fonts in Flatpak apps on my system. The changes also look sane to the extent I can say that without familiarity with glib code.

@beh-10257
Copy link

So this will get merged now ??

@alois31
Copy link
Contributor

alois31 commented Feb 3, 2024

So this will get merged now ??

No, this has to be done by someone with commit rights. Please be more patient.

Copy link
Contributor

@Henry-Hiles Henry-Hiles left a comment

Choose a reason for hiding this comment

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

LGTM

@Henry-Hiles Henry-Hiles added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Feb 27, 2024
LostAttractor added a commit to LostAttractor/flake that referenced this pull request Feb 28, 2024
LostAttractor added a commit to LostAttractor/flake that referenced this pull request Feb 28, 2024
@andrevmatos
Copy link
Member

@SuperSandro2000 could you please give a look at this PR? I've been running it for months with great success.

LostAttractor added a commit to LostAttractor/flake that referenced this pull request Mar 6, 2024
LostAttractor added a commit to LostAttractor/flake that referenced this pull request Mar 6, 2024
@wegank wegank added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Mar 8, 2024
Copy link
Contributor

@Ramblurr Ramblurr left a comment

Choose a reason for hiding this comment

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

Works for me too!

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

LGTM but I have no clue about the C code :/

@beh-10257
Copy link

@andrevmatos can you post just the bit when you overrided the module with this pr
since its been 3 months and there is no guarantee that this will be merged anytime soon I wanna check it out

@andrevmatos
Copy link
Member

@beh-10257 my system is a flake using https://github.com/gytis-ivaskevicius/flake-utils-plus, and I use the channels.<name>.patches functionality of it to apply arbitrary PRs to my system's nixpkgs (tracking nixos-unstable-small).

@SuperSandro2000
Copy link
Member

@ofborg build flatpak

@SuperSandro2000 SuperSandro2000 merged commit dcbfb2b into NixOS:master Mar 25, 2024
5 of 7 checks passed
@SamLukeYes SamLukeYes mentioned this pull request Mar 31, 2024
13 tasks
@RAVENz46
Copy link
Member

RAVENz46 commented Apr 10, 2024

Fonts fine but icons are still missing.
Is this because I installed icons via home-manager?

Edit: Install icon system-wide solve issue.

LostAttractor added a commit to LostAttractor/flake that referenced this pull request Apr 16, 2024
@SadSock
Copy link

SadSock commented Apr 20, 2024

How to apply this patch in nixos 23.11?

4JX added a commit to 4JX/nixos-config that referenced this pull request May 13, 2024
LostAttractor added a commit to LostAttractor/flake that referenced this pull request Jul 20, 2024
LostAttractor added a commit to LostAttractor/flake that referenced this pull request Jul 26, 2024
Jack5079 added a commit to Jack5079/dotfiles that referenced this pull request Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 11-100 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flatpak can't access system fonts and icons