Spot gpos fixes #380
Spot gpos fixes #380
Conversation
The AFM dumping logic requires creating a temporary file. With the 64-bit update, I also made a change to replace the deprecated tmpnam() function. The replacement code created a file name of length 0 for the temporary file, so the file open failed. Note that I have now replaced the tmpnam() function with tmpfile(). This used to fail under Windows Vista through Windows 8, unless the user had admin privileges. Compiling with with VS 2017 allows this to run under Windows 7 and Windows 10. I now need to update all the Windows build files to use VS 2017 to avoid this problem with Windows users. 'spot' built with VC6 will have this problem under all platforms. We need to update to the new VS 2017 anyway, to support 64 bit tools under Windows, so I'd rather to do this than add complexity to the code to support older compilers
With the 64-bit update, the function global.c::getGlyphName() can return a glyph name of up to 128+7 characters long. The returned string was, in many places, copied into a string buffer that was much shorter.
|
Something is going on with Codacy. |
|
@readroberts Doesn't Python 2.7 use VS 2010 to compile C code on Windows (i.e. when you build everything through setup.py)? |
|
@msousa @cjchapman. Yes, Travis is using VS 2010 to compile C code on Windows. I wouldn't accept this PR as is until I update the Windows builds to VS 2017. The alternative is to write some simple code for Windows only ( development time about 15 minutes - would just create a temp file in the current directory, without attempting to deal more than one version of 'spot' running) and then take it out when we get around to updating all the Windows projects. I favor the latter. |
|
@readroberts how's updating the project files to VS 2017 going to make a difference given that (AFAIK) VisualStudio hardwires the version of Python it calls? Our Python scripts are not ready to go beyond Python 2.7 which (AFAIK) means we're stuck on VS 2010. |
|
@msousa @cchapman Not on our system: we don't use Cython to build extensions. In the AFDKO setup.py, I override setuptools.command.build_py.build_py module with a custom class 'CustomBuild()'. The run() method in this class calls shell scripts that compile the code. The Windows 'BuildAll.cmd' command file references the build system for specific version of Visual Studio. I don't know how Travis works, but I think I recall Chis telling me that it can be configured to run on whatever Windows configuration you want. |
|
@readroberts When I did |
|
you need vs2010 simply because that's the version of the Visual Studio solution files (.sln) currently used to compile the afdko programs. There is no required link between the version of python and the version on visual studio in your case, because you are not building an extension module that has to dynamically link with the python interpreter (and thus would require you to use the same VS used to compile python interpreter itself, to avoid incompatibilities between different C runtimes). You are building standalone executables (.exe) which don't require python to run. You simply call them as external subprocesses. |
|
strictly speaking, if you were to build an extension module for python 2.7 on windows, you would need visual studio 2008, which is ancient. MS provides a package called "Visual C++ for Python 2.7" that includes only the commandline tools from vs2008 and can be used to compile python extension module for 2.7. For compreffor, that was |
|
(typo -- of course I meant, "that was too old"...) |
you meant Appveyor. All the available visual studio versions should already be installed there, see |
|
@anthrotype Thanks for all of the information! |
|
I believe the next step is for @readroberts to update spot's Visual Studio files. |
…y of several types of VS 2017. The Appveyor batch process failed with the more complex command file syntax using vswhere.exe to locate the msbuild.exe file. Try a very simple approach.
Try updating sln file to 23017 format, which we should do anywhere.
Try asking appveyor for VS 2017 image. I suspect that this will make all the 2010 builds fail
|
@miguelsousa @cjchapman After some experimentation, I see that we cannot build a mix of VS 2010 and VS 2017 Windows projects. The default image used by Appveyor is the VS 2015 image, which has the VS 2010 .Net build tools that is used by our Windows projects. However, the VS 2017 image does not have this. If I want to update any of our windows projects to VS 2017, I need to update all of them. This is pretty easy, and I expect that I could do it in one day. Going ahead with this is my preference, but I could also revert to plan B: for the moment, continue to use VS 2010, but use a simple and reasonably reliable method for generating a temp file. With plan B, we defer updating the Windows projects until later. |
What do you have in mind? |
Updating to VS 2017 would be my preference too, but your time estimates are always over-optimistic, so it's difficult for me to weigh what the best option is. |
|
The simple approach to generating a temp file is to create a temp file with hard-coded name like "spot.tmp' in the current directory, without attempting to deal more than one version of 'spot' running, and then reverting this when we have updated to VS 2017. The work involved in updating to to VS 2017 is to 1) open each of 12 Visual Studio solution files in VS 2017, and compile it, then 2) edit each of the 12 BuildAll.cmd files to reference the VS 2017 tools. |
|
The temp file hack doesn't seem like a good option to me; I don't want us to end up with another issue like #244. Here's my suggestion of what should be done:
|
|
Requiring such a recent version of Microsoft's toolchain just to get a working |
|
I quite agree that this issue is too minor to solve by stepping up to a new development environment. I proposed this only because I had been planning to upgrade from VS 2010 in any case for some time, and this seemed as good a time as any. The FDK is all straight C, and we have been careful to avoid any constructs that are not very much plain vanilla. Quite a few Adobe groups have already stepped up to VS 2017, so overall this seems safe to me. However, you have pointed out an issue I overlooked, which is that most Python extensions are built with older versions of Visual Studio, and we are not doing AFDKO developers a service to require them to upgrade just for the afdko . Given that, I would agree that this year we should upgrade only to VS 2015, and I'll solve the tempname problem for windows separately. |
|
Starting a new PR to address these issues separately. |
Fixes for issues #373 and #371 in dumping GPOS table with spot.