-
Notifications
You must be signed in to change notification settings - Fork 499
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
Modify subset-fonts.pe to add floor/ceiling characters #1864
Conversation
This follows advice in asciidoctor#1832 on how to add some missing characters in the M+ fallback font. I think the floor/ceiling characters are the only ones not already present, but my branch explicitly tags all the glyphs the Vulkan spec requires that aren't in the bundled (non-fallback) fonts, for possible future reference. The change to the bundled font is very small - about 220 bytes - and I hope it can be included with asciidoctor-pdf going forward. I had problems building from the repository scripts, as I don't have much of the infrastructure the entire repository expects. In addition there was some sort of issue with a mis-named font in subset-fonts.sh relative to HEAD at the time I did the change. Eventually I cobbled together a shell script which modified subset-fonts.sh and let me build the one font I needed (the M+ fallback) locally using fontforge, without using podman, and have verified that works for us.
This seems very reasonable. I'll have a look at as soon as I get a spare moment. I don't have a problem with adding new glyphs. We just don't want to add the whole font because it takes up space for stuff we don't need and is hard to validate when the font is updated. We just have to figure out where that line is. Your analysis is very helpful for that. |
The whole script should only require podman or docker. So, in theory, it should run on any machine. (It's not something you'll be able to run easily without podman or docker). However, the instructions may be lacking about how to invoke it. I assure you, though, that it is self-contained. |
I'm familiar with docker but not podman, which is less of a thing on Debian AFAICT. If docker happened to just be a drop-in replacement for podman that would be simple to try, but the subset-fonts.sh invocation is hardwired. I poked at GH Actions a bit and it looks like the CI isn't leaving artifacts behind. It might be useful for at least one of the job matrix to preserve the generated font artifacts. BTW the repository LICENSE information is a bit lacking. Since we are taking a snapshot of the generated M+ fallback font for inclusion in the Vulkan repo I had to do some legwork to figure out it was under the Open Font License and incorporate that in our repo's license summary. I get that all that stuff is being downloaded and doesn't directly impact this repo's license, but still helpful to know about. We have moved to using REUSE as part of repo CI checks, so every file must have a license declared. |
Fair point. The script should work with either. I'll update it so that it's possible to specify which container launcher to use. podman the better option because it does not rely on a daemon that runs as root, but I understand it's not yet universally available. (Though it's now available in Ubuntu).
CI does not currently play a role in subsetting the fonts. It's something I do manually from time to time, then commit the output. That's because small changes to the font can have dramatic impact on compatibility and the point is to have consistency from release to release.
I disagree, and I took great care to document all the licenses involved. See https://github.com/asciidoctor/asciidoctor-pdf/tree/master/data/fonts You'll find both ABOUT and LICENSE files for each font / font family we subset and distribute. The NOTICE file at the repository root also addresses it. So you just didn't look hard enough ;)
If there is a place where we need additional notices, please identify those locations. For the record, every file does not need a license declared if it is declared in the repository. It's a common misconception that a license has to be redeclared over and over. There's no legal precedence for doing that to the best of my knowledge. |
This is not correct. The M+ fonts are CC0-1.0. |
While you're of course welcome to your own practice in your own repo, I'm just saying it's helpful for this information to be as up-front as possible. You are right that I missed the NOTICE, although the repository just says "MIT License" at the top level - github's handling of multiple-license repos is too squishy IMO. Khronos is a non-profit corporation for which member and non-member IP is central to what we do. We have already been required to make depositions and give discovery in multiple lawsuit (which, speaking from too much personal experience, is no fun whatsoever for the unfortunate individuals tagged to do it - 8 hours of "Did you write this email / say these words 15 years ago?" "Maybe. It looks like something I might have written, but I can't be sure without looking at my own records."). So we must operate by a very cautious legal standard and be as crystal clear as possible about record-keeping. In conjunction with our IP counsel we feel that all of our files should be explicitly licensed, and REUSE helps in making sure we're doing that, and generating a license manifest for downstream users. It's a good tool. |
That is not what the Google Fonts page says: https://fonts.google.com/specimen/M+PLUS+1p?preview.text=m%20plus&preview.text_type=custom&query=m+plus#license |
And for added license complication, the OSDN project page claims they're under the "mplus Font License" although it's unclear what that is: https://osdn.net/projects/mplus-fonts/ |
Agreed. They are trying to be helpful, but in this case, it's misleading. |
Google is wrong*. The license is clearly stated here: http://mplus-fonts.osdn.jp/about-en.html#license Technically, that's not a license recognized by the OSI (afaik). Fedora has interpreted this license as CC0, which's what is assigned to the RPM package. It's effectively a public domain license. * Technically they are distributing a derivative of a public domain work, so they can relicense their derivative. What is wrong is for them to mislead users into thinking the original font has that license, which it doesn't. |
You do what you have to do. I'm not questioning that. I'm simply stating why I believe it is not necessary for this project to do it that way. If information changes, I'm certainly willing to comply. But the legal advice I have been privy to has asserted that licenses on individual files in a repository is unnecessary. |
I'm going to follow up directly with them. This is too vague and squishy for my comfort level; fortunately we haven't published the branch with the fallback font yet. Relying on third-party lawyers like Fedora is definitely not in my comfort level for something we will be publishing. |
I should point out that we don't use the fonts from Google Fonts (in this project), nor is Google Fonts the authority on (or even owner of) a given font. I would not give statements made by them about fonts any credence in a legal review (unless it's a font Google itself maintains). |
I think it's fair to say that dealing with fonts is extremely complicated and why I'm hesitant to ship any more fonts than we already do. That's not to say we cannot add new glyphs. We can. I'm just making a general comment. |
You would not be relying on Fedora as a lawyer. Their lawyer is Red Hat legal and no package is put into a Fedora release without being validated by Red Hat legal (both because Fedora is represented by Red Hat legal and because those packages ultimately end up in RHEL, which the M+ fonts are). But by all means, do the verification you need to do. It's worth being thorough. |
Just for the record, I want to clarify a few things about the mplus RPM package. It's not listed in the RHEL 8 package manifest. I checked. Rather, it's provided as part of EPEL (extended package repository). In Fedora, the package itself identifies as having the mplus license (it's the metainfo file in the package that says CC0-1.0, but only applies to the package metadata). One thing we've established is that it's not an OSI-recognized license, but the language is certainly that of a public domain mark. It waves all rights ("Unlimited permission is granted"). So that's what we know. Do with that information what you will. |
Thanks. To be clear, the concern isn't that they're not under an acceptable license - any of the four licenses that have variously been attributed to the M+ fonts would do. It's just to determine what the actual license is so we don't make an incorrect assertion. I am getting the help of a native Japanese speaker to communicate with the font project and make sure this is adequately sorted from our POV. I don't know anything about the internals of TTF files - do they potentially have license metadata imbedded already? |
Also, the license question is completely orthogonal to this PR - as far as we're concerned you can accept it whenever you're comfortable with that. |
A very worthwhile endeavor indeed. I certainly want the information here to be as accurate as possible.
Thanks. I certainly appreciate that as it will also benefit all users.
They do indeed have a Copyright field, which you can see using fontforge. But it doesn't tell us anything that we don't already know (and is, in fact, more vague).
Yep, understood. It was just a conversation worth pursuing, so I wanted to let it go without interruption. I'll merge the PR once I have my hacking hat back on this week. |
Something wasn't sitting right with me, so I did a bit more research. It turns out, the mplus license is effectively the same as the Zero Clause BSD license (see https://opensource.org/licenses/0BSD) (not CC0, as I previously claimed). It trims some of the disclaimer and also clarifies that it can be used both commercially and non-commercially. Because of this modification, Fedora and RHEL actually identify it as its own license (mplus). It seems reasonable that the author would adopt an approved OSI license instead of this derivative. Let's see. |
To close out the licensing side topic - one of the Khronos participants who is a native Japanese speaker got in touch with Coji Morishita and discussed the licenses by phone & email. Here's their summary ("he" below refers to the font creator).
Straightforward, though I (@oddhack) am not sure which "github project" is being referred to. Since asciidoctor-pdf is downloading the fonts from OSDN the M+ fonts license carries through, as it does for the modified version we're going to publish until this PR is published in a gem - but Google is not wrong in their license claim either, even though it's a different license. So if you were to switch the source from which the font is pulled to Google or the "github project" in the future, the license would also switch. All makes sense now. |
Thanks for sharing that exchange! That's very informative and definitely clears up a lot of the confusion. |
Several of these are already included in the M+ 1p fallback font:
So the only ones we need to add are 0u2308, 0u2309, 0u230A, and 0u230B. |
Resolved with commit 9432cb0 (which actually updates the font). |
This follows advice in #1832 on how to add some missing characters in the M+ fallback font. I think the floor/ceiling characters are the only ones not already present, but
my branch explicitly tags all the glyphs the Vulkan spec requires that
aren't in the bundled (non-fallback) fonts, for possible future
reference.
The change to the bundled font is very small - about 220 bytes - and I
hope it can be included with asciidoctor-pdf going forward.
I had problems building from the repository scripts, as I don't have
much of the infrastructure the entire repository expects. In addition
there was some sort of issue with a mis-named font in subset-fonts.sh
relative to HEAD at the time I did the change. Eventually I cobbled
together a shell script which modified subset-fonts.sh and let me build
the one font I needed (the M+ fallback) locally using fontforge, without
using podman, and have verified that works for us.
Closes #1832