fontconfig: fix etc priority #16730
fontconfig: fix etc priority #16730
Conversation
|
@ericsagnes, thanks for your PR! By analyzing the annotation information on this pull request, we identified @ttuegel, @abbradar and @lethalman to be potential reviewers |
vcunat
added
the
1.severity: mass-rebuild
|
We've maintained support for fontconfig < 2.11 for about year and a half #4410; maybe we could drop it already. |
|
For whom will loosing fontconfig < 2.11 be a problem? Non-NixOS users on old systems? (If so, how old?) |
|
I think Ubuntu 12.04 LTS has old fontconfig and is still supported upstream. Also SteamOS derives from it, and so Steam uses this old version (2.8.0) too. |
|
Old systems shouldn't be broken, as my old comments #2050 claim that newer fontconfig library doesn't have a problem with old |
|
Indeed and indeed. BTW, Hydra has not yet started evaluating several staging merges. They all are quite focused in what they really impact, so I propose batching this patch, too. I'll test it in a VM and if it's okay -- merge. |
|
I'm currently testing four stdenv-rebuilding changes on top of staging, so I certainly count on those in this iteration. I haven't yet got to properly reviewing this PR. |
|
I have reviewed it some time ago, but it's difficult to catch all the changes because there's lots of moving lines around in play. I think it looks okay (there is an unreferenced derivation for testing left, but it can be fixup'ed). Testing in VM showed that all |
| "ln -s ${userConf} $out/etc/fonts/${version}/conf.d/99-user.conf"} | ||
|
|
||
| ''; | ||
| confPkg2 = pkgs.runCommand "test" {} '' |
There was a problem hiding this comment.
This is leftover from testing IIUC.
There was a problem hiding this comment.
Nice catch! Thanks for noticing, I updated the PR.
|
Closing as this has been merged in 1e53d4a. |
ericsagnes
changed the title
|
Reopening as this was reverted by cfc0a54. Current state issues:
|
ericsagnes
force-pushed
the
feat/etc-font
branch
4 times, most recently
from
d6f8ef9 to
3d99fbc
Compare
|
OK, the PR is becoming a real mess. So this was based on staging, and it was merged but for some reason the PR is still open. So it is applied on staging, applied and reverted on master, and I wrongly rebased it on master so there are hundred something commits that don't belong here. But as the same time as it is not reverted on staging it generates conflicts if I try to apply it on the latest staging. What can be done from here? revert 1e53d4a on this PR and adding the new changes on top of that? |
|
@ericsagnes I've merged master to staging -- if you rebase you can then revert the revert and make your fixes. FWIW I've tried to fix #16983 yesterday but no luck -- when I add |
ericsagnes
force-pushed
the
feat/etc-font
branch
from
3d99fbc to
aee3145
Compare
|
@abbradar Thank you! But |
ericsagnes
force-pushed
the
feat/etc-font
branch
2 times, most recently
from
b703053 to
00ce85a
Compare
|
@ericsagnes That's what I meant by "weird evaluation errors". Interesting thing is that I managed to get several different seemingly unrelated errors. But I don't remember how ;_; |
| fonts.fontconfig.confPkgs = [ confPkg ]; | ||
|
|
||
| # Proposition A: use environment.etc | ||
| # TO FIX: this this create /etc/fonts.tmp if /etc/fonts is already present |
There was a problem hiding this comment.
Created a fix at #17042. You might be interested.
There was a problem hiding this comment.
Great! thanks, I will try that.
There was a problem hiding this comment.
Works perfectly, thank you!
ericsagnes
force-pushed
the
feat/etc-font
branch
from
901de7e to
da64fed
Compare
|
As #17042 was merged, this PR is ready to be merged. The core functionality of the module is not changed, just a few improvements were made:
The installer problem should be fixed as The PR was tested it with multiple font setups on a VM and a real machine. |
ericsagnes
changed the title
|
|
||
| # user settings configuration file | ||
| # priority 99 | ||
| userConf = pkgs.writeText "fc-99-user.conf" '' |
There was a problem hiding this comment.
In the default way fontconfig is set up, this has priority 50 (see the README in conf.d).
But 50-user.conf is removed in line 69 of fontconfig/default.nix (See also this comment). As you can see here, the default 50-user.conf is more or less identical to the user configuration file here.
Another thing I wonder: don't the system-wide configurations have precedence over the user-level configurations, since you have 99 as priority? The default fontconfig setup has the priorities the other way around: 50 for user configurations, and 51 for the system-wide configuration in local.conf.
ericsagnes
force-pushed
the
feat/etc-font
branch
from
da64fed to
26a51f2
Compare
ericsagnes
force-pushed
the
feat/etc-font
branch
from
26a51f2 to
2483353
Compare
|
Added 2 commits. fontconfig module: respect upstream definitionsThis keep upstream
The module default fonts configuration priority as been moved to 52, with a lower priority than upstream user (50) and local (51) configuration files. fontconfig-ultimate: fix wqy-zenhei-sharp priorityA file from fontconfig-ultimate font patch set, wqy-zenhei-sharp, has wrong priority (43) and set default fonts with a higher priority than any "alternate config file loading" files making This minor patch fix the file priority so |
|
Yes, this already looks much better to me :). |
| # local.conf (indirect priority 51) | ||
| ${optionalString (cfg.localConf != "") '' | ||
| ln -s ${localConf} $support_folder/local.conf | ||
| ln -s ${localConf} $latest_folder/local.conf |
There was a problem hiding this comment.
I wonder whether this gets included anywhere by another config file, since latest_folder is the versioned directory (e.g. /etc/fonts/2.11)?
There was a problem hiding this comment.
Nice catch!
Indeed the latest local.conf wasn't used.
Even if both local.conf have the same contents, I amended the commit so latest 51-local.conf to look for latest local.conf. (da342b0#diff-376672cba12c87d4103e6134632e306fR164)
It feels cleaner and it will make things easier if in the future we want to have a different local.conf for each version.
There was a problem hiding this comment.
It feels cleaner and it will make things easier if in the future we want to have a different
local.conffor each version.
Weren't the versioned configurations only put in place for backwards compatibility?
There was a problem hiding this comment.
Weren't the versioned configurations only put in place for backwards compatibility?
Right, I must confess that I don't know much about the reasons and implications for multiple fontconfig version support so I might have made silly assumptions.
I just thought that there might be a need to have different local.conf for each fontconfig version in case there were incompatibilities.
But the main reason is that, as a personal taste, I think it easier to understand and cleaner to have symmetric file structure for both versions.
But I have no problem in removing latest/local.conf in favor of a single /etc/fonts/local.conf if it is more appropriate.
There was a problem hiding this comment.
Nah it's fine, that makes sense.
Right, I must confess that I don't know much about the reasons and implications for multiple fontconfig version support so I might have made silly assumptions.
I don't know either :P.
ericsagnes
force-pushed
the
feat/etc-font
branch
from
2483353 to
1723c01
Compare
|
I made a branch for combined testing of new font-related updates (#16730, #17770, #17846). I believe this is ready for merging, so after #17846 is also ready I'll test them all, keep for a few more days in case anyone has objections and merge to staging. That said, I may have skipped some unresolved problem arised in this thread -- are there any? |
There shouldn't be anymore problems. The only thing that I couldn't check is the In theory, the |
|
Thanks for merging! |
You are welcome, thanks for your contributions ^_^Nikolay. Sent from a mobile device, please excuse my brevity. |
Motivation for this change
Address #16026
Current state issues:
When /etc/fonts is already present, the newfixed by etc: remove obsolete directories #17042/etc/fontswill be created as/etc/fonts.tmpbreaking fontconfig configuration. Issue Font looks different + really bad kerning #16978usingfixed by usingsymlinkJoinfor creating the/etc/fontpackage maketest.installers.*fails becauselndiris not available. Issue Tests fail due to failure to download curl source #16983buildEnvinstead ofsymlinkJoinThis make all the fontconfig related configuration files be symlinked in the correct order in
etcso fontconfig settings andfontconfig.defaultFontsare applied correctly.This will trigger a rebuild of fontconfig and all related packages, so the PR is made on
staging.The change in fontconfig is required in order to have a correct file loading order for
${pkgs.fontconfig.out}/etc/fonts/conf.d/*. (that are currently linked in/etc/fonts/{,2.11}/fonts.conf).example:
Notes
/etc/fontsis already present, the new folder will be created as/etc/fonts.tmpunless/etc/fontsis manually deleted. I suppose it is because current/etc/fontsis a real folder and this PR/etc/fontsis a symlink./etc/fontsand/etc/fonts/2.11difference, I tried to keep it as close as it was before the PR, but I would appreciate if someone with better fontconfig knowledge could verify everything work as intended.fontconfig.defaultFontsis affected by the system locale (verified with asian locales and japanese fonts), as far as I can tell it seems to be a feature of fontconfig to raise locale matching fonts at a higher priority than what they are declared.Tests, feedback and improvements are welcome.
cc @vcunat @ttuegel
Things done
(nix.useChroot on NixOS,
or option
build-use-chrootinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)