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

Cygwin build bot? #1274

Closed
khaledhosny opened this issue Oct 18, 2018 · 8 comments
Closed

Cygwin build bot? #1274

khaledhosny opened this issue Oct 18, 2018 · 8 comments
Assignees

Comments

@khaledhosny
Copy link
Collaborator

I’m told that HarfBuzz does not currently build on Cygwin, is this a known issue?:

../../src/hb-blob.cc: In function ‘hb_blob_t* hb_blob_create_from_file(const char*)’:
../../src/hb-blob.cc:545:40: error: ‘_O_BINARY’ was not declared in this scope
   int fd = open (file_name, O_RDONLY | _O_BINARY, 0);
                                        ^~~~~~~~~
../../src/hb-blob.cc:545:40: note: suggested alternative: ‘O_BINARY’
   int fd = open (file_name, O_RDONLY | _O_BINARY, 0);
                                        ^~~~~~~~~
                                        O_BINARY

Do we have a Cygwin build bot, and if not do we want one?

@khaledhosny khaledhosny changed the title Cygwin bot? Cygwin build bot? Oct 18, 2018
@khaledhosny
Copy link
Collaborator Author

Seems to be similar to 21e5d7e (thanks to @davidcarlisle for finding this out).

@HinTak
Copy link
Contributor

HinTak commented Oct 19, 2018

I don't quite remember, but those *BINARY symbols are quite compiler specific ( different windows compilers want with/without leading underscore). You might be a case of using the wrong headers?

@davidcarlisle
Copy link

I can't say I understand where these macros are used but it all builds for me on cygwin with this diff
That is it compiles, I haven't run any tests.


$ git diff
diff --git a/source/libs/harfbuzz/harfbuzz-src/src/hb-blob.cc b/source/libs/harfbuzz/harfbuzz-src/src/hb-blob.cc
index a335df308..1389972eb 100644
--- a/source/libs/harfbuzz/harfbuzz-src/src/hb-blob.cc
+++ b/source/libs/harfbuzz/harfbuzz-src/src/hb-blob.cc
@@ -489,6 +489,9 @@ hb_blob_t::try_make_writable (void)
 
 #if defined(_WIN32) || defined(__CYGWIN__)
 # include <windows.h>
+# ifndef _O_BINARY
+#  define _O_BINARY 0
+# endif
 #else
 # ifndef _O_BINARY
 #  define _O_BINARY 0

@ebraminio
Copy link
Collaborator

is this a known issue?:

No

Do we have a Cygwin build bot, and if not do we want one?

Why not, great

@khaledhosny
Copy link
Collaborator Author

@davidcarlisle what about this patch instead?

diff --git a/source/libs/harfbuzz/harfbuzz-src/src/hb-blob.cc b/source/libs/harfbuzz/harfbuzz-src/src/hb-blob.cc
index a335df308..9f27f520f 100644
--- a/source/libs/harfbuzz/harfbuzz-src/src/hb-blob.cc
+++ b/source/libs/harfbuzz/harfbuzz-src/src/hb-blob.cc
@@ -490,8 +490,8 @@ hb_blob_t::try_make_writable (void)
 #if defined(_WIN32) || defined(__CYGWIN__)
 # include <windows.h>
 #else
-# ifndef _O_BINARY
-#  define _O_BINARY 0
+# ifndef O_BINARY
+#  define O_BINARY 0
 # endif
 #endif
 
@@ -542,7 +542,7 @@ hb_blob_create_from_file (const char *file_name)
   hb_mapped_file_t *file = (hb_mapped_file_t *) calloc (1, sizeof (hb_mapped_file_t));
   if (unlikely (!file)) return hb_blob_get_empty ();
 
-  int fd = open (file_name, O_RDONLY | _O_BINARY, 0);
+  int fd = open (file_name, O_RDONLY | O_BINARY, 0);
   if (unlikely (fd == -1)) goto fail_without_close;
 
   struct stat st;

@behdad
Copy link
Member

behdad commented Oct 19, 2018

If O_BINARY works on cygwin and mingw32, then good. Not sure defining it to 0 is good as that would mask a bug.

@behdad
Copy link
Member

behdad commented Oct 19, 2018

This says cygwin has O_BINARY: http://www.cygwin.com/cygwin-ug-net/using-textbinary.html
Not sure why I changed it back then.

@khaledhosny khaledhosny self-assigned this Oct 19, 2018
@davidcarlisle
Copy link

If O_BINARY works on cygwin and mingw32, then good. Not sure defining it to 0 is good as that would mask a bug.

yes I was happy just to isolate where to fix, I wasn't at all confident that setting it to 0 was the correct fix other than it got the make build not to die at that point. I'll try with the diff from @khaledhosny

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

No branches or pull requests

5 participants