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

nixos/fontconfig: disable by default #98629

Closed
wants to merge 1 commit into from
Closed

nixos/fontconfig: disable by default #98629

wants to merge 1 commit into from

Conversation

lheckemann
Copy link
Member

Motivation for this change

This can reduce the closure size of headless systems.

Things done
  • 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.

@ofborg test xfce xrdp fontconfig-default-fonts cage chromium

@lheckemann
nixos/fontconfig: disable by default
12075ce 
This can reduce the closure size of headless systems.
@worldofpeace
Copy link
Contributor

It seems tests in the PR body stopped working at some point
@ofborg test xfce xrdp fontconfig-default-fonts cage chromium

worldofpeace
worldofpeace reviewed Sep 24, 2020
View reviewed changes
nixos/tests/fontconfig-default-fonts.nix
Comment on lines 12 to 13
fonts.enableDefaultFonts = true; # Background fonts
fonts.fonts = with pkgs; [
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what happens when you use fonts.fonts or enableDefaultFonts but maybe fonts.fontconfig.enable isn't true? Would it be worth making sure that doesn't happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so; most modules we currently have will ignore any config if enable is set to false. That said, fonts.fontconfig.enable isn't a sibling of fonts.enableDefaultFonts, so maybe we should indeed have enableDefaultFonts imply enable (either automatically, or by throwing an error, or not even forcing it, just producing a warning). Likewise for fonts.fonts being nonempty.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lheckemann Yeah, I think throwing an error would be okay.

@rycee
Copy link
Member

rycee commented Sep 24, 2020

It's interesting that profiles/minimal.nix contains environment.noXlibs = mkDefault true; but profiles/headless.nix does not. Perhaps this PR should instead be the introduction of that option assignment to headless.nix?

jtojnar
jtojnar reviewed Sep 24, 2020
View reviewed changes
Copy link
Contributor

@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.

It is hard to tell how common are servers that do not do image processing and if libraries like imagemagick can work without fontconfig when the image manipulation does not use texts (e.g. generating thumbnails for a gallery, cropping avatars in web app).

@lheckemann
Copy link
Member Author

if libraries like imagemagick can work without fontconfig when the image manipulation does not use texts

Yes, they do:

$ unshare -rm
# /run/current-system/sw/bin/mount --bind /var/empty /etc
# nix run nixpkgs.imagemagick
# convert -background white -fill black logo: /scratch/test1.png
# convert -background white -fill black label:test /scratch/test2.png
Fontconfig error: Cannot load default config file
convert: unable to read font `helvetica' @ error/annotate.c/RenderFreetype/1367.
convert: no images defined `/scratch/test2.png' @ error/convert.c/ConvertImageCommand/3226.

test1.png is created and looks fine.

It's interesting that profiles/minimal.nix contains environment.noXlibs = mkDefault true; but profiles/headless.nix does not. Perhaps this PR should instead be the introduction of that option assignment to headless.nix?

headless.nix is very weird anyway. My interpretation of "headless" is a machine that doesn't have a display attached; but this module disables the serial console, even though a serial console is particularly useful on such machines!

I suppose this is a matter of policy in the end: should a NixOS with a blank config be featureful or minimal by default? I think I lean towards minimalness, but of course that isn't up to me alone to decide :)

@lheckemann lheckemann closed this Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

@rycee rycee rycee left review comments

@worldofpeace worldofpeace worldofpeace left review comments

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lheckemann @worldofpeace @rycee @jtojnar @ryantm