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

fontconfig: 2.12.6 -> 2.13.92 #73795

Merged
merged 5 commits into from Jul 13, 2020
Merged

Conversation

worldofpeace
Copy link
Contributor

@worldofpeace worldofpeace commented Nov 20, 2019

Motivation for this change

This currently fails to build at postInstall with

/nix/store/hmqnlh2nvjc4893bxyr0n9i3zyvsh5c3-fontconfig-2.13.92/share/xml/fontconfig/fonts.dtd:127: parser error : xmlParseElementDecl: 'EMPTY', 'ANY' or '(' expected
<!ELEMENT reset-dirs >
                     ^
/nix/store/hmqnlh2nvjc4893bxyr0n9i3zyvsh5c3-fontconfig-2.13.92/share/xml/fontconfig/fonts.dtd:127: parser error : Content error in the external subset
<!ELEMENT reset-dirs >
                     ^
unable to parse /nix/store/hmqnlh2nvjc4893bxyr0n9i3zyvsh5c3-fontconfig-2.13.92/etc/fonts/fonts.conf
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 nix-review --run "nix-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.
Notify maintainers

cc @

Closes: #73725

@worldofpeace
Copy link
Contributor Author

@jtojnar
Copy link
Contributor

jtojnar commented Nov 20, 2019

The DTD looks wrong. The element should have EMPTY as a content specifier.

@worldofpeace worldofpeace marked this pull request as ready for review November 20, 2019 02:35
@worldofpeace
Copy link
Contributor Author

Un-drafted because it isn't broke anymore. Thanks @jtojnar for the quick patch ✨

@flokli
Copy link
Contributor

flokli commented Jul 11, 2020

This should probably be re-evaluated with 2.10 being insecure - I assume we might want to end up using 2.13 as a default, having $out/etc/fonts/conf.d pointing to the 2.13 config, and maybe keeping config working for 2.11+2.12 in $out/etc/fonts/2.11/conf.d, or did I miss something here?

Context: #92919 (comment)

@flokli
Copy link
Contributor

flokli commented Jul 11, 2020

Can we incorporate the removal of 2.10 (#92919) and its config from the module system in here?

@jtojnar
Copy link
Contributor

jtojnar commented Jul 11, 2020

We only use configs from 2.10 so that it should be safe.

We cannot use non-versioned config location since it might break apps from the future and from the past.

I suggest we drop non-versioned config on NixOS, altogether when we decide to stop supporting apps running fontconfig <2.11.

@jtojnar
Copy link
Contributor

jtojnar commented Jul 11, 2020

But I would not do that in this pull request, it is already a big enough change and removing unversioned config dir would make it harder to determine what caused potential issue.

worldofpeace and others added 5 commits July 11, 2020 17:05
Falling back to unversioned `/etc/fonts/conf.d` when versioned one does not exist
is problematic since it only occurs on non-NixOS systems and those are likely
to have a different version of fontconfig. When those versions use incompatible
elements in the config, apps using fontconfig will crash.

Instead, we are now falling back to the in-package `fonts.conf` file that loads
both the versioned global `conf.d` directory and the in-package `conf.d` since using
upstream settings on non-NixOS is preferable to not being able to use apps there.

In fact, we would not even need to link `fonts.conf`, as the in-package `fonts.conf`
will be always used unless someone creates the global one manually (the option is still
retained if one wants to write a custom NixOS module and to avoid unnecessary stat call on NixOS).

Additionally, since the `fonts.conf` will always load `conf.d` from the package, we no longer
need to install them to sytem `/etc` in the module. This needed some mucking with `50-user.conf`
which disables configs in user directories (a good thing IMO, NixOS module will turn it back on)
but otherwise, it is cleaner. The files are still prioritized by their name, regardless of their location.

See NixOS#73795 (comment) for more information.
With previous patch, we no longer load non-versioned fonts.conf file to avoid incompatibilities
but this also means fontconfig will not load system-wide installed fonts on non-NixOS systems.

As a compromise, let's hardcode the FHS font paths to the built-in config so that the system
fonts work there. Unlike with the system config we do not need to worry about compatibility as
incompatible font files will be simply ignored.

Of course there will still be disparity if the system install fonts to some other location than
these two but I am afraid this is the best we can do.

See NixOS#73795 (comment) for discussion.
ITS rules are used for extracting translatable strings and they have
been moved to external files in 2.13.92 so they are not needed in
the config files themselves.

Removing them also cuts down on errors/warnings produced when using
older versions of fontconfig (< 2.12.92). Now it will only complain
about the description element but that is fortunately just a warning,
not errors like the ones caused by the its attributes.

Thanks to this, we can change the config version in NixOS module
back to 2.11 allowing us to re-use the 2.13/2.14 configs for apps
built against 2.12 fontconfig.
@jtojnar
Copy link
Contributor

jtojnar commented Jul 11, 2020

Rebased.

@flokli
Copy link
Contributor

flokli commented Jul 11, 2020

But I would not do that in this pull request, it is already a big enough change and removing unversioned config dir would make it harder to determine what caused potential issue.

Okay. I assume such a change only is in the module system, so we could file the followup PR to master once this has cycled through staging.

@jtojnar jtojnar merged commit 09558f1 into NixOS:staging Jul 13, 2020
Staging automation moved this from Needs review to Done Jul 13, 2020
@jtojnar jtojnar deleted the fontconfig-2.13.92 branch July 13, 2020 01:34
rycee added a commit to nix-community/home-manager that referenced this pull request Jul 31, 2020
This test started failing as described in the code comment. May be
related to Nixpkgs updating Fontconfig from version 2.12 to 2.13. See
NixOS/nixpkgs#73795.
seylerius pushed a commit to seylerius/home-manager that referenced this pull request Aug 3, 2020
This test started failing as described in the code comment. May be
related to Nixpkgs updating Fontconfig from version 2.12 to 2.13. See
NixOS/nixpkgs#73795.
@worldofpeace worldofpeace mentioned this pull request Aug 31, 2020
10 tasks
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/thunderbird-crash-after-upgrade-to-20-09/9915/4

malte-v pushed a commit to malte-v/home-manager that referenced this pull request Feb 24, 2021
This test started failing as described in the code comment. May be
related to Nixpkgs updating Fontconfig from version 2.12 to 2.13. See
NixOS/nixpkgs#73795.
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nixos-builds-a-ton-of-packages-instead-of-fetching-them-in-the-cache/13231/8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet