Skip to content
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

Fix CMake builds on Windows, MSVC in particular #877

Merged
merged 6 commits into from
Mar 13, 2018

Conversation

fanc999
Copy link
Contributor

@fanc999 fanc999 commented Mar 12, 2018

Hi,

The CMake build files evolved with more testing support, but also resulted in builds (and tests) failing on Windows, in particular the older versions of Visual Studio. Please bear with me, as I am not sure of the compiler support policy, but this attempts to fix building and testing on Visual Studio.

Note that after this patchset all the tests (including shaping, fuzzing and subset, in addition to the API tests) pass on Visual Studio 2008/x86 and 2017/x64 builds, except for the tests/basics.tests (Failed) and tests/full-font.tests (Failed), as the which command could not be found, and it is only usable on Windows via MSYS or Cygwin builds, AFAICT, which I did not (yet) try to address here.

Note also that as we gained more source files for the main HarfBuzz/HarfBuzz-GObject DLLs, the command line that we need to build the introspection files becomes too long for Windows, so I am also currently looking for a way to fix this here, by using the --filelist option for g-ir-scanner, which I will try to address a little bit later.

With blessings, thank you!

@fanc999 fanc999 force-pushed the master.msvc branch 10 times, most recently from 3276fe3 to a0e1de7 Compare March 12, 2018 07:33
@fanc999
Copy link
Contributor Author

fanc999 commented Mar 12, 2018

Hmm,

Somehow I was having problems getting the image on Travis to recognize that it does indeed have round(), but nothing could convince it, even if I tried looking for round in the 'm' library, although the circleci had no complaints...

I am in no way knowledgeable in the math library stuff on *NIX, so I think I need some advice for this.

With blessings, thank you!

@ebraminio
Copy link
Collaborator

ebraminio commented Mar 12, 2018

as I am not sure of the compiler support policy

I feel the policy is like if you can put it on CI so collaborators know they are breaking something with their change, then it is a supported compiler for a foreseeable future; we have a compiler for DOS on our CI (djgpp, which compiles 32bit executable of course) and a gaming console related (psvita) compiler (which had issues to support), nothing can beat those I'd guess :)

(Adding Garret as a reviewer was not intentional, sorry for the noise)

@ebraminio ebraminio removed the request for review from garretrieger March 12, 2018 08:35
@ebraminio
Copy link
Collaborator

@fanc999 fanc999 force-pushed the master.msvc branch 13 times, most recently from 7630a0a to 1712416 Compare March 13, 2018 11:58
Not all the compilers that HarfBuzz is buildable on supports round() and
has the header stdbool.h, so we check for them and define HAVE_ROUND and
HAVE_STDBOOL_H repsectively in our CFLAGS so that we include them only
when they are found, or use fallback implementations when necessary.

Also include FindPythonInterp earlier as we need PYTHON_EXECUTABLE to be
set for running the tests.
Add a simplistic round() implementation for our purposes, used when the
compiler does not support round() directly.
For the API tests, output the test programs at $(TOP_BUILDDIR) so that
the freshly-built DLLs will be available for the test programs.  For
those that are run through the Python wrapper scripts, use
${PYTHON_EXECUTABLE} instead of plain 'python' in case the Python
interpreter is not in the PATH.
Include stdbool.h in hb-setset-test.h instead of in the individual
sources, if it is found; otherwise use a simplistic fallback for it if
it is not found.

Also declare variables at the top of the block, to build on pre-C99
compiliers.
The list of source files to pass to g-ir-scanner is becoming too
long for Windows, as Windows imposes a 8192-character limit for command
lines, so we need to first transform that list into a listings file, and
then use the --filelist option for g-ir-scanner to build the
introspection files.
Put in the utility program that was missed in installation by replacing
the one that was duplicated.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 64.712% when pulling 7c43ada on fanc999:master.msvc into 28f25f3 on harfbuzz:master.

@fanc999
Copy link
Contributor Author

fanc999 commented Mar 13, 2018

Hi @ebraminio,

Thanks for the tips...

Seems that the GCC/CLang installation on Travis is older, so it didn't pick up the round() from math.h as it didn't support C99 flags when checking somehow (see the outputs). So, as a workaround, I had it look for round()'s declaration in math.h if AC_CHECK_FUNCS([round]) did not work even as we do -lm...

Does not look the nicest, but at least it worked on Travis... :|

With blessings, thank you!

@ebraminio
Copy link
Collaborator

ebraminio commented Mar 13, 2018

Thank you very much @fanc999, this looks great! @behdad if the autotools change part make good I think this can be merged

@ebraminio
Copy link
Collaborator

ebraminio commented Mar 13, 2018

I really don't know how your change has broken msys2 (or maybe something has changed on appveyor side)... I've added gen-def.py to project so I feel responsible for it somehow also, can you see why it is failing? I guess maybe a change from io.open(h, encoding='utf8') to io.open(h, 'rb', encoding='utf8') can fix it, if not (and you had no any other idea) we will merge this as is and fix that later. Thank you

@anthrotype
Copy link
Member

maybe a change from io.open(h, encoding='utf8') to io.open(h, 'rb', encoding='utf8') can fix it

The encoding argument is for reading text, can’t be used with rb, which is for reading bytes.

@ebraminio
Copy link
Collaborator

Thank you, do you have any other idea? I couldn't reproduce this locally on my machine with a fairly similar configuration...

@anthrotype
Copy link
Member

you should check that the text file your are trying to read in as UTF-8 was actually encoded as UTF-8.
Maybe it was written with some different encoding. On Windows, the default encoding might not be UTF-8.

@ebraminio
Copy link
Collaborator

The failure happens regardless of this patch and I think I have a solution for this so lets merge this first and then fix that

@ebraminio ebraminio merged commit a12dd6f into harfbuzz:master Mar 13, 2018
@ebraminio
Copy link
Collaborator

Some essential part of this is reverted on 86a0ac2 @behdad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake CMake related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants