The default bug view has changed. See this FAQ.

missing #includes for OpenBSD

RESOLVED FIXED in mozilla6

Status

()

Core
Build Config
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gaston, Assigned: gaston)

Tracking

Trunk
mozilla6
Other
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(9 attachments, 9 obsolete attachments)

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
(Assignee)

Description

6 years ago
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

6 years ago
Created attachment 512056 [details] [diff] [review]
missing stdint.h to fix build with --enable-debug
Attachment #512056 - Flags: review?
(Assignee)

Comment 2

6 years ago
Created attachment 512057 [details] [diff] [review]
missing stdint.h inclusion for definition of intptr_t
Attachment #512057 - Flags: review?
(Assignee)

Comment 3

6 years ago
Created attachment 512058 [details] [diff] [review]
missing sys/types.h inclusion
Attachment #512058 - Flags: review?
(Assignee)

Comment 4

6 years ago
Created attachment 512059 [details] [diff] [review]
missing sys/types.h inclusion
Attachment #512059 - Flags: review?
(Assignee)

Updated

6 years ago
Version: unspecified → Trunk

Updated

6 years ago
Attachment #512058 - Flags: review? → review?(nnethercote)

Updated

6 years ago
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
(Assignee)

Comment 6

6 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 ?
(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

6 years ago
Created attachment 512219 [details] [diff] [review]
wrap inclusion of stdint.h within defined(XP_UNIX)
Attachment #512056 - Attachment is obsolete: true
Attachment #512056 - Flags: review?
(Assignee)

Comment 9

6 years ago
Created attachment 512220 [details] [diff] [review]
missing stdint.h inclusion for definition of intptr_t

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+
(Assignee)

Comment 10

6 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

6 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

6 years ago
Attachment #512220 - Flags: review? → review?(vladimir)
(Assignee)

Updated

6 years ago
Attachment #512219 - Flags: review? → review?(vladimir)
(Assignee)

Comment 12

6 years ago
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
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.

Updated

6 years ago
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 #512802 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Attachment #512219 - Flags: review?(vladimir) → review?(joe)
(Assignee)

Updated

6 years ago
Attachment #512220 - Flags: review?(vladimir) → review?(joe)
(Assignee)

Comment 18

6 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.
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?)
(Assignee)

Comment 20

6 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 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 ?
(Assignee)

Comment 24

6 years ago
Created attachment 520453 [details] [diff] [review]
Fix build failure in nsWebMReader.cpp

Exact same patch as 512802, but made with hg to please tryserver.
Attachment #512802 - Attachment is obsolete: true
(Assignee)

Comment 25

6 years ago
Created attachment 520454 [details] [diff] [review]
Include stdint.h on unix platforms

Same patch as 512219 but made with hg to please tryserver.
Attachment #512219 - Attachment is obsolete: true
(Assignee)

Comment 26

6 years ago
Created attachment 520455 [details] [diff] [review]
Include stdint.h on unix platforms

Same patch as 512220 but made with hg to please tryserver
Attachment #512220 - Attachment is obsolete: true
(Assignee)

Comment 27

6 years ago
Created attachment 520456 [details] [diff] [review]
Include sys/types.h

Same patch as 512058 but made with hg to please tryserver.
Attachment #512058 - Attachment is obsolete: true
(Assignee)

Comment 28

6 years ago
Created attachment 520457 [details] [diff] [review]
Include sys/types.h

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)
Attachment #520453 - Flags: review?(roc) → review+
(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)
(Assignee)

Comment 38

6 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

6 years ago
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 ?
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
Mac builders failed so pushing mac only : http://hg.mozilla.org/try/pushloghtml?changeset=26aaae3fa7fd and http://tbpl.mozilla.org/?tree=MozillaTry&rev=26aaae3fa7fd
Attachment #524810 - Flags: review?(jones.chris.g) → review+
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
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!
(Assignee)

Comment 43

6 years ago
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...
(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

6 years ago
Created attachment 525491 [details] [diff] [review]
all patches but the nanojit one
(Assignee)

Comment 46

6 years ago
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....
(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

Comment 49

6 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.
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
(Assignee)

Comment 52

6 years ago
Created attachment 525956 [details] [diff] [review]
nsWebMReader diff againt tip

Fixed patch
(Assignee)

Comment 53

6 years ago
Created attachment 525961 [details] [diff] [review]
nsWebMReader diff againt tip

Grr, seems like mercurial hates me, and it's a shared feeling. hopefully against real tip.
Attachment #525956 - Attachment is obsolete: true
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
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Target Milestone: --- → mozilla6
(Assignee)

Comment 55

6 years ago
Phew. Thanks to everyone involved :) !
Keywords: checkin-needed
(Assignee)

Comment 56

6 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

6 years ago
Keywords: checkin-needed

Comment 57

6 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.

Updated

6 years ago
Attachment #525481 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(Assignee)

Comment 58

6 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

6 years ago
Blocks: 650665
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-
You need to log in before you can comment on or make changes to this bug.