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)
Firefox Build System
General
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.
Assignee | ||
Comment 1•8 years ago
|
||
Patch: https://hg.mozilla.org/try/rev/abfa1bb430a9 Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7fedb9062ec
Assignee | ||
Comment 2•8 years ago
|
||
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.
Comment 3•8 years ago
|
||
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).
Assignee | ||
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c894981ed34e
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09de55f874d8
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=07fb6c729ee2
Assignee | ||
Comment 8•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c104024e3a0
Assignee | ||
Comment 9•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5970ce1601e
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7fa7070108c
Assignee | ||
Comment 11•8 years ago
|
||
That mostly worked, but Asan builds were broken by a visibility issue. I'm rolling that into the patch in bug 1237869.
Assignee | ||
Comment 12•8 years ago
|
||
By "that" I meant "the try push from comment 9".
Assignee | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/30725/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30725/
Attachment #8707472 -
Flags: review?(mh+mozilla)
Comment 14•8 years ago
|
||
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)
Comment 15•8 years ago
|
||
Reading the review comment, I have mozreview for not sending the comment in the order it was in mozreview...
Comment 16•8 years ago
|
||
s/have/hate/
Assignee | ||
Comment 17•8 years ago
|
||
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!
Assignee | ||
Comment 18•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1129bc4589b2
Assignee | ||
Comment 19•8 years ago
|
||
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)
Assignee | ||
Comment 20•8 years ago
|
||
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.
Comment 21•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8707472 -
Flags: review?(mh+mozilla) → review+
Comment 22•8 years ago
|
||
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
Assignee | ||
Comment 23•8 years ago
|
||
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.
Assignee | ||
Comment 24•8 years ago
|
||
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
Assignee | ||
Comment 25•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31725/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31725/
Attachment #8710355 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 26•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31727/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31727/
Attachment #8710356 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 27•8 years ago
|
||
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/
Assignee | ||
Comment 28•8 years ago
|
||
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/
Assignee | ||
Comment 29•8 years ago
|
||
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/
Assignee | ||
Comment 30•8 years ago
|
||
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.
Assignee | ||
Comment 31•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17ebca232ba1
Assignee | ||
Comment 32•8 years ago
|
||
(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
Assignee | ||
Comment 33•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=abc0135dc3ed
Assignee | ||
Comment 34•8 years ago
|
||
I apparently neglected to rebase when I did the try push in comment 32. Comment 33's push should work.
Assignee | ||
Updated•8 years ago
|
Attachment #8707472 -
Flags: review+ → review?(mh+mozilla)
Comment 35•8 years ago
|
||
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 36•8 years ago
|
||
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 37•8 years ago
|
||
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+
Assignee | ||
Comment 38•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe759d940e9f68c7476f5ac384e563b7263f9207 bug 1230117 - Stop using NSPR's configure. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/7daaaffbb5b78138e2268a9be01811f4b3a47334 bug 1230117 - get rid of {HOST,TARGET}_NSPR_MDCPUCFG. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/99bdd3287bcf9ecf974c6f68ba3ba15e6fc17937 bug 1230117 - get rid of TARGET_MD_ARCH. r=glandium
Comment 39•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fe759d940e9f https://hg.mozilla.org/mozilla-central/rev/7daaaffbb5b7 https://hg.mozilla.org/mozilla-central/rev/99bdd3287bcf
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Updated•8 years ago
|
Target Milestone: mozilla46 → mozilla47
Comment 40•8 years ago
|
||
Thanks for doing this, Ted!
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•