Skip to content
This repository has been archived by the owner on Sep 2, 2023. It is now read-only.

Setup MS FontValidator #1

Closed
felipesanches opened this issue May 9, 2017 · 24 comments · Fixed by #55
Closed

Setup MS FontValidator #1

felipesanches opened this issue May 9, 2017 · 24 comments · Fixed by #55

Comments

@felipesanches
Copy link
Member

felipesanches commented May 9, 2017

Originally submitted at fonttools/fontbakery#1297

MS-FontVal is not currently running on the worker container.

@graphicore There's an issue (and there will be a commit closing it pretty soon) here (fonttools/fontbakery#1336) that will provide some clues on how to set up the Microsoft Font Validator on the kubernetes cluster.

@davelab6 davelab6 modified the milestone: 0.1.0 Jul 25, 2017
@felipesanches
Copy link
Member Author

At fonttools/fontbakery#1524 there's a conversation regarding rendering tests performed by MSFontVal. A major dashboard-related issue is the fact that the rendering tests rely on a heavily patched private fork of freetype.

@m4rc1e uses MSFontVal on MacOS and is facing the issue as well. Hin-Tak suggests that using a binary package of his custom (and non-free if I understood it correctly) freetype-build may enable proper execution of the rendering tests on MacOS. This won't help us here as the Google Container Engine relies on GNU+Linux-based containers. So, while it is straightforward to setup MSFontVal to run the vast majority of its tests on a container, we may need some extra-effort to get the dependencies sorted out to also run the rendering tests (which are very important, specially after google/fonts#1137).

@davelab6 davelab6 modified the milestones: 0.1.2, 0.2.0 Aug 28, 2017
@HinTak
Copy link

HinTak commented Aug 28, 2017

@felipesanches
I didn't make any announcement, but I also uploaded FontVal-2.1.1-py-ubuntu-16.04-x64.tgz
https://sourceforge.net/projects/hp-pxl-jetready/files/Microsoft%20Font%20Validator/misc/
Functionally it works as if it is statically linked with its own private copy of heavily patched freetype, although it wasn't technically constructed that way.

Neither of these requires mono anymore, btw.

@m4rc1e : if you could give some feedback on the
FontVal-2.1.1-py-osxbin.tgz that would be good too (the details is in freetype-devel under the "call for mac FontVal testers" thread" in the last day or two). The main one to test is the -clang one. It is also sort of functionally have it own copy of freetype built-in; and it is a native mac os x executable - don't need the rest of mono. If I get confirmation that the -clang one works correctly I'll ship it. The FontVal-32/FontVal-64 are the GUIs which may or may not work without DYLD_*.

@HinTak
Copy link

HinTak commented Aug 28, 2017

I buillt both the ubuntu-16.04 , ubuntu-14.04 binaries in ubuntu docker containers BTW.

@HinTak
Copy link

HinTak commented Aug 28, 2017

You should use the 16.04 binary if you can - because ubuntu 16.04 has a more recent gcc. (a small part of the code uses compiler _built-in's which is only available on gcc 5+ and clang 3.4(?)+). Ubuntu 14.04 has gcc 4.8.x .

@HinTak
Copy link

HinTak commented Aug 28, 2017

@felipesanches : BTW, I would rather describe the full patch set as "not yet suitable for public consumption", instead of "not free". Nobody is spending any time on upstreaming the first patch of 6-ish, that's all there is to it. You are welcomed to improve it to make it upstream-able. When the first patch is upstreamable, then plans may be made on making the rest upstream'able. If the first patch is not suitable for public consumption, the rest is not.

Patch 1 is a somewhat not-yet-upstreamable ugly hack which one can build a very extensive and useful tool on, as long as one accepts that it is not (yet) suitable to be applied to system-wide freetype. Not worrying about breaking system-wide font-rendering is an advantage. You are welcomed to improve it.

Patch 1 (= b06) is 482 lines. I checked that the current is identical except for adjusting for upstream changes. (always useful to check that it does not drift). And it is also about 1/6 of the whole by line counts too.

$ git diff VER-2-8-100-g587264cfd | wc -l
2941

I would prefer you call it 'not yet suitable for public consumption' - and as I said, you are welcomed to improve the first patch to make it more upstream'able, and I'll adopt your work if it proves to be good to replace Werner's. If the first patch is not suitable for public consumption, the rest is not.

@felipesanches
Copy link
Member Author

I still can't understand the nomenclature you use to refer to the patches. You talk about b06, but I don't know where the actual patch is available online. Is "b06" a 3-char prefix of a git commit hash of some repo somewhere?

For instance, you talk about both b66 and b55 in this commit message, but I don't get a clue on where to find the actual patch(es) for that change-set: HinTak/Font-Validator@2b41360

@HinTak
Copy link

HinTak commented Aug 29, 2017

@felipesanches : b06 is simply http://lists.nongnu.org/archive/html/freetype-devel/2016-07/msg00016.html itself. i.e.
lynx --dump http://lists.nongnu.org/archive/html/freetype-devel/2016-07/msg00016.html > patch1-b06.diff . The mailing-list archive display Werner's patch inline after the message, but it arrives at my mail box as an attachment ready to be used. The message itself is patch 1, or b06 as I call it afterwards.

$ cd /tmp
$ lynx --dump http://lists.nongnu.org/archive/html/freetype-devel/2016-07/msg00016.html > patch1-b06.diff
$ git clone git://git.sv.nongnu.org/freetype/freetype2.git
...
$ cd freetype2/
$ git checkout VER-2-6-4
...
$ patch -p1 --dry-run < ../patch1-b06.diff 
checking file src/truetype/ttinterp.c
Hunk #1 succeeded at 90 (offset 8 lines).
Hunk #2 succeeded at 110 (offset 8 lines).
Hunk #3 succeeded at 964 (offset 8 lines).
Hunk #4 succeeded at 1718 (offset 8 lines).
Hunk #5 succeeded at 1741 (offset 8 lines).
Hunk #6 succeeded at 1755 (offset 8 lines).
Hunk #7 succeeded at 4057 (offset 8 lines).
Hunk #8 succeeded at 4240 (offset 8 lines).
Hunk #9 succeeded at 4396 (offset 8 lines).
Hunk #10 succeeded at 4433 (offset 8 lines).
Hunk #11 succeeded at 4784 (offset 8 lines).
Hunk #12 succeeded at 4824 (offset 8 lines).
Hunk #13 succeeded at 4871 (offset 8 lines).
Hunk #14 succeeded at 4953 (offset 8 lines).
Hunk #15 succeeded at 5183 (offset 8 lines).
Hunk #16 succeeded at 5314 (offset 8 lines).
Hunk #17 succeeded at 5362 (offset 8 lines).
Hunk #18 succeeded at 5402 (offset 8 lines).
Hunk #19 succeeded at 5439 (offset 8 lines).
Hunk #20 succeeded at 5528 (offset 8 lines).
Hunk #21 succeeded at 5699 (offset 8 lines).
Hunk #22 succeeded at 5848 (offset 8 lines).
Hunk #23 succeeded at 5904 (offset 8 lines).
Hunk #24 succeeded at 5972 (offset 8 lines).
Hunk #25 succeeded at 6086 (offset 8 lines).
Hunk #26 succeeded at 6243 (offset 8 lines).
Hunk #27 succeeded at 6459 (offset 8 lines).
Hunk #28 succeeded at 6477 (offset 8 lines).
Hunk #29 succeeded at 6540 (offset 8 lines).
Hunk #30 succeeded at 6621 (offset 8 lines).
Hunk #31 succeeded at 6669 (offset 8 lines).
Hunk #32 succeeded at 6690 (offset 8 lines).
Hunk #33 succeeded at 6726 (offset 8 lines).
Hunk #34 succeeded at 6778 (offset 8 lines).
Hunk #35 succeeded at 6820 (offset 8 lines).
Hunk #36 succeeded at 7048 (offset 8 lines).
Hunk #37 succeeded at 7260 (offset 8 lines).
$ 

Hope this is clear.

@felipesanches
Copy link
Member Author

Yes. That's much better. It does not clarify the naming-scheme though. Why did you "nickname" it "b06" ? :-)

Also, what criteria should I apply in order to find the other 5 patches that you said are already made public ? There's not a clear path to locating and getting a copy of them all, which is pretty frustrating to me.

@HinTak
Copy link

HinTak commented Aug 29, 2017

The legacy backend can emit 70-ish type of errors/warnings. The first patch estabilishes the basics/beginnings, plus 6 of those, hence 06. "b" for build/beta/beginning, I guess.

You'd find that b06 itself builds fine but does not do anything. If you do "CFLAGS=-DFT_DIAGNOSTICS ./configure && make " however, you'll find that it doesn't build any more, but is missing a semi-colon. The rest is described in
fonttools/fontbakery#1524 (comment)
.

@felipesanches
Copy link
Member Author

At fonttools/fontbakery#1524 (comment) you mention that 3 of the patches were "made available to others" and that "that's what's in the bin dir of the public repo".

Looking at https://github.com/HinTak/Font-Validator/tree/master/bin and its subdirectories, I only see binaries (mostly DLLs). It is extremely hard to actually find the source code patches!

@HinTak
Copy link

HinTak commented Aug 29, 2017

The other 3 patches, one is pasted inline as in:

diff --git a/builds/exports.mk b/builds/exports.mk
index d5a5085..c95c6e8 100644
--- a/builds/exports.mk
+++ b/builds/exports.mk
@@ -68,6 +68,8 @@ ifneq ($(EXPORTS_LIST),)
 	  $(subst /,$(SEP),$(APINAMES_EXE)) -o$@ $(APINAMES_OPTIONS) $(PUBLIC_HEADERS)
 	  @echo TT_New_Context >> $(EXPORTS_LIST)
 	  @echo TT_RunIns >> $(EXPORTS_LIST)
+	  @echo TT_Diagnostics_Unset >> $(EXPORTS_LIST)
+	  @echo TT_Diagnostics_Set >> $(EXPORTS_LIST)
 
   $(PROJECT_LIBRARY): $(EXPORTS_LIST)
 

and you are aware that CFLAGS=-DFT_DIAGNOSTICS ./configure can also be done as a patch as #define FT_DIAGNOSTICS 1 at the top of a source file? That's the 2nd. The 3rd is also a one-liner, the missing semi-colon.

@felipesanches
Copy link
Member Author

OK, cool! So the first one, b06, is a large one but "does nothing" (meaning it only prepares the codebase for further code patches along the patch-set). The other 2 are minimal fixes to that first one as described above. So, in order to improve the patch-set and try to see if freetype could accept it (or even if it makes sense for freetype to upstream it) I think we'd need to look at the next substantial patches in the queue. Could you please make the rest public so that I can review it ?

@HinTak
Copy link

HinTak commented Aug 29, 2017

If you get b06 (plus the 3 mini-patches outlined above) running, it detects 6 of the 70+ errors/warnings that the legacy backend detects. In order for the rest to be suitable for public consumption, b06 needs to be. At the moment, b06 has a fundamental flaw and makes it not suitable to be applied to a system-wide freetype (but does not affect its usefulness for non-system-rendering freetype use). I seems to be writing the same thing over and over as in fonttools/fontbakery#1524 (comment) . Can you go back to re-read that instead?

@felipesanches
Copy link
Member Author

The puzzle is starting to snap together, indeed. Thanks for your patience in explaining it all. And, yes, I see that you in some cases repeated some portions of the info multiple times, while you omitted other portions (unintentionally, I would hope).

If I keep asking questions, it is not to bug you. It is because the things you said previously did not seem enough to make it all clear.

@felipesanches
Copy link
Member Author

OK. For the record, here are the 6 message-types emmited by the first patch:

felipe@guarana:~/devel/git.sv.nongnu.org/freetype2 (detached*)*$ git grep _rast_ | cut -d\" -f2 | sort | uniq
_rast_E_INSTR_DEFD_BY_FS
_rast_E_NOT_CALLED_FROM_PREPROGRAM
_rast_E_POINT_OUT_OF_RANGE
_rast_E_RP1_RP2_SAME_POS_ON_PROJ
_rast_E_VECTOR_XY_INVALID
_rast_W_PF_VECTORS_AT_OR_NEAR_PERP

@HinTak
Copy link

HinTak commented Aug 29, 2017

@felipesanches : it hasn't occurred to me that you didn't see http://lists.nongnu.org/archive/html/freetype-devel/2016-07/msg00016.html as the patch itself, sorry about that. @davelab6 was CC'ed in on almost all e-mails about FontVal for the past two years . @davelab6 started it in this http://lists.nongnu.org/archive/html/freetype-devel/2015-06/msg00025.html , and this was my first post on it, i think http://lists.nongnu.org/archive/html/freetype-devel/2015-06/msg00038.html . The rest is history, as they say...

@felipesanches
Copy link
Member Author

Thanks, HinTak!

@HinTak
Copy link

HinTak commented Aug 29, 2017

It is interesting to re-read that thread - apparently it started with @anthrotype with http://lists.nongnu.org/archive/html/freetype-devel/2015-06/msg00021.html , with Werner and @davelab6 and Behdad . I only came in to the thread two days later.

@felipesanches
Copy link
Member Author

felipesanches commented Aug 29, 2017

yep, I'm reading that all. Takes some time to digest all the info ;-)

I'll let you know once I'm ready to crunch more code-patches. For now, the first patches and that thread are going to keep me busy for a couple hours, I think. But it all is starting to make sense. I was able to build the patches git checkout and the code makes a lot of sense, indeed.

Can you, perhaps, pinpoint exactly where in that thread someone from the freetype project presented objections to the usage of those global symbols ?

@HinTak
Copy link

HinTak commented Aug 29, 2017

Behdad's http://lists.nongnu.org/archive/html/freetype-devel/2016-07/msg00012.html was one of the first objections .

@HinTak
Copy link

HinTak commented Aug 29, 2017

This post from @davelab6 perhaps explains his role in the whole thing a bit more explicitly.
http://lists.nongnu.org/archive/html/freetype-devel/2015-06/msg00029.html
I didn't particularly want to "maintain" what comes of it :-(. And remember my first post in this thread was still two more days in the future.

@felipesanches
Copy link
Member Author

OK. After a lot of reading, I now feel comfortable with the code and what it is precisely doing. It actually looks much more straightforward then I originally expected it to be. And so I get a much better idea now of the ways we can go forward to get this thing working nicely.

I can see 2 pretty much straightforward possibilities here:

  1. I noticed that you seem to not want to take the extra time of adapting the code for upstreaming into freetype and then having to carry the burden of maintaining the feature after it gets merged. If that's the case, then I can volunteer to do this kind of work myself.

  2. The other possibility is that we can use the patch-set "as-is" (without upstreaming into freetype). As you can see, the baseline idea of this issue is to integrate FontVal into a docker container. The container engine runs on GNU+Linux, which means the OSX or Windows prebuilt packages can't help here. So, in this case, I could build the patched freetype myself and it would be useful even with that nasty global in there. You would not have to take care of maintaining any of that in this case as well.

In both scenarios the only missing piece of the puzzle are the actual patches. Once you make them available I can start working on them (and possibly even improving them) right away. And that won't result in any maintenance effort from you. Quite on the contrary, I will likely end up contributing improvements that can be reincorporated into your work, which is the whole point of free software / open source collaboration.

@HinTak
Copy link

HinTak commented Aug 29, 2017

@felipesanches did you not see my earlier post on
"I didn't make any announcement, but I also uploaded FontVal-2.1.1-py-ubuntu-16.04-x64.tgz
https://sourceforge.net/projects/hp-pxl-jetready/files/Microsoft%20Font%20Validator/misc/.. " - that's right, it is a prebuilt package for Ubuntu 16.04 x86_64 linux. It was built partly inside a Ubuntu 16.04 container. It should run on your container, hopefully if your container is not too ancient.
(there was a prebuilt package for Ubuntu 14.04 too... again please read what I about it... fonttools/fontbakery#1524 (comment) ).

Conceptually it works almost as if it is statically linked to a private freetype (technically it is not done that way ), it also doesn't need any mono bits. It is a standalone x86_64 linux binary.

Maybe I did not make it clear: when b06 is suitable for public consumption (i.e. it does not break freetype's use system-wide), then plans can be made about the rest. Not before that.

@HinTak
Copy link

HinTak commented Aug 31, 2017

See fonttools/fontbakery#1524 (comment) regarding FontVal 2.1.1 and new binaries for ubuntu and mac os x.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants