1243493 - Bug 1230117 broke bsd builds with bundled nspr

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Landry Breuil (:gaston)]

I'm not even surprised...
http://buildbot.rhaalovely.net/builders/mozilla-central-amd64/builds/1622/steps/configure/logs/stdio
http://buildbot.rhaalovely.net/builders/mozilla-central-freebsd-amd64/builds/731/steps/configure/logs/stdio

Comment 1

[Landry Breuil (:gaston)]

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 :)

Comment 2

[(not currently active) Ted Mielczarek]

Sorry, I did ask on dev-platform:
https://lists.mozilla.org/pipermail/dev-platform/2016-January/013094.html

Happy to review a patch.

Comment 3

[Landry Breuil (:gaston)]

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

Comment 4

[(not currently active) Ted Mielczarek]

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

Comment 5

[Landry Breuil (:gaston)]

Attachment: 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 ?

Comment 6

[(not currently active) Ted Mielczarek]

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.

Comment 7

[Jan Beich]

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

Comment 8

[Jan Beich]

Attachment: 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}

Comment 9

[Jan Beich]

Comment on attachment 8713878 [details] [diff] [review]
v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bfa1cd1f81e

Comment 10

[Jan Beich]

Attachment: 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.

Comment 11

[Landry Breuil (:gaston)]

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.

Comment 12

[Landry Breuil (:gaston)]

ted, how should we proceed to get https://hg.mozilla.org/projects/nspr/rev/ed39f339dc1e, https://hg.mozilla.org/projects/nspr/rev/85f6c6480115 & https://hg.mozilla.org/projects/nspr/rev/091cfd3c8c13 in m-c ? Wait for the next nspr beta tag ?

Comment 13

[Landry Breuil (:gaston)]

(In reply to Landry Breuil (:gaston) from comment #12)
> ted, how should we proceed to get
> https://hg.mozilla.org/projects/nspr/rev/ed39f339dc1e,
> https://hg.mozilla.org/projects/nspr/rev/85f6c6480115 &
> https://hg.mozilla.org/projects/nspr/rev/091cfd3c8c13 in m-c ? Wait for the
> next nspr beta tag ?

was taken care of in https://bugzilla.mozilla.org/show_bug.cgi?id=1244062 / https://hg.mozilla.org/integration/mozilla-inbound/rev/44ab9df0cd78, thanks kaie!

Comment 14

[(not currently active) Ted Mielczarek]

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. :)

Comment 15

[Jan Beich]

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

Comment 16

[Landry Breuil (:gaston)]

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

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ac8a04c4635

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ac8a04c4635