Closed Bug 847078 Opened 12 years ago Closed 12 years ago

Can't build using VC9 errors in harfbuzz (hb-common.h)

Categories

(Core :: Layout: Text and Fonts, defect)

x86
Windows 7
defect
Not set
blocker

Tracking

()

RESOLVED FIXED

People

(Reporter: wgianopoulos, Unassigned)

References

Details

(Keywords: regression)

Attachments

(2 obsolete files)

I have no real idea what is going on here as I can't find a recent change anywhere in the area where the build fails. It worked esterday and fails today. I have a really hacky workaround I will be posting soon.
These are the errors I am getting: c:\users\wag\mozilla\mozilla2\fx-obj\dist\include\harfbuzz\hb-common.h(56) : error C2371: 'int8_t' : redefinition; different basic types c:\users\wag\mozilla\mozilla2\fx-obj\dist\include\mozilla/MSStdInt.h(82) : see declaration of 'int8_t'
I suspect the correct fix here is to define HB_DONT_DEFINE_STDINT, but I was not sure where to put this.
Oh I also suspect this is the entirely incorrect component, yet this seems to be the component that other harfbuzz bugs have been filed under.
We should probably just add an AC_DEFINE for this in configure.
Attached patch Really hacky workaround (obsolete) (deleted) — Splinter Review
This is a really hacky workaround. I really think the better approach is what I suggested in comment 2.
Or what Ted suggested in comment 4.
Hmm seems defining HB_DONT_DEFINE_STDINT does not work. It results in broken compiles both under VC9 and using gcc 4.5.1 under Linux.
In any case, we already define HB_DONT_DEFINE_STDINT, I believe. See gfx/thebes/Makefile.in. Given that "It worked [y]esterday and fails today" (comment #0), could you bisect to find what change triggered the problem you're seeing?
(In reply to Jonathan Kew (:jfkthame) from comment #8) > In any case, we already define HB_DONT_DEFINE_STDINT, I believe. See > gfx/thebes/Makefile.in. Hmm, that would apply when we're #including harfbuzz headers in thebes, but it wouldn't apply to the compilation of harfbuzz itself. We probably should have that defined in gfx/harfbuzz/src/Makefile.in as well.
(In reply to Jonathan Kew (:jfkthame) from comment #9) > (In reply to Jonathan Kew (:jfkthame) from comment #8) > > In any case, we already define HB_DONT_DEFINE_STDINT, I believe. See > > gfx/thebes/Makefile.in. > > Hmm, that would apply when we're #including harfbuzz headers in thebes, but > it wouldn't apply to the compilation of harfbuzz itself. We probably should > have that defined in gfx/harfbuzz/src/Makefile.in as well. Sorry, no, ignore that comment ... we do only want that to be #define'd when we're #include-ing harfbuzz headers in non-harfbuzz code. When building harfbuzz itself, it needs to define the stdint types one way or another, which is what the code in hb-common.h is supposed to achieve. Re the error: >c:\users\wag\mozilla\mozilla2\fx-obj\dist\include\harfbuzz\hb-common.h(56) : error C2371: 'int8_t' : redefinition; different basic types > > c:\users\wag\mozilla\mozilla2\fx-obj\dist\include\mozilla/MSStdInt.h(82) : see declaration of 'int8_t' what file is actually being compiled when you encounter this?
See bug 722299 comment 55 and following
Attached patch possible fix (obsolete) (deleted) — Splinter Review
Attachment #720455 - Flags: review?
This essentially does the same as my previous "Really hacky workaround". However, it does not hack at the upstream code as badly. It does actually seem to fix the issue.
QA Contact: bill
Attachment #720455 - Flags: review? → review?(ted)
Assignee: nobody → bill
QA Contact: bill
(In reply to Jonathan Kew (:jfkthame) from comment #8) > In any case, we already define HB_DONT_DEFINE_STDINT, I believe. See > gfx/thebes/Makefile.in. > > Given that "It worked [y]esterday and fails today" (comment #0), could you > bisect to find what change triggered the problem you're seeing? If we already define that, then I have no idea how the patch that fixes this could possibly work.
(In reply to Jonathan Kew (:jfkthame) from comment #8) > In any case, we already define HB_DONT_DEFINE_STDINT, I believe. See > gfx/thebes/Makefile.in. > > Given that "It worked [y]esterday and fails today" (comment #0), could you > bisect to find what change triggered the problem you're seeing? Nor do I understand how if this is the case that defining this in configure.in breaks the build on all platforms.
See Simon's comment above. Does the patch from bug 722299 comment 57 fix things for you? AFAICT we should *not* have HB_DONT_DEFINE_STDINT when compiling harfbuzz itself (so we don't want to define it globally for the entire build). But we *do* want to define it when compiling non-harfbuzz source files that include the harfbuzz headers, as we use mozilla/StandardInteger.h to provide those typedefs in the rest of our codebase.
(In reply to Jonathan Kew (:jfkthame) from comment #16) > See Simon's comment above. Does the patch from bug 722299 comment 57 fix > things for you? > > AFAICT we should *not* have HB_DONT_DEFINE_STDINT when compiling harfbuzz > itself (so we don't want to define it globally for the entire build). But we > *do* want to define it when compiling non-harfbuzz source files that include > the harfbuzz headers, as we use mozilla/StandardInteger.h to provide those > typedefs in the rest of our codebase. I am trying builds with that patch now. Is there something wrong with bugzilla mail? I have no gotten emails about any comments on this bug since comment 4.
(In reply to Bill Gianopoulos [:WG9s] from comment #17) > I am trying builds with that patch now. Is there something wrong with > bugzilla mail? I have no gotten emails about any comments on this bug since > comment 4. I found that issue. Comcast seems to now be blocking inbound access to port 25! they had sent out a notice that they would be blocking access to port 25 outbound, but it made no mention of inbound.
(In reply to Jonathan Kew (:jfkthame) from comment #16) > See Simon's comment above. Does the patch from bug 722299 comment 57 fix > things for you? Yes that DOES fix the issue for me. Sorry I did not see the previous comments sooner.
Attachment #720455 - Attachment is obsolete: true
Attachment #720455 - Flags: review?(ted)
Attachment #720321 - Attachment is obsolete: true
Assignee: wgianopoulos → nobody
Blocks: 722299
Fixed by this check-in.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Would have been even better with the link: https://hg.mozilla.org/mozilla-central/rev/333e89511b31
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: