Create top-level fonts namespace #18928

chris-martin · 2016-09-24T18:14:21Z

Motivation for this change

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- OS X
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2016-09-24T18:14:23Z

@chris-martin, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @aszlig and @ttuegel to be potential reviewers

rycee · 2016-09-24T21:18:40Z

Awesome! Thanks for working on this!

I think it might be appropriate to maintain aliases to the old attribute names for a release cycle to not break peoples' configuration unexpectedly. I also think this change could use a mention in the release notes.

This is also a good opportunity to clean up the attribute names, for example, instead of fonts.anonymousPro and fonts.dejavu_fonts we could have fonts.anonymous-pro and fonts.dejavu-fonts…

chris-martin · 2016-09-24T23:32:44Z

I think it might be appropriate to maintain aliases to the old attribute names

Agreed, done.

ericsagnes · 2016-09-25T02:31:16Z

Nice!
Could you also update font references in the modules (e.g. fonts.nix)?
It would also be nice to have a line about this change in the release notes.

chris-martin · 2016-09-25T02:42:13Z

@ericsagnes

Could you also update font references in the modules (e.g. fonts.nix)?

I already did - Is there something I missed?

chris-martin · 2016-09-25T02:51:14Z

I'm confused about where to update the release notes - Why hasn't release-notes.xml been touched since 2012?

ericsagnes · 2016-09-25T02:51:40Z

Oops, sorry I missed that changes.
Looks all good then!

groxxda · 2016-09-25T12:41:44Z

@chris-martin I think the release-notes for nixpkgs are unmaintained since the merge of nixos into nixpkgs.
The documentation for this change should probably go into https://github.com/NixOS/nixpkgs/blob/master/nixos/doc/manual/release-notes/rl-1703.xml

aneeshusa · 2016-09-26T00:38:33Z

I think another nice cleanup would be removing font or fonts suffixes, since these are now in a fonts attrset, e.g. fonts.dejavu-fonts to simply fonts.dejavu. (Likely the filenames could be cleaned up as well to match the attribute names.)

chris-martin · 2016-09-26T02:41:06Z

Should we remove "ttf" from font package names too, or is that meaningful?

aneeshusa · 2016-09-26T03:27:15Z

ttf does have some value, as there are other font formats (e.g. see stix-otf). IMO it's not very useful, but others may have other opinions, so I think removing it would be best to save for a separate PR.

edolstra · 2016-09-26T06:24:43Z

nixos/doc/manual/release-notes/rl-1703.xml

@@ -34,6 +34,9 @@ following incompatible changes:</para>
      <literal>gtk</literal>, <literal>gtkmm</literal> and several others.
      Now you need to use versioned attributes, like <literal>gnome3</literal>.
    </para>
+    <para>
+      All font packages have been moved into `nixpkgs.fonts`.


Note that this is not Markdown, so please use something like <literal> instead of backticks.

edolstra · 2016-09-26T06:30:57Z

pkgs/top-level/fonts.nix

+
+  crimson = callPackage ../data/fonts/crimson {};
+
+  dejavu = lowPrio (callPackage ../data/fonts/dejavu-fonts {


Generally the attribute name should match the package name, so this should still be dejavu-fonts. Otherwise we should also rename google-fonts to google etc.

I'm confused by this. Seems like an attribute path of fonts.google and a package name of google-fonts makes a lot of sense?

Not sure if this can be relevant for fonts, but input method engines also have attribute names that differs from package names to avoid redundancy. (The attribute path is enough to remove any ambiguity)

fcitx-engines = recurseIntoAttrs { anthy = callPackage ../tools/inputmethods/fcitx-engines/fcitx-anthy { }; chewing = callPackage ../tools/inputmethods/fcitx-engines/fcitx-chewing { }; hangul = callPackage ../tools/inputmethods/fcitx-engines/fcitx-hangul { };

I'm inclined to agree that allowing different names in this case makes sense. But I would suggest adopting Debian style package name with a fonts- prefix rather than a -fonts suffix for all font packages, so that alphanumeric sorting clumps them together. That is, fonts-google, fonts-dejavu, fonts-font-awesome, etc.

I'm on board with the fonts- prefix for all of the package names. Any objections, @edolstra, @fpletz?

edolstra · 2016-09-27T15:37:02Z

Actually, thinking about this a bit more, I'm not sure if putting fonts in a separate set is a good idea. We've generally not used attribute sets to partition packages, e.g. we don't put all compilers in a compilers attribute set. The reason is that it creates a taxonomy problem. For example, the ghostscript package provides fonts, so should it be included in the fonts set? (In fact, the hierarchical filesystem structure of Nixpkgs has exactly this problem, but that's not directly visible to users.)

The use of sets also makes it harder to guess the name of a package. E.g. if you know about the existence of package foo, you might expect that nix-env -iA nixpkgs.foo installs it, but instead you have to do nix-env -iA nixpkgs.<some prefix>.foo.

chris-martin · 2016-09-27T16:05:56Z

For example, the ghostscript package provides fonts, so should it be included in the fonts set?

Perhaps the fonts set should only contain packages where the name of the package is the name of a font family (in which case a few of these should be moved out). In that case I don't think there would be any room for ambiguity about where something goes. Ghostscript isn't a font, so it doesn't go in the fonts set.

chris-martin · 2016-09-27T16:09:45Z

Are package sets like altcoins something that we should be getting rid of?

rycee · 2016-10-14T18:08:55Z

Hi all, it would be nice to have some closure of this. I would propose choosing one of the following three options:

No change. That is, this PR is closed and we keep the font package and attribute names as-is.

This does not stop, for example, a future change of the attribute name dejavu_fonts to dejavu-fonts but it would be done on a case-by-case basis.
Continue with this PR and move font packages to their own attribute set fonts. To avoid the Ghostscript situation I would suggest that this move only apply to "pure" font packages, that is, the ones present in pkgs/data/fonts. Further this change includes the clean up of attribute and package names as discussed above.

As noted, with this option we have to accept that users must install fonts through an attribute name that may be unintuitive.
Close this PR but perform a font attribute and package name cleanup similar to the one discussed above. That is, fonts are still present in all-packages.nix but their attribute and package names are cleaned up and modernized. For example anonymousPro and dejavu_fonts are renamed to anonymous-pro and dejavu-fonts. Further, we agree that in the case where a name conflict occurs between a font and software package the software package "wins" and the font attribute and package names will begin with fonts-.

Do these seem reasonable? Is there anything I have missed?

chris-martin · 2016-10-14T19:09:22Z

Option 3 seems sensible to me. Clearly the flatness of the nixpkgs object is more revered than I previously thought, but the initial excitement for this tells me that there's still a general desire for cleanup.

chris-martin · 2016-11-29T01:49:01Z

I guess this effort is dead.

chris-martin force-pushed the fonts branch 3 times, most recently from 7e5722f to dd4ab0e Compare September 24, 2016 18:48

chris-martin force-pushed the fonts branch from dd4ab0e to 86cd059 Compare September 24, 2016 23:31

fpletz added the 6.topic: hygiene label Sep 25, 2016

chris-martin force-pushed the fonts branch from 86cd059 to ed1addf Compare September 26, 2016 00:44

chris-martin force-pushed the fonts branch from 664216c to 0cde766 Compare September 26, 2016 03:30

edolstra reviewed Sep 26, 2016

View reviewed changes

chris-martin added 2 commits September 26, 2016 11:43

Create top-level fonts namespace

3ed7992

fonts: clean up package names

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

1614817

chris-martin force-pushed the fonts branch from 0cde766 to 1614817 Compare September 26, 2016 15:43

rycee mentioned this pull request Oct 14, 2016

vertex-theme: 20160329 -> 20161009 #19541

Merged

7 tasks

chris-martin closed this Nov 29, 2016

chris-martin deleted the fonts branch December 31, 2016 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create top-level fonts namespace #18928

Create top-level fonts namespace #18928

chris-martin commented Sep 24, 2016

mention-bot commented Sep 24, 2016

rycee commented Sep 24, 2016

chris-martin commented Sep 24, 2016

ericsagnes commented Sep 25, 2016

chris-martin commented Sep 25, 2016

chris-martin commented Sep 25, 2016

ericsagnes commented Sep 25, 2016

groxxda commented Sep 25, 2016

aneeshusa commented Sep 26, 2016

chris-martin commented Sep 26, 2016

aneeshusa commented Sep 26, 2016

edolstra Sep 26, 2016

edolstra Sep 26, 2016

chris-martin Sep 26, 2016

ericsagnes Sep 26, 2016

rycee Sep 26, 2016

chris-martin Sep 27, 2016

edolstra commented Sep 27, 2016

chris-martin commented Sep 27, 2016

chris-martin commented Sep 27, 2016

rycee commented Oct 14, 2016

chris-martin commented Oct 14, 2016

chris-martin commented Nov 29, 2016


		crimson = callPackage ../data/fonts/crimson {};

		dejavu = lowPrio (callPackage ../data/fonts/dejavu-fonts {

Create top-level fonts namespace #18928

Create top-level fonts namespace #18928

Conversation

chris-martin commented Sep 24, 2016

Motivation for this change

Things done

mention-bot commented Sep 24, 2016

rycee commented Sep 24, 2016

chris-martin commented Sep 24, 2016

ericsagnes commented Sep 25, 2016

chris-martin commented Sep 25, 2016

chris-martin commented Sep 25, 2016

ericsagnes commented Sep 25, 2016

groxxda commented Sep 25, 2016

aneeshusa commented Sep 26, 2016

chris-martin commented Sep 26, 2016

aneeshusa commented Sep 26, 2016

edolstra Sep 26, 2016

Choose a reason for hiding this comment

edolstra Sep 26, 2016

Choose a reason for hiding this comment

chris-martin Sep 26, 2016

Choose a reason for hiding this comment

ericsagnes Sep 26, 2016

Choose a reason for hiding this comment

rycee Sep 26, 2016

Choose a reason for hiding this comment

chris-martin Sep 27, 2016

Choose a reason for hiding this comment

edolstra commented Sep 27, 2016

chris-martin commented Sep 27, 2016

chris-martin commented Sep 27, 2016

rycee commented Oct 14, 2016

chris-martin commented Oct 14, 2016

chris-martin commented Nov 29, 2016