Closed Bug 633857 Opened 13 years ago Closed 13 years ago

missing #includes for OpenBSD

Categories

(Firefox Build System :: General, defect)

Other
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla6

People

(Reporter: gaston, Assigned: gaston)

References

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey)

Attachments

(9 files, 9 obsolete files)

501 bytes, patch
roc
: review+
Details | Diff | Splinter Review
320 bytes, patch
Usul
: review+
Details | Diff | Splinter Review
344 bytes, patch
Usul
: review+
Details | Diff | Splinter Review
338 bytes, patch
dvander
: review+
Details | Diff | Splinter Review
432 bytes, patch
cjones
: review+
Details | Diff | Splinter Review
3.51 KB, patch
dveditz
: approval2.0-
Details | Diff | Splinter Review
2.80 KB, patch
Details | Diff | Splinter Review
931 bytes, patch
Details | Diff | Splinter Review
784 bytes, patch
Details | Diff | Splinter Review
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
Attached patch missing sys/types.h inclusion (obsolete) — Splinter Review
Attachment #512058 - Flags: review?
Attached patch missing sys/types.h inclusion (obsolete) — Splinter Review
Attachment #512059 - Flags: review?
Version: unspecified → Trunk
Attachment #512058 - Flags: review? → review?(nnethercote)
Attachment #512059 - Flags: review? → review?(jones.chris.g)
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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 ?
(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__ .
Attachment #512056 - Attachment is obsolete: true
Attachment #512056 - Flags: review?
wrap inclusion of stdint.h within defined(XP_UNIX)
Attachment #512057 - Attachment is obsolete: true
Attachment #512057 - Flags: review?
Attachment #512058 - Flags: review?(nnethercote) → review+
Attachment #512059 - Flags: review?(jones.chris.g) → review+
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.
Attachment #512219 - Flags: review?
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.
Attachment #512220 - Flags: review?
Attachment #512220 - Flags: review? → review?(vladimir)
Attachment #512219 - Flags: review? → review?(vladimir)
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
Attachment #512802 - Flags: review?(dolske)
Comment on attachment 512059 [details] [diff] [review]
missing sys/types.h inclusion

no risk due to adding #include
Attachment #512059 - Flags: approval2.0?
(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.
Depends on: 634793
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
Attachment #512059 - Flags: approval2.0? → approval2.0-
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 joe@drew.ca 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()?
Attachment #512802 - Flags: review?(dolske) → review?(roc)
Oh, I guess comment 12/14 already cover what the fix is that I'd have expected.
Attachment #512219 - Flags: review?(vladimir) → review?(joe)
Attachment #512220 - Flags: review?(vladimir) → review?(joe)
(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.
Assignee: nobody → landry
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?)
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 on attachment 512219 [details] [diff] [review]
wrap inclusion of stdint.h within defined(XP_UNIX)

r=me as long as this passes try
Attachment #512219 - Flags: review?(joe) → review+
Comment on attachment 512220 [details] [diff] [review]
missing stdint.h inclusion for definition of intptr_t

r=me as long as this passes try
Attachment #512220 - Flags: review?(joe) → review+
Mark can you push to try ?
Exact same patch as 512802, but made with hg to please tryserver.
Attachment #512802 - Attachment is obsolete: true
Same patch as 512219 but made with hg to please tryserver.
Attachment #512219 - Attachment is obsolete: true
Same patch as 512220 but made with hg to please tryserver
Attachment #512220 - Attachment is obsolete: true
Same patch as 512058 but made with hg to please tryserver.
Attachment #512058 - Attachment is obsolete: true
Attached patch Include sys/types.h (obsolete) — Splinter Review
Same patch as 512059 but made with hg to please tryserver.
Attachment #512059 - Attachment is obsolete: true
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
Attachment #520454 - Flags: review+
(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 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
Attachment #520455 - Flags: review+
(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.
Attachment #520456 - Flags: review?(dvander)
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.
Attachment #520457 - Flags: review?(ted.mielczarek)
Attachment #520453 - Flags: review?(jones.chris.g)
Comment on attachment 520453 [details] [diff] [review]
Fix build failure in nsWebMReader.cpp

I can't review code here.
Attachment #520453 - Flags: review?(jones.chris.g) → review?(roc)
(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 :(
Whiteboard: not-ready
Attachment #520456 - Flags: review?(dvander) → review+
Comment on attachment 520457 [details] [diff] [review]
Include sys/types.h

Punting to cjones, sorry.
Attachment #520457 - Flags: review?(ted.mielczarek) → review?(jones.chris.g)
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.
Attachment #520457 - Flags: review?(jones.chris.g)
(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 ?
OS: OpenBSD → All
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 ?
Attachment #520457 - Attachment is obsolete: true
Attachment #524810 - Flags: review?(jones.chris.g)
(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
Attachment #524810 - Flags: review?(jones.chris.g) → review+
Whiteboard: not-ready
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!
Here you are, though i'm not really familiar with mercurial...
(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
Attached patch nanojit patchSplinter Review
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....
(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.
patching file content/media/webm/nsWebMReader.cpp
Hunk #1 FAILED at 768
Keywords: checkin-needed
(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.
Also, I will take care for landing.
landed nanojit part to nanojit-central
http://hg.mozilla.org/projects/nanojit-central/rev/05d5e4afb6e4
Whiteboard: fixed-in-nanojit
Attached patch nsWebMReader diff againt tip (obsolete) — Splinter Review
Fixed patch
Grr, seems like mercurial hates me, and it's a shared feeling. hopefully against real tip.
Attachment #525956 - Attachment is obsolete: true
Target Milestone: --- → mozilla6
Phew. Thanks to everyone involved :) !
Keywords: checkin-needed
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 ?
Attachment #525481 - Flags: approval2.0?
Attachment #525481 - Flags: approval1.9.2.18?
Attachment #525481 - Flags: approval1.9.1.20?
Attachment #525481 - Flags: approval-mozilla-aurora?
Keywords: checkin-needed
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.
Attachment #525481 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(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.
Blocks: openbsdmeta
http://hg.mozilla.org/tracemonkey/rev/6c0c0e9351fd
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
Comment on attachment 525481 [details] [diff] [review]
patch ready to land for the 5 diffs

Same rationale for older branches as for aurora
Attachment #525481 - Flags: approval2.0?
Attachment #525481 - Flags: approval2.0-
Attachment #525481 - Flags: approval1.9.2.18?
Attachment #525481 - Flags: approval1.9.2.18-
Attachment #525481 - Flags: approval1.9.1.20?
Attachment #525481 - Flags: approval1.9.1.20-
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: