Closed
Bug 633857
Opened 14 years ago
Closed 14 years ago
missing #includes for OpenBSD
Categories
(Firefox Build System :: General, defect)
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
|
christian
:
approval-mozilla-aurora-
dveditz
:
approval2.0-
dveditz
:
approval1.9.2.18-
dveditz
:
approval1.9.1.20-
|
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
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #512056 -
Flags: review?
Assignee | ||
Comment 2•14 years ago
|
||
Attachment #512057 -
Flags: review?
Assignee | ||
Comment 3•14 years ago
|
||
Attachment #512058 -
Flags: review?
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #512059 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Version: unspecified → Trunk
Updated•14 years ago
|
Attachment #512058 -
Flags: review? → review?(nnethercote)
Updated•14 years ago
|
Attachment #512059 -
Flags: review? → review?(jones.chris.g)
Comment 5•14 years ago
|
||
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
Assignee | ||
Comment 6•14 years ago
|
||
(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•14 years ago
|
||
(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__ .
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #512056 -
Attachment is obsolete: true
Attachment #512056 -
Flags: review?
Assignee | ||
Comment 9•14 years ago
|
||
wrap inclusion of stdint.h within defined(XP_UNIX)
Attachment #512057 -
Attachment is obsolete: true
Attachment #512057 -
Flags: review?
Updated•14 years ago
|
Attachment #512058 -
Flags: review?(nnethercote) → review+
Updated•14 years ago
|
Attachment #512059 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 10•14 years ago
|
||
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?
Assignee | ||
Comment 11•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
Attachment #512220 -
Flags: review? → review?(vladimir)
Assignee | ||
Updated•14 years ago
|
Attachment #512219 -
Flags: review? → review?(vladimir)
Assignee | ||
Comment 12•14 years ago
|
||
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 13•14 years ago
|
||
Comment on attachment 512059 [details] [diff] [review]
missing sys/types.h inclusion
no risk due to adding #include
Attachment #512059 -
Flags: approval2.0?
Comment 14•14 years ago
|
||
(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•14 years ago
|
||
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 16•14 years ago
|
||
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)
Comment 17•14 years ago
|
||
Oh, I guess comment 12/14 already cover what the fix is that I'd have expected.
Attachment #512802 -
Flags: review?(roc) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #512219 -
Flags: review?(vladimir) → review?(joe)
Assignee | ||
Updated•14 years ago
|
Attachment #512220 -
Flags: review?(vladimir) → review?(joe)
Assignee | ||
Comment 18•14 years ago
|
||
(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.
Updated•14 years ago
|
Assignee: nobody → landry
Comment 19•14 years ago
|
||
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?)
Assignee | ||
Comment 20•14 years ago
|
||
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•14 years ago
|
||
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 22•14 years ago
|
||
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+
Comment 23•14 years ago
|
||
Mark can you push to try ?
Assignee | ||
Comment 24•14 years ago
|
||
Exact same patch as 512802, but made with hg to please tryserver.
Attachment #512802 -
Attachment is obsolete: true
Assignee | ||
Comment 25•14 years ago
|
||
Same patch as 512219 but made with hg to please tryserver.
Attachment #512219 -
Attachment is obsolete: true
Assignee | ||
Comment 26•14 years ago
|
||
Same patch as 512220 but made with hg to please tryserver
Attachment #512220 -
Attachment is obsolete: true
Assignee | ||
Comment 27•14 years ago
|
||
Same patch as 512058 but made with hg to please tryserver.
Attachment #512058 -
Attachment is obsolete: true
Assignee | ||
Comment 28•14 years ago
|
||
Same patch as 512059 but made with hg to please tryserver.
Attachment #512059 -
Attachment is obsolete: true
Comment 29•14 years ago
|
||
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+
Comment 30•14 years ago
|
||
(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•14 years ago
|
||
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+
Comment 32•14 years ago
|
||
(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.
Updated•14 years ago
|
Attachment #520456 -
Flags: review?(dvander)
Comment 33•14 years ago
|
||
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)
Updated•14 years ago
|
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)
Attachment #520453 -
Flags: review?(roc) → review+
Comment 35•14 years ago
|
||
(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 :(
Updated•14 years ago
|
Whiteboard: not-ready
Updated•14 years ago
|
Attachment #520456 -
Flags: review?(dvander) → review+
Comment 36•14 years ago
|
||
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)
Assignee | ||
Comment 38•14 years ago
|
||
(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
Assignee | ||
Comment 39•14 years ago
|
||
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)
Comment 40•14 years ago
|
||
(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•14 years ago
|
||
Mac builders failed so pushing mac only : http://hg.mozilla.org/try/pushloghtml?changeset=26aaae3fa7fd and http://tbpl.mozilla.org/?tree=MozillaTry&rev=26aaae3fa7fd
Updated•14 years ago
|
Attachment #524810 -
Flags: review?(jones.chris.g) → review+
Updated•14 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•14 years ago
|
Whiteboard: not-ready
Comment 42•14 years ago
|
||
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!
Assignee | ||
Comment 43•14 years ago
|
||
Here you are, though i'm not really familiar with mercurial...
Comment 44•14 years ago
|
||
(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
Assignee | ||
Comment 45•14 years ago
|
||
Assignee | ||
Comment 46•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
patching file content/media/webm/nsWebMReader.cpp
Hunk #1 FAILED at 768
Keywords: checkin-needed
Comment 49•14 years ago
|
||
(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•14 years ago
|
||
Also, I will take care for landing.
Comment 51•14 years ago
|
||
landed nanojit part to nanojit-central
http://hg.mozilla.org/projects/nanojit-central/rev/05d5e4afb6e4
Whiteboard: fixed-in-nanojit
Assignee | ||
Comment 52•14 years ago
|
||
Fixed patch
Assignee | ||
Comment 53•14 years ago
|
||
Grr, seems like mercurial hates me, and it's a shared feeling. hopefully against real tip.
Attachment #525956 -
Attachment is obsolete: true
Comment 54•14 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Target Milestone: --- → mozilla6
Assignee | ||
Comment 56•14 years ago
|
||
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?
Updated•14 years ago
|
Keywords: checkin-needed
Comment 57•14 years ago
|
||
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-
Assignee | ||
Comment 58•14 years ago
|
||
(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.
Assignee | ||
Updated•14 years ago
|
Blocks: openbsdmeta
Comment 59•14 years ago
|
||
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
Comment 60•14 years ago
|
||
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-
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•