Closed Bug 1230117 Opened 8 years ago Closed 8 years ago

Use moz.build files instead of NSPR's build system

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: ted, Assigned: ted)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

I've got a patch, it works on Linux/Mac so far.
glandium commented on IRC that removing the NSPR Makefiles wouldn't fly. I was thinking we'd do it in the client.py "update_nspr" script, but he also said we could just put these moz.build files under config/external/nspr, which isn't unreasonable. The only thing that makes slightly ugly is that we'd have to specify absolute topsrcdir paths for sources etc.
One good reason to keep the Makefiles around is so that it's still possible to compile NSPR from those sources for people on non-supported tiers for the moz.build version. Sure, we could tell them to clone the nspr repo, or get a nspr tarball, but that would sound quite ridiculous (and getting the right version is not necessarily easy).
We had that discussion on IRC at some point and it does make sense--we should use the moz.build files for our tier-1 platforms, but have a fallback to using NSPR's build system for non-tier-1 platforms. It should be significantly simpler than what we have now--just invoke configure and run `make` in nsprpub.
Depends on: 1237869
That mostly worked, but Asan builds were broken by a visibility issue. I'm rolling that into the patch in bug 1237869.
By "that" I meant "the try push from comment 9".
Comment on attachment 8707472 [details]
MozReview Request: bug 1230117 - get rid of TARGET_MD_ARCH. r?glandium

https://reviewboard.mozilla.org/r/30725/#review27835

::: config/external/nspr/_pr_bld.h:1
(Diff revision 1)
> +/* This file intentionally left blank. */

Why do these .h files exist? Is the question I was left to answer on my own, so a comment about the fact that they are needed, respectively, for plvrsion.c and prvrsion.c would be useful. (and one of them currently being empty is weird :) )

::: config/external/nspr/ds/moz.build:9
(Diff revision 1)
> +    DEFINES['_NSPR_BUILD_'] = True

This should be define independently of MOZ_FOLD_LIBS (true for all those moz.build files)

::: config/external/nspr/ds/moz.build:14
(Diff revision 1)
> +# We allow warnings for third-party code that can be updated from upstream.

Note, we may want to fix those warnings upstream :)

::: config/external/nspr/ds/moz.build:17
(Diff revision 1)
> +USE_LIBS += ['/config/external/nspr/pr/nspr4']

No need for the full path here again. Also, it would be better to put the USE_LIBS for the SharedLibrary case only (same applies to other moz.builds)

::: config/external/nspr/moz.build:22
(Diff revision 1)
> -        '/nsprpub/lib/ds/plds4',
> -        '/nsprpub/lib/libc/src/plc4',
> -        '/nsprpub/pr/src/nspr4',
> +            '/config/external/nspr/ds/plds4',
> +            '/config/external/nspr/libc/plc4',
> +            '/config/external/nspr/pr/nspr4',

The full paths were only required because they are third party and from moz.build processing perspective, they couldn't be found. They now can, so

USE_LIBS += [
  'nspr4',
  'plc4',
  'plds4',
]

would work (yay!).

::: config/external/nspr/pr/moz.build:18
(Diff revision 1)
> +    DEFINES.update({
> +        'LINUX': True,

note that

DEFINES.update(
  LINUX=True,
  ...
)

also is valid python, this may or may not be a better syntax for moz.build files in general, but we have the update({}) pattern everywhere, so we could leave that for a followup doing it everywhere.

::: config/external/nspr/pr/moz.build:21
(Diff revision 1)
> +        'HAVE_SYSCALL': True,
> +        'HAVE_SETPRIORITY': True,
> +        'HAVE_STRERROR': True,
> +        'HAVE_LCHOWN': True,

Those are traditionally gotten from AC_CHECK_FUNCS.

::: config/external/nspr/pr/moz.build:57
(Diff revision 1)
> +        'WIN95': True,
> +        'WINNT': False,

Please add a comment that yes, this is really what we want because of hysterical raisins.

::: config/external/nspr/pr/moz.build:65
(Diff revision 1)
> +

Please make either this moz.build emit an error() on unsupported platforms/architectures, or both configure|js/src/configure throw such an error (the former seems more desirable, avoids maintaining two lists), and make that error say to either add support for the platform to that moz.build or, just fall back to building nspr separately and build with a combination of --with-system-nspr/--with-nspr-prefix/--with-nspr-exec-prefix (this should be tested, but I think you only really need the two first ones, except if you `make install` nspr, in which case you should only need the first.

::: config/external/nspr/pr/moz.build:218
(Diff revision 1)
> +GENERATED_FILES += ['prcpucfg.h']
> +GENERATED_FILES['prcpucfg.h'].script = 'copy_file.py'
> +GENERATED_FILES['prcpucfg.h'].inputs = [
> +    '/nsprpub/pr/include/' + CONFIG['NSPR_MDCPUCFG']
> +]

Considering we don't need the NSPR_MDCPUCFG variable for anything else, I'd rather do the following: create prcpucfg.h in config/external/nspr, add it to EXPORTS.nspr, and make its content something like:

#ifdef XP_DARWIN
#include "md/_darwin.cfg"
#else
#ifdef XP_WIN
etc.

And there you go, without copy_file.py :)

::: config/external/nss/moz.build:23
(Diff revision 1)
> -        'static:/nsprpub/lib/ds/plds4%s' % suffix,
> -        'static:/nsprpub/lib/libc/src/plc4%s' % suffix,
> -        'static:/nsprpub/pr/src/nspr4%s' % suffix,
> +        '/config/external/nspr/ds/plds4',
> +        '/config/external/nspr/libc/plc4',
> +        '/config/external/nspr/pr/nspr4',

No full path.

::: configure.in:8802
(Diff revision 1)
> +TARGET_NSPR_MDCPUCFG=\"${NSPR_MDCPUCFG}\"

This variable is only used by the CppEclipse backend, which, presumably, won't need it anymore. I'm not even sure why it needs it anyways, they look like cargocult from ancient history.

As a matter of fact, the same can be said about HOST_NSPR_MDCPUCFG, which only remaining use got there from cargocult, and I removed other uses in bug 838165.
Attachment #8707472 - Flags: review?(mh+mozilla)
Reading the review comment, I have mozreview for not sending the comment in the order it was in mozreview...
s/have/hate/
https://reviewboard.mozilla.org/r/30725/#review27835

> The full paths were only required because they are third party and from moz.build processing perspective, they couldn't be found. They now can, so
> 
> USE_LIBS += [
>   'nspr4',
>   'plc4',
>   'plds4',
> ]
> 
> would work (yay!).

I totally missed that. That's great!

> Those are traditionally gotten from AC_CHECK_FUNCS.

Yeah, I don't know why I did it that way, they're AC_CHECK_FUNCS in NSPR's configure.

> Please make either this moz.build emit an error() on unsupported platforms/architectures, or both configure|js/src/configure throw such an error (the former seems more desirable, avoids maintaining two lists), and make that error say to either add support for the platform to that moz.build or, just fall back to building nspr separately and build with a combination of --with-system-nspr/--with-nspr-prefix/--with-nspr-exec-prefix (this should be tested, but I think you only really need the two first ones, except if you `make install` nspr, in which case you should only need the first.

I asked on dev.platform about non-Tier 1 builders. I'd like to know if they're commonly using --with-system-nspr in the first place before I spend the time making an in-tree-system-nspr build work. I think you're right that it won't be an incredible amount of work, but I'd still like to get an idea of whether it'd be used or not first.

> This variable is only used by the CppEclipse backend, which, presumably, won't need it anymore. I'm not even sure why it needs it anyways, they look like cargocult from ancient history.
> 
> As a matter of fact, the same can be said about HOST_NSPR_MDCPUCFG, which only remaining use got there from cargocult, and I removed other uses in bug 838165.

I totally missed that. Nice catch!
Comment on attachment 8707472 [details]
MozReview Request: bug 1230117 - get rid of TARGET_MD_ARCH. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30725/diff/1-2/
Attachment #8707472 - Flags: review?(mh+mozilla)
https://reviewboard.mozilla.org/r/30725/#review27873

::: config/external/nspr/pr/moz.build:65
(Diff revisions 1 - 2)
> +    error('Not a supported OS_TARGET for NSPR in moz.build: "%s". Use --with-system-nspr' % CONFIG['OS_TARGET'])

This makes test_filesystem_traversal_no_config fail, which is unfortunate.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #20)
> https://reviewboard.mozilla.org/r/30725/#review27873
> 
> ::: config/external/nspr/pr/moz.build:65
> (Diff revisions 1 - 2)
> > +    error('Not a supported OS_TARGET for NSPR in moz.build: "%s". Use --with-system-nspr' % CONFIG['OS_TARGET'])
> 
> This makes test_filesystem_traversal_no_config fail, which is unfortunate.

You could make the filesystem traversal mode set some AC_SUBST that you can then add a conditional on.
Attachment #8707472 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8707472 [details]
MozReview Request: bug 1230117 - get rid of TARGET_MD_ARCH. r?glandium

https://reviewboard.mozilla.org/r/30725/#review28093

::: configure.in
(Diff revision 2)
> -    HOST_NSPR_MDCPUCFG='"md/_winnt.cfg"'

Would you mind separating out the *_NSPR_MDCPUCFG change?

+ comment 21
Depends on: 1241272
https://reviewboard.mozilla.org/r/30725/#review27873

> This makes test_filesystem_traversal_no_config fail, which is unfortunate.

I spun this off to bug 1241272, I have a patch there.
Comment on attachment 8707472 [details]
MozReview Request: bug 1230117 - get rid of TARGET_MD_ARCH. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30725/diff/2-3/
Attachment #8707472 - Attachment description: MozReview Request: bug 1230117 - Stop using NSPR's configure. r?glandium → MozReview Request: bug 1230117 - get rid of TARGET_MD_ARCH. r?glandium
Comment on attachment 8710355 [details]
MozReview Request: bug 1230117 - Stop using NSPR's configure. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31725/diff/1-2/
Comment on attachment 8710356 [details]
MozReview Request: bug 1230117 - get rid of {HOST,TARGET}_NSPR_MDCPUCFG. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31727/diff/1-2/
Comment on attachment 8707472 [details]
MozReview Request: bug 1230117 - get rid of TARGET_MD_ARCH. r?glandium

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30725/diff/3-4/
Okay then. I split the {HOST,TARGET}_NSPR_MDCPUCFG changes to a separate changeset, and also removed TARGET_MD_ARCH while I was there since it was used in all of one moz.build file (too lazy to file another bug for it, so it's here).

There's only one substantive change in the main changeset--I had to tweak the defines I was using in prcpucfg.h because it also gets included from NSS, which doesn't define the same set of platform conditionals as our top-level build.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #31)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=17ebca232ba1

The Windows builds on this push are burning due to bug 1240993, so I re-pushed just Windows builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b7505743867
I apparently neglected to rebase when I did the try push in comment 32. Comment 33's push should work.
Attachment #8707472 - Flags: review+ → review?(mh+mozilla)
Comment on attachment 8710356 [details]
MozReview Request: bug 1230117 - get rid of {HOST,TARGET}_NSPR_MDCPUCFG. r?glandium

https://reviewboard.mozilla.org/r/31727/#review28807
Attachment #8710356 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8707472 [details]
MozReview Request: bug 1230117 - get rid of TARGET_MD_ARCH. r?glandium

This one is already r+ in mozreview.
Attachment #8707472 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8710355 [details]
MozReview Request: bug 1230117 - Stop using NSPR's configure. r?glandium

https://reviewboard.mozilla.org/r/31725/#review28809
Attachment #8710355 - Flags: review?(mh+mozilla) → review+
Target Milestone: mozilla46 → mozilla47
Thanks for doing this, Ted!
Depends on: 1243349
Depends on: 1243493
Blocks: 1266046
Blocks: 1272626
Blocks: 1350426
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.