633857 - missing #includes for OpenBSD

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Landry Breuil (:gaston)]

User-Agent:       Midori/0.2 (X11; OpenBSD; U; fr-fr) WebKit/531.2+
Build Identifier: 

Trunk fails to build on OpenBSD in various places, because of a bunch of missing #include statements.
Those shouldn't harm other platforms, hence i didn't put them in defined() blocks.

Reproducible: Always

Comment 1

[Landry Breuil (:gaston)]

Attachment: missing stdint.h to fix build with --enable-debug

Comment 2

[Landry Breuil (:gaston)]

Attachment: missing stdint.h inclusion for definition of intptr_t

Comment 3

[Landry Breuil (:gaston)]

Attachment: missing sys/types.h inclusion

Comment 4

[Landry Breuil (:gaston)]

Attachment: missing sys/types.h inclusion

Comment 5

[Makoto Kato [:m_kato]]

Landry, you should set reviewer (I set it about last 2 patches).  Also, do you test your patches on other platform such as Windows?

I think that first 2 patches for gfx/thebe is incorrect. Because VC8 and VC9 have no stdint.h, so this cause bustage.

Comment 6

[Landry Breuil (:gaston)]

(In reply to comment #5)
> Landry, you should set reviewer (I set it about last 2 patches).  Also, do you
> test your patches on other platform such as Windows?

I only use one platform.

> I think that first 2 patches for gfx/thebe is incorrect. Because VC8 and VC9
> have no stdint.h, so this cause bustage.

Sure, maybe put them inside #ifdef HAVE_STDINT_H, as it seems to be a common idiom in mozilla codebase for that case. I'm not aware of the correct way of including system-specific headers in system-independant code, maybe XP_UNIX ?

Comment 7

[Makoto Kato [:m_kato]]

(In reply to comment #6)
> Sure, maybe put them inside #ifdef HAVE_STDINT_H, as it seems to be a common
> idiom in mozilla codebase for that case. I'm not aware of the correct way of
> including system-specific headers in system-independant code, maybe XP_UNIX ?

mozilla-central code has no HAVE_STDINT_H.  You should add check in configure.in like http://mxr.mozilla.org/mozilla-central/source/js/src/configure.in?mark=3121-3135#3123, or use XP_UNIX or OS predefined macro such as __OpenBSD__ .

Comment 8

[Landry Breuil (:gaston)]

Attachment: wrap inclusion of stdint.h within defined(XP_UNIX)

Comment 9

[Landry Breuil (:gaston)]

Attachment: missing stdint.h inclusion for definition of intptr_t

wrap inclusion of stdint.h within defined(XP_UNIX)

Comment 10

[Landry Breuil (:gaston)]

Comment on attachment 512219 [details] [diff] [review]
wrap inclusion of stdint.h within defined(XP_UNIX)

Setting ? for review flag, i have no idea whom to assign it, and https://wiki.mozilla.org/Bugzilla:Review points to a list of reviewers for... bugzilla itself.

Comment 11

[Landry Breuil (:gaston)]

Comment on attachment 512220 [details] [diff] [review]
missing stdint.h inclusion for definition of intptr_t

Setting ? for review flag, i have no idea whom to assign it, and
https://wiki.mozilla.org/Bugzilla:Review points to a list of reviewers for...
bugzilla itself.

Comment 12

[Landry Breuil (:gaston)]

Attachment: Fix build failure in nsWebMReader.cpp

Without this patch, fails to build on OpenBSD/amd64 with :
nsWebMReader.cpp:704: error: invalid conversion from 'PRUint64*' to 'uint64_t*'

Might be related to https://bugzilla.mozilla.org/show_bug.cgi?id=599764

Comment 13

[Makoto Kato [:m_kato]]

Comment on attachment 512059 [details] [diff] [review]
missing sys/types.h inclusion

no risk due to adding #include

Comment 14

[Makoto Kato [:m_kato]]

(In reply to comment #12)
> Created attachment 512802 [details] [diff] [review]
> Fix build failure in nsWebMReader.cpp
> 
> Without this patch, fails to build on OpenBSD/amd64 with :
> nsWebMReader.cpp:704: error: invalid conversion from 'PRUint64*' to 'uint64_t*'
> 
> Might be related to https://bugzilla.mozilla.org/show_bug.cgi?id=599764

You should file NSPR bug.  We need OS macro like __APPLE__ into prtypes.h to fix this.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 512059 [details] [diff] [review]
missing sys/types.h inclusion

r+ before a? please, for all these patches, and make sure it passes tryserver

Comment 16

[Justin Dolske [:Dolske]]

Comment on attachment 512802 [details] [diff] [review]
Fix build failure in nsWebMReader.cpp

I can't review this, you probably want roc. I've moved the review. (You may also want [e-mail] for the GFX bits, not sure if Vlad is still active there).

Seems like an odd change, though, since PRUint64s are used all over this file. Maybe OpenBSD is missing some header-fu to make the conversion to a pointer work properly? Or some conflicting definition of nestegg_tstamp_scale()?

Comment 17

[Justin Dolske [:Dolske]]

Oh, I guess comment 12/14 already cover what the fix is that I'd have expected.

Comment 18

[Landry Breuil (:gaston)]

(In reply to comment #15)
> Comment on attachment 512059 [details] [diff] [review]
> missing sys/types.h inclusion
> 
> r+ before a? please, for all these patches, and make sure it passes tryserver

What's the procedure to get those patches sent to tryserver now ? Wait for r+ on the two stdint.h patches ?

https://bugzilla.mozilla.org/show_bug.cgi?id=634793 confirmed that https://bugzilla.mozilla.org/attachment.cgi?id=512802&action=diff was the right way to go.

Comment 19

[Joe Drew (not getting mail)]

Comment on attachment 512219 [details] [diff] [review]
wrap inclusion of stdint.h within defined(XP_UNIX)

Is there a downside to just including stdint.h everywhere? (Does that work on Windows?)

Comment 20

[Landry Breuil (:gaston)]

I don't know Windows SDKs and which one mozilla uses, but the wikipedia page of stdint.h states that this file is "not shipped with older C++ compilers and Visual Studio C++ products prior to Visual Studio 2010".

Comment 21

[Joe Drew (not getting mail)]

Comment on attachment 512219 [details] [diff] [review]
wrap inclusion of stdint.h within defined(XP_UNIX)

r=me as long as this passes try

Comment 22

[Joe Drew (not getting mail)]

Comment on attachment 512220 [details] [diff] [review]
missing stdint.h inclusion for definition of intptr_t

r=me as long as this passes try

Comment 23

[Ludovic Hirlimann [:Usul]]

Mark can you push to try ?

Comment 24

[Landry Breuil (:gaston)]

Attachment: Fix build failure in nsWebMReader.cpp

Exact same patch as 512802, but made with hg to please tryserver.

Comment 25

[Landry Breuil (:gaston)]

Attachment: Include stdint.h on unix platforms

Same patch as 512219 but made with hg to please tryserver.

Comment 26

[Landry Breuil (:gaston)]

Attachment: Include stdint.h on unix platforms

Same patch as 512220 but made with hg to please tryserver

Comment 27

[Landry Breuil (:gaston)]

Attachment: Include sys/types.h

Same patch as 512058 but made with hg to please tryserver.

Comment 28

[Landry Breuil (:gaston)]

Attachment: Include sys/types.h

Same patch as 512059 but made with hg to please tryserver.

Comment 29

[Ludovic Hirlimann [:Usul]]

Comment on attachment 520454 [details] [diff] [review]
Include stdint.h on unix platforms

Carrying r+ from attachement 512219.

Pushed to try :
http://hg.mozilla.org/try/pushloghtml?changeset=f81d08f55de4
http://tbpl.mozilla.org/?tree=MozillaTry&rev=f81d08f55de4

Comment 30

[Ludovic Hirlimann [:Usul]]

(In reply to comment #29)
> Comment on attachment 520454 [details] [diff] [review]
> Include stdint.h on unix platforms
> 
> Carrying r+ from attachement 512219.
> 
> Pushed to try :
> http://hg.mozilla.org/try/pushloghtml?changeset=f81d08f55de4
> http://tbpl.mozilla.org/?tree=MozillaTry&rev=f81d08f55de4

Passes.

Comment 31

[Ludovic Hirlimann [:Usul]]

Comment on attachment 520455 [details] [diff] [review]
Include stdint.h on unix platforms

Carrying r+

Pushed to try http://tbpl.mozilla.org/?tree=MozillaTry&rev=1e5ac56f8706

Comment 32

[Ludovic Hirlimann [:Usul]]

(In reply to comment #31)
> Comment on attachment 520455 [details] [diff] [review]
> Include stdint.h on unix platforms
> 
> Carrying r+
> 
> Pushed to try http://tbpl.mozilla.org/?tree=MozillaTry&rev=1e5ac56f8706

Passes. Now we need to wait for the tree to reopen to mark these checking needed.

Comment 33

[Ludovic Hirlimann [:Usul]]

Comment on attachment 520457 [details] [diff] [review]
Include sys/types.h

Ted ain't sure you're the right person to ask review to , sorry if you aren't.

Comment 34

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 520453 [details] [diff] [review]
Fix build failure in nsWebMReader.cpp

I can't review code here.

Comment 35

[Ludovic Hirlimann [:Usul]]

(In reply to comment #34)
> Comment on attachment 520453 [details] [diff] [review]
> Fix build failure in nsWebMReader.cpp
> 
> I can't review code here.

Wasn't sure who could for Webm :(

Comment 36

[(not currently active) Ted Mielczarek]

Comment on attachment 520457 [details] [diff] [review]
Include sys/types.h

Punting to cjones, sorry.

Comment 37

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 520457 [details] [diff] [review]
Include sys/types.h

Why is sys/types.h only included #ifdef MALLOC_H?  MSVC has sys/types.h; if you just include it unconditionally and it builds everywhere, that would be fine with me.

Comment 38

[Landry Breuil (:gaston)]

(In reply to comment #37)
> Comment on attachment 520457 [details] [diff] [review]
> Include sys/types.h
> 
> Why is sys/types.h only included #ifdef MALLOC_H?  MSVC has sys/types.h; if you
> just include it unconditionally and it builds everywhere, that would be fine
> with me.

Sure, find someone to push it to try. I'm not the one who will test for other archs, and i have no idea if it can cause follout there. 

Build failure was:
In file included from mozalloc.cpp:46:
/usr/include/sys/malloc.h:324: error: 'u_short' does not name a type
/usr/include/sys/malloc.h:325: error: 'u_short' does not name a type
/usr/include/sys/malloc.h:338: error: 'u_short' does not name a type
/usr/include/sys/malloc.h:339: error: 'u_short' does not name a type
/usr/include/sys/malloc.h:349: error: 'caddr_t' does not name a type
/usr/include/sys/malloc.h:350: error: 'caddr_t' does not name a type
/usr/include/sys/malloc.h:351: error: 'u_int64_t' does not name a type
/usr/include/sys/malloc.h:352: error: 'u_int64_t' does not name a type
/usr/include/sys/malloc.h:353: error: 'u_int64_t' does not name a type
/usr/include/sys/malloc.h:354: error: 'u_int64_t' does not name a type
/usr/include/sys/malloc.h:355: error: 'u_int64_t' does not name a type
/usr/include/sys/malloc.h:356: error: 'u_int64_t' does not name a type

To me, the logic is that one should not include sys/malloc.h directly, but rather use stdlib.h. The comments states it's for valloc(), which requires stdlib.h.

All those patches were already r+, why do we need to discuss them again and again ?

Comment 39

[Landry Breuil (:gaston)]

Attachment: Include sys/types.h inconditionally

Include sys/types.h inconditionally as requested in https://bugzilla.mozilla.org/show_bug.cgi?id=633857#c37
Can someone push it to try-server ?

Comment 40

[Ludovic Hirlimann [:Usul]]

(In reply to comment #39)
> Created attachment 524810 [details] [diff] [review]
> Include sys/types.h inconditionally
> 
> Include sys/types.h inconditionally as requested in
> https://bugzilla.mozilla.org/show_bug.cgi?id=633857#c37
> Can someone push it to try-server ?

http://hg.mozilla.org/try/pushloghtml?changeset=780a8c250c14
http://tbpl.mozilla.org/?tree=MozillaTry&rev=780a8c250c14

Comment 41

[Ludovic Hirlimann [:Usul]]

Mac builders failed so pushing mac only : http://hg.mozilla.org/try/pushloghtml?changeset=26aaae3fa7fd and http://tbpl.mozilla.org/?tree=MozillaTry&rev=26aaae3fa7fd

Comment 42

[(no longer active)]

Which one of these patches needs to be landed?  If all 5, then could you please submit a single patch combining them (or a bungle containing all 5 of them)?

Please see <https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f> on how to generate the correct format of patches.  Thanks!

Comment 43

[Landry Breuil (:gaston)]

Attachment: patch ready to land for the 5 diffs

Here you are, though i'm not really familiar with mercurial...

Comment 44

[Makoto Kato [:m_kato]]

(In reply to comment #43)
> Created attachment 525481 [details] [diff] [review]
> patch ready to land for the 5 diffs
> 
> Here you are, though i'm not really familiar with mercurial...

Don't include nanojit fix.  See https://developer.mozilla.org/en/NanojitMerge

Comment 45

[Landry Breuil (:gaston)]

Attachment: all patches but the nanojit one

Comment 46

[Landry Breuil (:gaston)]

Attachment: nanojit patch

Can't someone familiar with mozilla trees and process take care of landing the patches, in one go or separated ? I've done the same patches 3 or 4 times, they are mostly oneliners, have been reviewed twice, passed try-servers, but still can't get simply commited.
The process is utterly painful. No wonder why contributors are dragged away....

Comment 47

[Makoto Kato [:m_kato]]

(In reply to comment #46)
> Created attachment 525492 [details] [diff] [review]
> nanojit patch
> 
> Can't someone familiar with mozilla trees and process take care of landing the
> patches, in one go or separated ? I've done the same patches 3 or 4 times, they
> are mostly oneliners, have been reviewed twice, passed try-servers, but still
> can't get simply commited.
> The process is utterly painful. No wonder why contributors are dragged away....

nanojit fix is already reviewed by dvander.  But checked-in rule for nanojit is different.  Because it is shared with Adobe (for ActionScript).  So we must land it into nanojit-central at first, then it merged into tracemonkey tree, then mozilla-central...  Mozilla uses a lot of third-party code, so there is a few complex rules.

I understand that you (and non-Mozilla developer) confuse this.  But each OSS projects have owns checked-in rule.  (You are OpenBSD developer, so you can understand it). If you separate bugs each modules when filing bugs, I believe that review process will be more easy.

Also, unfortunately, Although you filed this at Feb, that time was needed a+ for Firefox 4, also for Firefox 5, tree was managed.  So this will be landed for Firefox 6.

Comment 48

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

patching file content/media/webm/nsWebMReader.cpp
Hunk #1 FAILED at 768

Comment 49

[Paul Biggar]

(In reply to comment #46)
> The process is utterly painful. No wonder why contributors are dragged away....

I agree. We've got a process underway to try and make this easier. I'm going to bring this bug up as an example of how difficult it is for contributors.

I'd just like to say that as an organization, we are very interested in solving this. However, with the number of modules, components, and even repositories, we're currently facing an uphill battle.

I believe that this is felt most strongly in patches like this that cross many boundaries, and that it isn't quite so bad otherwise. But I shepherded a new contributor through a bug that needed to be split into 5 parts and get 5 separate reviewers, and it was certainly a harrowing experience.

I'm going to bring this up on mozillians@ (and maybe dev-planning@), and see if we can figure this out. I'll CC you.

Comment 50

[Makoto Kato [:m_kato]]

Also, I will take care for landing.

Comment 51

[Makoto Kato [:m_kato]]

landed nanojit part to nanojit-central
http://hg.mozilla.org/projects/nanojit-central/rev/05d5e4afb6e4

Comment 52

[Landry Breuil (:gaston)]

Attachment: nsWebMReader diff againt tip

Fixed patch

Comment 53

[Landry Breuil (:gaston)]

Attachment: nsWebMReader diff againt tip

Grr, seems like mercurial hates me, and it's a shared feeling. hopefully against real tip.

Comment 54

[Makoto Kato [:m_kato]]

landed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/cfb795311a30
http://hg.mozilla.org/mozilla-central/rev/005a5f7ba52c
http://hg.mozilla.org/mozilla-central/rev/85ced3bcb55d

Comment 55

[Landry Breuil (:gaston)]

Phew. Thanks to everyone involved :) !

Comment 56

[Landry Breuil (:gaston)]

Comment on attachment 525481 [details] [diff] [review]
patch ready to land for the 5 diffs

Since those are only build fixes and causes no harm on m-c, can this patch (well, those 5 patches/4 commits) be backported to existing branches ?
If needed, i can provide distinct diffs against each branches, but i'd like to avoid checking out 3 * releases/mozilla-xxx and mozilla-aurora.

What's the process here, should i reopen this bug, or file new ones targetting each branch ?

Comment 57

[LegNeato]

You got the process right :-)

We're not going to approve for aurora:

1. BSD distros can patch on top for 5
2. BDS distros will get the convenience for 6 (6 weeks after 5) as this already landed on mozilla-central

The other branches are a different issue as they have different rules and update cadences. The branch triage meeting will take care of those.

Comment 58

[Landry Breuil (:gaston)]

(In reply to comment #57)
> You got the process right :-)
> 
> We're not going to approve for aurora:
> 
> 1. BSD distros can patch on top for 5
> 2. BDS distros will get the convenience for 6 (6 weeks after 5) as this already
> landed on mozilla-central

Agreed, thanks makes sense with the new schedule.

> The other branches are a different issue as they have different rules and
> update cadences. The branch triage meeting will take care of those.

ok, thanks.

Comment 59

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/6c0c0e9351fd

Comment 60

[Daniel Veditz [:dveditz]]

Comment on attachment 525481 [details] [diff] [review]
patch ready to land for the 5 diffs

Same rationale for older branches as for aurora