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

fontconfig_210: remove package #93562

Merged
merged 3 commits into from Aug 12, 2020
Merged

fontconfig_210: remove package #93562

merged 3 commits into from Aug 12, 2020

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Jul 20, 2020

This removes the fontconfig_210 package, as well as the only remaining usage, the rendering of 2.10.x config and cache.

I switched my system to this, and didn't spot any breakages.

Motivation for this change

Closes #88289

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

@flokli
Copy link
Contributor Author

flokli commented Jul 20, 2020

There's a lot more of (now possibly unused) code - there's make-fonts-conf.xsl used to post-massage fontconfigs /etc/fonts/font.conf file to include fontDirectories - and to load files from the versioned subdir.

This is also exposed as makeFontsConf (which seems to be used in various places)

For now I decide to keep this, to not change too much at once, but I assume we might want to remove the configVersion parts in there, too.

jtojnar
jtojnar approved these changes Jul 21, 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.

This looks great but I would like to hold off on merging this until we have dealt with the possible fallback from #73795

@jtojnar
Copy link
Contributor

jtojnar commented Jul 21, 2020

There's a lot more of (now possibly unused) code - there's make-fonts-conf.xsl used to post-massage fontconfigs /etc/fonts/font.conf file to include fontDirectories - and to load files from the versioned subdir.

This is also exposed as makeFontsConf (which seems to be used in various places)

For now I decide to keep this, to not change too much at once, but I assume we might want to remove the configVersion parts in there, too.

Yeah, the XSL stylesheet does two things at once. The reason why it is used in the fontconfig derivation is precisely to introduce the versioned include so we cannot remove the fontconfigConfigVersion variable or fontconfig would no longer load its global configuration.

@flokli flokli mentioned this pull request Jul 21, 2020
Closed
1 task
@flokli
Copy link
Contributor Author

flokli commented Jul 21, 2020

@jtojnar has there been any breakage reported already, or do you mean you just want to await one staging cycle?

@flokli
Copy link
Contributor Author

flokli commented Jul 21, 2020

Yeah, the XSL stylesheet does two things at once. The reason why it is used in the fontconfig derivation is precisely to introduce the versioned include so we cannot remove the fontconfigConfigVersion variable or fontconfig would no longer load its global configuration.

I'd propose removing the fontconfigConfigVersion parts from the xslt stylesheet, if the support fontconfig was the only (remaining?) consumer of it. (But in a followup PR after this one)

@jtojnar
Copy link
Contributor

jtojnar commented Jul 21, 2020

@jtojnar has there been any breakage reported already, or do you mean you just want to await one staging cycle?

Yeah, I would like to wait one staging cycle, or maybe slightly more since the channels lag a bit behind.

I'd propose removing the fontconfigConfigVersion parts from the xslt stylesheet, if the support fontconfig was the only (remaining?) consumer of it. (But in a followup PR after this one)

The current fontconfig needs it too, since it is the mechanism that makes it load the global config.

@flokli
nixos/fontconfig: stop generating fontconfig_210 config and cache
f527651 
This fontconfig version isn't used anywhere inside nixpkgs anymore.
@flokli
fontconfig_210: remove
b761543 
This isn't used anymore anywhere, and vulnerable to CVE-2016-5384.
@flokli
nixos/doc/manual/release-notes: document fontconfig 2.10.x config and…
bab13cc 
… cache removal
@flokli
Copy link
Contributor Author

flokli commented Aug 12, 2020

We had enough some staging rounds since #93562 (review).

Rebased to latest master.

@flokli flokli merged commit 5aca426 into NixOS:master Aug 12, 2020
@flokli flokli deleted the fontconfig_210_remove branch August 12, 2020 11:45
@rtldg rtldg mentioned this pull request Aug 13, 2020
Closed
@flokli
Copy link
Contributor Author

flokli commented Aug 13, 2020 •
edited

Edit: moved over into #95319.

@NickHu
Copy link
Contributor

NickHu commented Aug 17, 2020

NixOS module fonts.fontconfig.penultimate still relies on fontconfig_210 and will trip the assertion. I suppose that the code which generates the fontconfig 2.10 caches in that module can be deleted.

@NickHu NickHu mentioned this pull request Aug 17, 2020
Closed
10 tasks
@flokli flokli mentioned this pull request Aug 17, 2020
Closed
flokli added a commit to flokli/nixpkgs that referenced this pull request Aug 17, 2020
@flokli
nixos/fontconfig: fix 50-user.conf handling
8425726 
Apparently, edf2541 was missed while
rebasing NixOS#93562.

Provide 50-user.conf in fontconfig if includeUserConf is true (the
default), and don't try removing the non-existent one if it's disabled

Fixes NixOS#95685
Fixes NixOS#95712
@flokli flokli mentioned this pull request Aug 17, 2020
Merged
10 tasks
wchresta pushed a commit to wchresta/nixpkgs that referenced this pull request Aug 17, 2020
@flokli @wchresta
nixos/fontconfig: fix 50-user.conf handling
494c887 
Apparently, edf2541 was missed while
rebasing NixOS#93562.

Provide 50-user.conf in fontconfig if includeUserConf is true (the
default), and don't try removing the non-existent one if it's disabled

Fixes NixOS#95685
Fixes NixOS#95712
@andresilva andresilva mentioned this pull request Aug 18, 2020
Closed
jtojnar
jtojnar reviewed Aug 18, 2020
View reviewed changes
nixos/modules/config/fonts/fontconfig.nix
Comment on lines +190 to +195
# update 51-local.conf path to look at local.conf
rm $dst/51-local.conf

substitute ${pkg.out}/etc/fonts/conf.d/51-local.conf \
$dst/51-local.conf \
--replace local.conf /etc/fonts/${pkg.configVersion}/local.conf
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another section that should remain removed from edf2541. See #86601 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I grok this. Can you file a PR to master on what's needed to change?

Copy link
Contributor

Choose a reason for hiding this comment

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

These six lines should have remained removed. I will not get to filing PR before tomorrow.

jtojnar added a commit to jtojnar/nixpkgs that referenced this pull request Aug 20, 2020
@jtojnar
nixos/fontconfig: fix local.conf regression
fe1b9eb 
Another part of edf2541 was missed while
rebasing NixOS#93562, resulting in incorrect path
as described by NixOS#86601 (comment)
@jtojnar jtojnar mentioned this pull request Aug 20, 2020
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar approved these changes

@Valodim Valodim Awaiting requested review from Valodim

@worldofpeace worldofpeace Awaiting requested review from worldofpeace

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: clean-up 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.

Vulnerability roundup 84: fontconfig-2.10.2: 1 advisory
4 participants
@flokli @jtojnar @NickHu @aszlig