[WIP] TeX Live patches for Windows #21

mojca · 2019-01-28T06:59:57Z

This is a set of patches as currently used by TeX Live. I'm not the author of them (some of them were contributed by Peter Breitenlohner, some by Akira Kakuto), but I'm submitting them unchanged for now, so that you can make a code review and we can try to find a way to end up patch-free in TeX Live.

(I suspect that some of the patches were ifdef-ed to W32TeX just to minimize the impact elsewhere, but maybe some could be used outside of W32TeX as well.)

The sources need to work:

from msys2 / native MinGW on Windows
cross-compilations with 32-bit and 64-bit mingw-w64 from either Linux or Mac
from Visual Studio (I'm not yet sure how to test that; Akira has some zips with visual studio projects on the w32tex website, I would need to look at those)

I will edit history and commit messages as necessary.

@kberry @norbusan, see: #17, #20

mojca · 2019-01-28T07:22:27Z

Even with those patches in place, the build on msys2 currently fails with:

g++  -g -O2   -o cfftot1.exe cfftot1.o maket1font.o ../libefont/libefont.a ../liblcdf/liblcdf.a
../libefont/libefont.a(otf.o):C:\Programs\MSYS\msys64\home\me\lcdf-typetools\libefont/./../include/efont/otfdata.hh:122: undefined reference to `__imp_ntohs'

kohler · 2019-01-29T22:31:50Z

liblcdf/filename.cc

@@ -126,5 +126,5 @@ Filename::open_write(bool binary) const
    if (_actual || !_path)
 	return _actual;
    else
-	return fopen(_path.c_str(), binary ? "wb" : "w");
+	return fopen(_path.c_str(), "wb");


Is this really necessary?

Kakuto:

Not really necessary in the upstream. I wanted that text files have
same EOL as Unix in TeX Live w32.

I'm not sure how to read Kakuto's comment. Is he saying that we should use binary mode for all files on Windows, or that using binary mode is not necessary?

I'm sorry, I nearly missed this question.

Kakuto:

I said that it is not necessary to use binary mode in the upstream for all files on Windows. It will be better not to change original "w" and "a" in the upstream because LCDF-TYPETOOLS is used also outside the TeX Live.
In the case of TeX Live, sometimes files are shared between various platforms and ls-R files for Windows have Unix style lineendings. Thus I'll change "w" and "a" as "wb" and "ab" in the TeX Live, because I think that it is desirable.

My own comment (please note that I didn't test any of those binary-parameters, and didn't try to read the code from a wider context either, so please excuse my ignorance and ignore my comment in case it's irrelevant): the other patches use unconditional binary mode for files, but I noticed that this is specifically a function which takes an argument for whether or not the file should be binary:

Filename::open_write(bool binary) const

and thus I see this particular patch as the least problematic of them all. In its current state the function is actually ignoring the argument / lying about what it would do.

Seems like you're misreading the patch. In its current state the function is not ignoring the binary argument. The patch would make it ignore the argument.

kohler · 2019-01-29T22:32:14Z

otftotfm/automatic.cc

-# define mkdir(dir, access) mkdir(dir)
+# include <io.h>
+# include <direct.h>
+# define mkdir(dir, access) _mkdir(dir)


I don't object to this

Kakuto:

Thanks a lot.

kohler · 2019-01-29T22:32:34Z

otftotfm/automatic.cc

    look_for_writable_texdir("$TEXMFVAR", true);
+#endif


My question about this part would mainly be: is there a need to have this #ifdef-ed away? Note that I don't know the sources at all, but this looks as if the only reason for checking for W32TEX was because TEXMFVAR is not defined. Wouldn't the same code work everywhere (not just in that particular TeX distribution) where TEXMFVAR is missing? It might take a little more CPU cycles to execute, but it looks generic enough to me. (Maybe the error message below would need to be adapted a bit.)

I agree that it would probably be fine not ifdef'd.

kohler · 2019-01-29T22:34:00Z

otftotfm/automatic.cc

@@ -313,7 +326,7 @@ update_odir(int o, String file, ErrorHandler *errh)
    String ls_r = writable_texdir + "ls-R";
    bool success = false;
    if (access(ls_r.c_str(), R_OK) >= 0) // make sure it already exists
-        if (FILE *f = fopen(ls_r.c_str(), "a")) {
+        if (FILE *f = fopen(ls_r.c_str(), "ab")) {


Why are all files being made binary? Is there an actual concrete reason? Because this is just a text file. But maybe on Windows all kpathsea files such as ls-R should use Unix lineendings.

Kakuto:

In the TeX Live, ls-R files on w32 use Unix style lineendings.

kohler · 2019-01-29T22:34:15Z

otftotfm/automatic.cc

            mktexupd = kpsei_string(kpsei_find_file("mktexupd", KPSEI_FMT_WEB2C));
+#endif


Kakuto:

Thanks a lot.

kohler · 2019-01-29T22:34:21Z

otftotfm/automatic.cc

@@ -677,7 +694,7 @@ update_autofont_map(const String &fontname, String mapline, ErrorHandler *errh)
 #endif
            {
                fclose(f);
-                f = fopen(map_file.c_str(), "w");
+                f = fopen(map_file.c_str(), "wb");


Kakuto:

See above

kohler · 2019-01-29T22:34:51Z

otftotfm/automatic.cc

+            } else // TeXLive
+#endif
+            String option = "--enable Map ";
+            String command = "updmap --nomkmap " + option  + shell_quote(filename) + redirect


This is not right. It won't even compile if W32TEX is defined.

I don't want to go into the contents / correctness (again, I'm not the author and I don't know why the syntax for calling updmap is different), but it looks like a valid / syntactically correct code to me. When W32TEX is defined, the code looks like

if (p != NULL) { // W32TeX ... String option = "--add "; } else String option = "--enable Map ";

which looks valid. It's just that the first time around the code is surrounded by braces, and in the else part it's not (but since it's a single statement, that's ok). I'm not saying that this is a super clear and maintainable code, just that it doesn't look wrong. I'm only slightly confused because I would expect the String option to go out of scope by the time command is composed?

What I would probably do here is something like

bool filetype_is_jtex() // ... but using a more suitable function name, this is just an example { char *p = kpsei_var_value("jtex_filetype"); if (p != NULL) { free(p); // I hate doing this manually, but well ... return true; } else { return false; } } ... String option; if (filetype_is_jtex()) option = "--add "; else option = "--enable Map ";

I also noticed that updmap_prog has been replaced by hardcoded "updmap", and that one is probably not ideal. Maybe it's just an oversight after this line changed in your (= upstream) sources.

Something like that would compile, because in the current code when W32TEX is defined, String option is never in scope in the definition of String command where it is used.

The thing is that obvious compilation errors under W32TEX make me wonder whether anyone needs W32TEX support for this program.

Kakuto:

I removed these wrong changes (r49873).
In W32TEX, updmap-sys[-user] is not executed because of
#if HAVE_KPATHSEA && !WIN32
here I
#define WIN32 1

kohler · 2019-01-29T22:35:17Z

t1dotlessj/t1dotlessj.cc

 #endif
+    if (binary) {


See above...

Kakuto:

See above...

mojca

Thank you very much for the review. I will ask Akira to take a look and provide feedback.

mojca · 2019-01-30T06:01:21Z

otftotfm/automatic.cc

    look_for_writable_texdir("$TEXMFVAR", true);
+#endif


My question about this part would mainly be: is there a need to have this #ifdef-ed away? Note that I don't know the sources at all, but this looks as if the only reason for checking for W32TEX was because TEXMFVAR is not defined. Wouldn't the same code work everywhere (not just in that particular TeX distribution) where TEXMFVAR is missing? It might take a little more CPU cycles to execute, but it looks generic enough to me. (Maybe the error message below would need to be adapted a bit.)

mojca · 2019-01-30T06:07:12Z

otftotfm/automatic.cc

+            } else // TeXLive
+#endif
+            String option = "--enable Map ";
+            String command = "updmap --nomkmap " + option  + shell_quote(filename) + redirect


I don't want to go into the contents / correctness (again, I'm not the author and I don't know why the syntax for calling updmap is different), but it looks like a valid / syntactically correct code to me. When W32TEX is defined, the code looks like

if (p != NULL) { // W32TeX ... String option = "--add "; } else String option = "--enable Map ";

which looks valid. It's just that the first time around the code is surrounded by braces, and in the else part it's not (but since it's a single statement, that's ok). I'm not saying that this is a super clear and maintainable code, just that it doesn't look wrong. I'm only slightly confused because I would expect the String option to go out of scope by the time command is composed?

What I would probably do here is something like

bool filetype_is_jtex() // ... but using a more suitable function name, this is just an example { char *p = kpsei_var_value("jtex_filetype"); if (p != NULL) { free(p); // I hate doing this manually, but well ... return true; } else { return false; } } ... String option; if (filetype_is_jtex()) option = "--add "; else option = "--enable Map ";

I also noticed that updmap_prog has been replaced by hardcoded "updmap", and that one is probably not ideal. Maybe it's just an oversight after this line changed in your (= upstream) sources.

mojca · 2019-01-30T06:25:56Z

otftotfm/kpseinterface.c

+{
+    return kpse_var_value(name);
+}
+#endif


I wonder why this definition is needed. That is: why isn't kpse_var_value itself working? (I now also see why all other code is ifdef-ed by W32TEX, since kpsei_var_value is not defined outside.)

Kakuto:

I wanted to use similar name, kpsei_... as in the upstream, for kpse interface.
kpsei_var_value() is used only in the case of W32TEX.

mojca · 2019-01-30T06:27:24Z

otftotfm/otftotfm.cc

@@ -63,6 +63,9 @@
 #ifdef HAVE_FCNTL_H
 # include <fcntl.h>
 #endif
+#ifdef _MSC_VER
+# include <io.h>
+#endif


I wonder if this is also needed for MinGW. I cannot currently complete the compilation due to some missing libraries in the link phase (see my previous comment).

Kakuto:

forgot why I added these lines.

It turned out that Luigi needed this for MinGW as well (as I suspected), so it made sense to put it under #ifdef WIN32. I probably didn't need it since I disabled kpathsea, and it was probably needed for parts ifdef-ed for kpathsea functionality.

mojca

Replying on behalf of Kakuto-san (copying his responses).

mojca · 2019-01-30T23:21:13Z

liblcdf/filename.cc

@@ -126,5 +126,5 @@ Filename::open_write(bool binary) const
    if (_actual || !_path)
 	return _actual;
    else
-	return fopen(_path.c_str(), binary ? "wb" : "w");
+	return fopen(_path.c_str(), "wb");


Kakuto:

Not really necessary in the upstream. I wanted that text files have
same EOL as Unix in TeX Live w32.

mojca · 2019-01-30T23:21:36Z

otftotfm/automatic.cc

-# define mkdir(dir, access) mkdir(dir)
+# include <io.h>
+# include <direct.h>
+# define mkdir(dir, access) _mkdir(dir)


Kakuto:

Thanks a lot.

mojca · 2019-01-30T23:22:19Z

otftotfm/automatic.cc

@@ -313,7 +326,7 @@ update_odir(int o, String file, ErrorHandler *errh)
    String ls_r = writable_texdir + "ls-R";
    bool success = false;
    if (access(ls_r.c_str(), R_OK) >= 0) // make sure it already exists
-        if (FILE *f = fopen(ls_r.c_str(), "a")) {
+        if (FILE *f = fopen(ls_r.c_str(), "ab")) {


Kakuto:

In the TeX Live, ls-R files on w32 use Unix style lineendings.

mojca · 2019-01-30T23:22:55Z

otftotfm/automatic.cc

            mktexupd = kpsei_string(kpsei_find_file("mktexupd", KPSEI_FMT_WEB2C));
+#endif


Kakuto:

Thanks a lot.

mojca · 2019-01-30T23:23:06Z

otftotfm/automatic.cc

@@ -677,7 +694,7 @@ update_autofont_map(const String &fontname, String mapline, ErrorHandler *errh)
 #endif
            {
                fclose(f);
-                f = fopen(map_file.c_str(), "w");
+                f = fopen(map_file.c_str(), "wb");


Kakuto:

See above

mojca · 2019-01-30T23:26:06Z

otftotfm/automatic.cc

+            } else // TeXLive
+#endif
+            String option = "--enable Map ";
+            String command = "updmap --nomkmap " + option  + shell_quote(filename) + redirect


Kakuto:

I removed these wrong changes (r49873).
In W32TEX, updmap-sys[-user] is not executed because of
#if HAVE_KPATHSEA && !WIN32
here I
#define WIN32 1

mojca · 2019-01-30T23:26:36Z

otftotfm/kpseinterface.c

+{
+    return kpse_var_value(name);
+}
+#endif


Kakuto:

I wanted to use similar name, kpsei_... as in the upstream, for kpse interface.
kpsei_var_value() is used only in the case of W32TEX.

mojca · 2019-01-30T23:27:06Z

otftotfm/otftotfm.cc

@@ -63,6 +63,9 @@
 #ifdef HAVE_FCNTL_H
 # include <fcntl.h>
 #endif
+#ifdef _MSC_VER
+# include <io.h>
+#endif


Kakuto:

forgot why I added these lines.

mojca · 2019-01-30T23:27:18Z

t1dotlessj/t1dotlessj.cc

 #endif
+    if (binary) {


Kakuto:

See above...

mojca · 2019-02-01T09:01:56Z

I removed the remaining #ifdef W32TEX, even though the code that checks for TEXMFLOCAL could probably be improved a bit (I left some TODO comments). Feedback welcome.

After manually fixing #22 the build with MinGW succeeds, earlier I would get undefined reference to __imp_ntohs.

mojca · 2019-02-07T08:32:53Z

@kohler: what do you want me to do next?

Luigi Scarso also just published two more patches that he needed to make the cross-compilation to windows work. I'll add them later (I already suspected that one of them was needed for MinGW as well, not just for MSVC).

mojca · 2019-02-08T21:27:11Z

include/config.h

@@ -7,6 +7,7 @@
 #ifdef WIN32
 # ifdef __MINGW32__
 #  include <winsock2.h>
+#  include <windows.h>


I assume this would not hurt even if defined immediately under WIN32. It's just that under VS it isn't really needed.

mojca · 2019-02-08T21:29:42Z

otftotfm/otftotfm.cc

@@ -63,6 +63,9 @@
 #ifdef HAVE_FCNTL_H
 # include <fcntl.h>
 #endif
+#ifdef _MSC_VER
+# include <io.h>
+#endif


It turned out that Luigi needed this for MinGW as well (as I suspected), so it made sense to put it under #ifdef WIN32. I probably didn't need it since I disabled kpathsea, and it was probably needed for parts ifdef-ed for kpathsea functionality.

mojca

I made the changes I wanted to make, including the ones reported as still missing by Luigi trying to cross-compile for Windows. I'm waiting for further instructions.

If needed, I can split the PR into multiple pieces, totally non-controversial ones, and those still needing some discussion. Please let me know.

kohler · 2019-02-10T18:57:10Z

@mojca Thanks very much for your work. I'd like you to remove the change to filename.cc. After that, I'll merge with a squash commit—it all looks good!

mojca

Thank you very much.

But just to double-check: you are saying that I should remove the "binary" patch from filename.cc and leave other "binary" patches in the code? (I'm a tiny bit confused because I don't see in what way exactly they differ, so that some should stay and some should go. I'm fine with the solution, I just want to make sure that I don't mess things up by accident.)

My view on the subject is the following (again, please remember that my primary platform is not even Windows and that I'm not really using lcdf-typetools either):

TeX Live may combine the files for multiple platforms (Windows + Unix-style), so ls-R and other files have unix file endings there even on Windows. In such environment it probably helps to be able to read both unix-style and windows-style line endings correctly when reading files, and (maybe/optionally?) write unix-style line-endings when generating files.
As Akira said, lcdf-typetools can be used outside of TeX Live, in which case it makes more sense to use windows-style line endings for output files.
It would be ideal to minimize patching in TeX Live.
Maybe there's a way that we come up with a flag (to write LF line endings on Windows instead of CRLF; it could either be a build-time flag or runtime flag, or both) and allow defining the behaviour with that flag; the flag would be off by default, but the TeX Live build could turn it on.

That way the tool could work correctly and as expected for everyone.

mojca · 2019-02-11T21:55:31Z

liblcdf/filename.cc

@@ -126,5 +126,5 @@ Filename::open_write(bool binary) const
    if (_actual || !_path)
 	return _actual;
    else
-	return fopen(_path.c_str(), binary ? "wb" : "w");
+	return fopen(_path.c_str(), "wb");


I'm sorry, I nearly missed this question.

Kakuto:

I said that it is not necessary to use binary mode in the upstream for all files on Windows. It will be better not to change original "w" and "a" in the upstream because LCDF-TYPETOOLS is used also outside the TeX Live.
In the case of TeX Live, sometimes files are shared between various platforms and ls-R files for Windows have Unix style lineendings. Thus I'll change "w" and "a" as "wb" and "ab" in the TeX Live, because I think that it is desirable.

My own comment (please note that I didn't test any of those binary-parameters, and didn't try to read the code from a wider context either, so please excuse my ignorance and ignore my comment in case it's irrelevant): the other patches use unconditional binary mode for files, but I noticed that this is specifically a function which takes an argument for whether or not the file should be binary:

Filename::open_write(bool binary) const

and thus I see this particular patch as the least problematic of them all. In its current state the function is actually ignoring the argument / lying about what it would do.

kohler · 2019-02-12T14:26:31Z

Hi Mojca, I read your comment, but rather than discuss further, please remove the patch from lib/filename.cc as requested, and leave all other patches as is. I think the other patches are fine, the one in lib/filename.cc is not. The one in lib/filename.cc (1) is not used anywhere (2) is to generic code. The other ones are changing the ways specific files are treated, and I believe that those specific files are being treated correctly after the patches.

You may overestimate how often the typetools are used outside texlive.

TeX Live may share the same files between Windows and Unix, so we need to make sure that files and line endings remain compatible.

Cosmetic change, only for consistency with other function names.

Some distributions like W32TEX may not have TEXMFVAR defined, so we also check whether TEXMFLOCAL is writable.

mojca · 2019-02-12T15:51:22Z

I'm sorry, yes, I misread the direction of the problematic patch :)

I fixed the PR. I hope that no squashing will be necessary: I tried to make simple logical commits, but please check.

Once this gets addressed, I would be grateful for some help with #22.

Thank you very much for the swift response and help.

mojca · 2019-02-12T20:10:10Z

Thank you very very much.

kohler reviewed Jan 29, 2019

View reviewed changes

mojca commented Jan 30, 2019

View reviewed changes

mojca mentioned this pull request Feb 1, 2019

tex live patches #20

Closed

mojca commented Feb 8, 2019

View reviewed changes

mojca commented Feb 11, 2019

View reviewed changes

mojca added 5 commits February 12, 2019 16:33

Read and write files in binary mode on Windows

a4e08ae

TeX Live may share the same files between Windows and Unix, so we need to make sure that files and line endings remain compatible.

Add missing headers for Windows

33a4cba

New function kpsei_var_value()

9a2f1db

Cosmetic change, only for consistency with other function names.

Also check if TEXMFLOCAL is writable

b5dd08d

Some distributions like W32TEX may not have TEXMFVAR defined, so we also check whether TEXMFLOCAL is writable.

Use mktexupd.exe on Windows

7cc3b38

mojca force-pushed the TL-patches branch from c031afc to 7cc3b38 Compare February 12, 2019 15:45

kohler merged commit cc78267 into kohler:master Feb 12, 2019

mojca deleted the TL-patches branch February 12, 2019 20:09

		mktexupd = kpsei_string(kpsei_find_file("mktexupd", KPSEI_FMT_WEB2C));
		#endif

[WIP] TeX Live patches for Windows #21

[WIP] TeX Live patches for Windows #21

Conversation

mojca commented Jan 28, 2019 • edited Loading

mojca commented Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojca commented Feb 1, 2019 • edited Loading

mojca commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojca left a comment • edited Loading

Choose a reason for hiding this comment

kohler commented Feb 10, 2019

mojca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohler commented Feb 12, 2019

mojca commented Feb 12, 2019

mojca commented Feb 12, 2019

mojca commented Jan 28, 2019 •

edited

Loading

mojca commented Jan 28, 2019 •

edited

Loading

mojca commented Feb 1, 2019 •

edited

Loading

mojca left a comment •

edited

Loading