Bug 1230117 broke bsd builds with bundled nspr

RESOLVED FIXED in Firefox 47

Status

Firefox Build System
General
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: gaston, Assigned: gaston)

Tracking

(Depends on: 1 bug)

unspecified
mozilla47
Unspecified
OpenBSD
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Updated

3 years ago
Blocks: 1230117
(Assignee)

Comment 1

3 years ago
Testing a tentative patch for OpenBSD to add the missing bits to nspr's moz.build and plumbing... would have be nice to cc the bsd maintainers before landing this, it's not like we're not around to complain and fix your breakages :)
(Assignee)

Comment 3

3 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> Sorry, I did ask on dev-platform:
> https://lists.mozilla.org/pipermail/dev-platform/2016-January/013094.html

Sorry, but i dont think you can't expect every contributor (especially for us when it's not the 'main' project we contribute to) to be subscribed to every mailing list...

I have something ugly that manages to build nspr libs from moz.build, but they're installed in $objdir/dist/bin, and linking nss fails because it tries to look them up in $objdir/dist/lib which doesnt exist...

I've added nspr4/plc4/plds4 to USE_LIBS in config/external/nss/moz.build but otherwise i dont see how nss is supposed to find nspr. Any hint ?
(In reply to Landry Breuil (:gaston) from comment #3)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> > Sorry, I did ask on dev-platform:
> > https://lists.mozilla.org/pipermail/dev-platform/2016-January/013094.html
> 
> Sorry, but i dont think you can't expect every contributor (especially for
> us when it's not the 'main' project we contribute to) to be subscribed to
> every mailing list...

Yeah, understood. I knew this patch would be a problem for tier-3 systems, sorry you didn't get any advance notice.

> I have something ugly that manages to build nspr libs from moz.build, but
> they're installed in $objdir/dist/bin, and linking nss fails because it
> tries to look them up in $objdir/dist/lib which doesnt exist...

Try rebasing on top of bug 1243349, that was a problem on Linux as well without a system nspr-dev installed.
(Assignee)

Comment 5

3 years ago
Created attachment 8713679 [details] [diff] [review]
Fix build on OpenBSD with bundled nspr

Okay, with this on top of bug 1243349 i can build m-c again on OpenBSD, and it should be more or less straightforward to adapt it for FreeBSD/NetBSD/Dragonfly, but:

* i had to remove #define _MD_INTERVAL_USE_GTOD from nsprpub/pr/include/md/_openbsd.h otherwise nspr build was spouting tons of warnings about MD_INTERVAL* macros being redefined in http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/md/_unixos.h#297 - i suppose there's a better fix
* i had to include <pthread_np.h> for OpenBSD/FreeBSD to reach pthread_set_name_np (undef function/didnt link otherwise) but i have no idea how this built before, and this is a change in nspr so i suppose it would have to go to nspr repo first, or figure out what made it build before
* there's maybe more defines to set in moz.build, only took the ones i thought relevant from nsprpub/configure.in

Jan, Martin, can you try to complete my patch with your corresponding chunks ?
Assignee: nobody → landry
Attachment #8713679 - Flags: feedback?(ted)
(Assignee)

Updated

3 years ago
Attachment #8713679 - Flags: feedback?(mh+mozilla)
Comment on attachment 8713679 [details] [diff] [review]
Fix build on OpenBSD with bundled nspr

Review of attachment 8713679 [details] [diff] [review]:
-----------------------------------------------------------------

The moz.build changes are fine. The other changes will have to at least be landed in parallel in the nspr repo, but that's not a big deal.

Finding all the required preprocessor defines in NSPR for other platforms was...tricky. They're scattered through configure and various Makefiles.

::: nsprpub/pr/include/md/_openbsd.h
@@ -191,5 @@
>  #endif /* ! _PR_PTHREADS */
>  
>  #define _MD_EARLY_INIT                  _MD_EarlyInit
>  #define _MD_FINAL_INIT			_PR_UnixInit
> -#define _MD_INTERVAL_USE_GTOD

There's something like this on Linux too that I didn't bother to fix, I think HAVE_CLOCK_MONOTONIC? I think it'd be safer to wrap this in an #ifndef ...

::: nsprpub/pr/src/pthreads/ptthread.c
@@ +17,5 @@
>  
>  #include <pthread.h>
> +#if defined(OPENBSD) || defined(FREEBSD)
> +#include <pthread_np.h>
> +#endif

It would be nicer to figure out how the original code actually worked! I had some issues with this on other platforms, I wound up examining the compiler invocations and even diffing the preprocessor output in one instance.
Attachment #8713679 - Flags: feedback?(ted) → feedback+

Comment 7

3 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> ::: nsprpub/pr/src/pthreads/ptthread.c
> @@ +17,5 @@
> >  
> >  #include <pthread.h>
> > +#if defined(OPENBSD) || defined(FREEBSD)
> > +#include <pthread_np.h>
> > +#endif
> 
> It would be nicer to figure out how the original code actually worked! I had
> some issues with this on other platforms, I wound up examining the compiler
> invocations and even diffing the preprocessor output in one instance.

Bug 782111 has the proper fix. My guess, no one cared about a warning on Tier3 platform.

Updated

3 years ago
Depends on: 1244320

Comment 8

3 years ago
Created attachment 8713878 [details] [diff] [review]
v2

With NSPR patches out of the way this should be ready for review. Bitrig and DragonFly are currently not supported by NSPR itself, so configs are based on devel/npsr in their ports.

GNU/kFreeBSD is left for :glandium to fix in a separate bug. It'd probably use nsprpub/pr/include/md/_linux.{cfg,h}
Attachment #8713679 - Attachment is obsolete: true
Attachment #8713679 - Flags: feedback?(mh+mozilla)
Attachment #8713878 - Flags: review?(ted)
Attachment #8713878 - Flags: feedback?(martin)
Attachment #8713878 - Flags: feedback?(landry)

Updated

3 years ago
Depends on: 782111

Comment 10

3 years ago
Created attachment 8713881 [details] [diff] [review]
v2.1

> +    SOURCES += ['/nsprpub/pr/src/md/unix/%s.c' % CONFIG['OS_TARGET'].lower()]
> +    if CONFIG['OS_TARGET'] == 'Bitrig':
> +        DEFINES['OPENBSD'] = True

This proved harder to fix than I initially thought. Scrapped.
Attachment #8713878 - Attachment is obsolete: true
Attachment #8713878 - Flags: review?(ted)
Attachment #8713878 - Flags: feedback?(martin)
Attachment #8713878 - Flags: feedback?(landry)
Attachment #8713881 - Flags: review?(ted)
Attachment #8713881 - Flags: feedback?(martin)
Attachment #8713881 - Flags: feedback?(landry)
(Assignee)

Comment 11

3 years ago
Comment on attachment 8713881 [details] [diff] [review]
v2.1

With this, https://bugzilla.mozilla.org/attachment.cgi?id=651629 and the removal of #define _MD_INTERVAL_USE_GTOD from nsprpub/pr/include/md/_openbsd.h to avoid #define redefinition warnings, i can build m-c again.
Attachment #8713881 - Flags: feedback?(landry) → feedback+

Updated

3 years ago
Attachment #8713881 - Flags: feedback?(martin) → feedback+
Comment on attachment 8713881 [details] [diff] [review]
v2.1

Review of attachment 8713881 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/external/nspr/pr/moz.build
@@ +34,5 @@
> +        HAVE_BSD_FLOCK=True,
> +        HAVE_SOCKLEN_T=True,
> +    )
> +    DEFINES[CONFIG['OS_TARGET'].upper()] = True
> +    SOURCES += ['/nsprpub/pr/src/md/unix/%s.c' % CONFIG['OS_TARGET'].lower()]

Clever. :)
Attachment #8713881 - Flags: review?(ted) → review+

Comment 15

3 years ago
Landry (assignee), what are we waiting for? This could land without waiting for blocking bugs which are already fixed, anyawy.
(Assignee)

Comment 16

3 years ago
(In reply to Jan Beich from comment #15)
> Landry (assignee), what are we waiting for? This could land without waiting
> for blocking bugs which are already fixed, anyawy.

No idea, i thought it was still pending review for some reason.
Keywords: checkin-needed

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ac8a04c4635
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.