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: Stop using versioned config dirs #95358

Merged
merged 3 commits into from
Sep 4, 2020

Conversation

jtojnar
Copy link
Member

@jtojnar jtojnar commented Aug 13, 2020

Motivation for this change

Turns out lot of software (including Chromium) use bundled fontconfig so we either need to wrap every one of those, or re-introduce the global unversioned config. The latter is easier but weakens hermetic configs.

But perhaps those are not really worth the effort. Given that programs linked against fc 2.12.6 on fc 2.13.92 system seem to at least display text, even while printing tons of errors (as long as you generate fc cache manually), and same thing the other way around, hopefully it will not be an issue in the future. If we give up on the hermetic configs, we can get rid of lot of complexity (see the diff).

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

@jtojnar
Copy link
Member Author

jtojnar commented Aug 13, 2020

Let’s fix the chromium in master first: #95362

@jtojnar jtojnar marked this pull request as draft August 13, 2020 18:54
@ofborg ofborg 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 Aug 13, 2020
@flokli
Copy link
Contributor

flokli commented Aug 13, 2020

#95362 was merged, so master should be fixed.

@jtojnar
Copy link
Member Author

jtojnar commented Aug 13, 2020

I am completely convinced this is the way to go.

# Don't try to write to /var/cache/fontconfig at install time.
"fc_cachedir=$(TMPDIR)/dummy"
"RUN_FC_CACHE_TEST=false"
"sysconfdir=${placeholder "out"}/etc"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to set sysconfdir for both install and configure?

Copy link
Member Author

@jtojnar jtojnar Aug 16, 2020

Choose a reason for hiding this comment

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

Yes, the configure makes /etc hardcoded into the code but we cannot install to /etc so we need to move the installation path into the derivation. It is quite common idiom.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay 👍

@flokli flokli marked this pull request as ready for review August 16, 2020 13:08
@flokli
Copy link
Contributor

flokli commented Aug 17, 2020

We might need to merge #95716 first (and then rebase this PR on top of master)

@flokli
Copy link
Contributor

flokli commented Aug 26, 2020

@jtojnar can this be rebased?

This reverts commit edf2541.

Turns out lot of software (including Chromium) use bundled fontconfig
so we either need to wrap every one of those, or re-introduce the global
unversioned config. The latter is easier but weakens hermetic configs.

But perhaps those are not really worth the effort. Given that programs
linked against fc 2.12.6 on fc 2.13.92 system seem to at least display text,
even while printing tons of errors (as long as you generate fc cache manually),
and same thing the other way around, hopefully it will not be an issue in the future.

We give up on the hermetic configs in exchange for getting rid of lot of complexity.
The incompatibility does not seem to exist any more: programs linked against fc 2.12
on fc 2.14 system seem to at least display text, even while printing tons of errors
(as long as you generate fc cache manually), and same thing the other way around.
Hopefully it will not be an issue in the future.
@jtojnar
Copy link
Member Author

jtojnar commented Aug 29, 2020

Rebased.

@worldofpeace worldofpeace added this to the 20.09 milestone Aug 31, 2020
@worldofpeace
Copy link
Contributor

@jtojnar I think we should document the fontconfig changes in the release notes eventually.

@worldofpeace worldofpeace mentioned this pull request Aug 31, 2020
10 tasks
@edolstra
Copy link
Member

edolstra commented Aug 31, 2020

Not convinced this is a good idea. What are we going to do when fontconfig changes in a backwards-incompatible way again in the future?

@jtojnar
Copy link
Member Author

jtojnar commented Sep 3, 2020

The versioned config was introduced in 0927405 and c0e2ace (#4410) to resolve #2050, there were also some issues with xml:space b16994f

Looking at the changes in #4410 (comment) it was really just minor relaxation of the schema. The unknown elements would already be skipped with a warning and the patch I sent upstream changes the unknown attributes severity to warning as well (and they were already ignored previously). For that reason backwards compatibility should not be an issue – the configs will continue to be parsed unless they change the format drastically, which is pretty unlikely with the upstream development pace. Forwards compatibility should not be an issue for the same reason.

Outside of NixOS, we would use the unversioned config in either case so nothing changes there.

@jtojnar
Copy link
Member Author

jtojnar commented Sep 3, 2020

The only other reason why we might still want to have versioning is to generate cache for multiple fontconfig versions declaratively but we are not really supporting multiple versions any way. And we could do that in a single config and fontconfig should ignore the cache files generated by other versions.

The use of the argument was removed in 2016 but its definition was left behind.

NixOS@cd2948a
@jtojnar
Copy link
Member Author

jtojnar commented Sep 4, 2020

Merging after IRC discussion with RM.

@jtojnar jtojnar merged commit 4f0f267 into NixOS:staging Sep 4, 2020
@jtojnar jtojnar deleted the global-fontconfig branch September 4, 2020 22:19
@vcunat
Copy link
Member

vcunat commented Sep 5, 2020

I wanted to try some tests on latest staging-next

nix build -f nixos/release-combined.nix nixos.tests.installer.swraid.x86_64-linux

but got blocked by

builder for '/nix/store/HASH1-fontconfig-conf.drv' failed with exit code 1; last 1 log lines:
  ln: failed to create symbolic link '/nix/store/HASH2-fontconfig-conf/etc/fonts/conf.d/50-user.conf': File exists

You'll probably know way faster than me.

--path $out/share/xml/fontconfig \
${./make-fonts-conf.xsl} $out/etc/fonts/fonts.conf \
> fonts.conf.tmp
mv fonts.conf.tmp $out/etc/fonts/fonts.conf

# Make it easier to remove user config in NixOS module.
Copy link
Contributor

Choose a reason for hiding this comment

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

@vcunat this probably should have been kept?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 forgot to revert also 8425726.

jtojnar added a commit that referenced this pull request Sep 6, 2020
This reverts commit 8425726.

This should have been reverted in #95358
but I forgot about it.
@jtojnar
Copy link
Member Author

jtojnar commented Sep 6, 2020

Should be fixed by f0cb5c6.

@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
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: 501+ 10.rebuild-darwin: 1001-2500 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants