Closed Bug 1243493 Opened 5 years ago Closed 5 years ago

Bug 1230117 broke bsd builds with bundled nspr

Categories

(Firefox Build System :: General, defect)

Unspecified
OpenBSD
defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: gaston, Assigned: gaston)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Blocks: 1230117
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 :)
(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.
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)
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+
(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.
Depends on: 1244320
Attached patch v2 (obsolete) — Splinter Review
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)
Depends on: 782111
Attached patch v2.1Splinter Review
> +    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)
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+
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+
Landry (assignee), what are we waiting for? This could land without waiting for blocking bugs which are already fixed, anyawy.
(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
https://hg.mozilla.org/mozilla-central/rev/2ac8a04c4635
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.