bug 1237872 covers writing gyp files to build NSS, this bug covers using them in the Mozilla build system via moz.build.

With the patches in that try push I've got NSS building from gyp files on Linux as part of a Firefox build. It doesn't 100% match the configuration we currently build, I'm missing the DISABLE_LIBPKIX handling and a few other bits. Those should be straightforward to fix. I'll need to test on other platforms next, as well as figure out how to handle MOZ_FOLD_LIBS. glandium and I discussed that a bit on IRC, it's not entirely clear what the best way to go about that is. It sounds like "build just static libs in NSS and link them in moz.build" might be the best way, since it matches what we do now and leaves us room to go farther in the future. The only fiddly bit we'll need to do is to figure out how to properly link the NSS binaries against the folded library.

That last try push actually worked fairly well. It broke during packaging because my patches don't currently build the NSS programs, so it fails to run shlibsign. That should not be hard to fix.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c059a81dcdf This try push built successfully on Linux64, and ran tests. It failed an xpcshell test that was testing TLS 1.3 support, which is off-by-default in NSS, so I fixed that. It failed to build on Linux32 because one of the NSS programs currently gets linked with the system zlib, and I missed that so the Linux64 build was accidentally working because it was invoking `pkg-config --libs zlib`, but that didn't work for the Linux32 build. I fixed that as well, so we'll see how the new try push does.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ed45617b06d This try run looks pretty good for Linux. I didn't triage all the test failures, but I spot-checked a few and there doesn't seem to be anything security-specific about them, and they all seem to have suggestions for intermittent bugs.

I got everything building in the MOZ_FOLD_LIBS case on OS X locally, that try push is to see how it goes there.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf8326575cd This failed on OS X because I didn't make the Python script that looks for the NSPR include dir resilient to the case where pkg-config is not installed. I fixed that. It also failed on Linux but just in one of the Python unit tests I added for the SYMBOLS_FILE patch (I made a change to it but forgot to update the test). There's one other failure I haven't fixed yet on Linux static analysis builds--one of the files in lib/freebl needs -fno-integrated-as when building with clang and I missed that.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #19) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=b23ccb4431c0 The OS X debug build was green here (hooray!) but the opt build is broken, looks like a 32-bit specific build failure. I thought I built a 32-bit NSS locally with gyp+ninja, but maybe I didn't (or maybe it's only broken in the Firefox build).

Another 32-bit build failure there, apparently there's an assembly file that gets built on 32-bit Mac but the Mozilla build system doesn't use DEFINES when building ASFILES, and it expects that to work...

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #23) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e0ea2df8aa0 This has green opt and debug OS X builds.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #27) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3dc7b7ed2ea The x86 builds were red here because we're not passing -safeseh to the assembler for a few assembly files. I have a fix for that locally, so I think this should be just about done.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #29) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dc4cbe71997 A few things are still busted in this build: 1) Linux/clang builds are broken, I need to pass -fno-integrated-as for some source files in lib/freebl. That should be a simple fix. 2) Android builds are broken like bug 1092004, turns out I'm not passing the hacky force include of android_stub.h to the NSS build: https://dxr.mozilla.org/mozilla-central/rev/ea104eeb14cc54da9a06c3766da63f73117723a0/config/external/nss/Makefile.in#221. This should also be a simple fix. 3) The Linux x64 ccov build is broken. It looks like the build system is racing to link libnspr4 and libnssutil3, and the latter is being linked first: 11:16:10 INFO - libnssutil3.so <...> 11:16:16 INFO - ../../../../config/nsinstall -R -m 644 'libnspr4.so' '../../../../dist/bin' I guess this is because I'm just passing '-lnspr4 ...' as `nspr_libs` down to the NSS build, so it's putting those in `OS_LIBS` instead of `USE_LIBS`. Apparently I left myself a note for why I didn't do that: https://hg.mozilla.org/try/file/3dc4cbe71997/security/moz.build#l44 I'll have to see if I can work around that, it's a race condition.

Oh, also, the Windows taskcluster builds seem completely broken, in that they're timing out without producing a log, so that's unfortunate.

The taskcluster builds timing out is some weird failure mode, I filed bug 1307803 on that. The root cause is that they're failing in backend generation with: 13:12:49 INFO - mozbuild.frontend.reader.BuildReaderError: 13:12:49 INFO - ============================== 13:12:49 INFO - ERROR PROCESSING MOZBUILD FILE 13:12:49 INFO - ============================== 13:12:49 INFO - The error occurred while processing the following file: 13:12:49 INFO - z:/task_1475671472/build/src/security/moz.build 13:12:49 INFO - The error appears to be part of the mozbuild.frontend.reader Python module itself! It is possible you have stumbled across a legitimate bug. 13:12:49 INFO - Traceback (most recent call last): 13:12:49 INFO - File "z:\task_1475671472\build\src\python\mozbuild\mozbuild\frontend\reader.py", line 1062, in read_mozbuild 13:12:49 INFO - metadata=metadata): 13:12:49 INFO - File "z:\task_1475671472\build\src\python\mozbuild\mozbuild\frontend\reader.py", line 1172, in _read_mozbuild 13:12:49 INFO - non_unified_sources = non_unified_sources): 13:12:49 INFO - File "z:\task_1475671472\build\src\python\mozbuild\mozbuild\frontend\gyp_reader.py", line 275, in read_from_gyp 13:12:49 INFO - msvs_settings = gyp.msvs_emulation.MsvsSettings(spec, {}) 13:12:49 INFO - File "z:\task_1475671472\build\src\media\webrtc\trunk\tools\gyp\pylib\gyp\msvs_emulation.py", line 193, in __init__ 13:12:49 INFO - self.vs_version = GetVSVersion(generator_flags) 13:12:49 INFO - File "z:\task_1475671472\build\src\media\webrtc\trunk\tools\gyp\pylib\gyp\msvs_emulation.py", line 934, in GetVSVersion 13:12:49 INFO - allow_fallback=False) 13:12:49 INFO - File "z:\task_1475671472\build\src\media\webrtc\trunk\tools\gyp\pylib\gyp\MSVSVersion.py", line 447, in SelectVisualStudioVersion 13:12:49 INFO - raise ValueError('Could not locate Visual Studio installation.') 13:12:49 INFO - ValueError: Could not locate Visual Studio installation. ...because the Taskcluster Windows workers don't have Visual Studio installed and some gyp code is not coping with that.

I've got fixes for #1 and #2 in hand, and am working on the Windows taskcluster issue.

I tested on a VM without VS installed and I believe I've fixed the Windows taskcluster issue. I also have a fix that should fix #3. All of those changes are in that try push.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #34) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf530b7e25db The Linux/clang jobs were still broken with my attempted fix--turns out that even though we build assembly files with clang in that configuration, we use ASFLAGS, not CFLAGS, so I have to pass -no-integrated-as in ASFLAGS which is just really weird.

OK, since I have green builds on Try this is ready to start review. I've split this into three patches: 1) The changes to gyp_reader to handle more cases and allow disabling the chromium special-casing. 2) The patch from bug 1237872, imported into m-c using `client.py update_nss`. You don't need to review this, but obviously it's needed for the build and it's probably useful to have for reference during review. For landing these patches we'll have to wait until it lands upstream in NSS then tag a beta and land that. Since the gyp build is independent of the Makefile build that can happen before the other patches in this bug land. 3) The build system changes to build NSS from the gyp build. I moved the moz.build file from config/external/nss to security/, mostly because `GYP_DIRS` didn't seem to handle non-relative paths well, but it seems like a sensible place to put it. The giant horrible Makefile.in file gets removed entirely (yay).

Comment on attachment 8798928 [details] bug 1295937 - Improvements to gyp_reader to handle NSS gyp files. https://reviewboard.mozilla.org/r/84276/#review84348 Preliminary review. I still need to do a pass with a look at the nss gyp files. ::: build/gyp_base.mozbuild:36 (Diff revision 1) > +arches = { > + 'x86_64': 'x64', > + 'x86': 'ia32', > +} > + > +gyp_vars['host_arch'] = arches.get(CONFIG['host_cpu'], CONFIG['host_cpu']) host_cpu comes from old-configure. Please add a set_config to build/moz.configure/init.configure for host.cpu instead of using that. ::: python/mozbuild/mozbuild/frontend/gyp_reader.py:53 (Diff revision 1) > + > + > # Default variables gyp uses when evaluating gyp files. > generator_default_variables = { > } > for dirname in ['INTERMEDIATE_DIR', 'SHARED_INTERMEDIATE_DIR', 'PRODUCT_DIR', you might as well use b'' literals here. ::: python/mozbuild/mozbuild/frontend/gyp_reader.py:188 (Diff revision 1) > + if is_win and config.substs['CPU_ARCH'] == 'x86_64' and 'Debug_x64' in spec['configurations'] and 'Release_x64' in spec['configurations']: > + c = 'Debug_x64' if config.substs['MOZ_DEBUG'] else 'Release_x64' I haven't looked at the nss gyp files yet, but this potentially smells bad for non x86-64 64-bits platforms.

Comment on attachment 8798930 [details] bug 1295937 - build NSS using gyp files. https://reviewboard.mozilla.org/r/84280/#review84350 Also need another pass with a look at the nss gyp files. ::: security/moz.build:74 (Diff revision 1) > + # bug 1307411 - Code in lib/freebl/mpi breaks with -Zc:inline. > + sandbox_vars['CFLAGS'] = ['-Zc:inline-'] That seems overly broad. ::: security/nss.symbols:85 (Diff revision 1) > CERT_EncodeAndAddBitStrExtension > CERT_EncodeAuthKeyID > CERT_EncodeBasicConstraintValue > CERT_EncodeCertPoliciesExtension > CERT_EncodeCRLDistributionPoints > +CERT_EncodeGeneralName That seems unrelated. ::: security/nss.symbols:355 (Diff revision 1) > PK11_GenerateRandomOnSlot > PK11_GetAllSlotsForCert > PK11_GetAllTokens > PK11_GetBestSlot > PK11_GetBestSlotMultiple > +PK11_GetBestSlotWithAttributes Likewise ::: old-configure.in:2174 (Diff revision 1) > fi > > if test -n "$MOZ_SYSTEM_NSS"; then > NSS_LIBS="$NSS_LIBS -lcrmf" > else > - NSS_CFLAGS="-I${DIST}/include/nss" > + NSS_CFLAGS="-I${DIST}/include/nss/public" Does this mean private headers are exported too? If so, that kind of sucks for standalone NSS. ::: security/generate_mapfile.py:9 (Diff revision 1) > +import subprocess > + > +def main(output, input): > + # linux > + if buildconfig.substs['OS_TARGET'] in ('Linux', 'Android'): > + # Could rewrite this as pure Python. Please do ::: security/generate_mapfile.py:11 (Diff revision 1) > + elif buildconfig.substs['OS_TARGET'] == 'Darwin': > + # same here. > + output.write(subprocess.check_output('grep -v ";+" "%s" | grep -v ";-" | sed -e "s; DATA ;;" -e "s,;;,," -e "s,;.*,," -e "s,^,_,"' % input, shell=True)) We shouldn't care about darwin and android here, only Linux (where we don't fold the libraries)

Comment on attachment 8798930 [details] bug 1295937 - build NSS using gyp files. https://reviewboard.mozilla.org/r/84280/#review84350 I pushed the gyp file patch to the NSS repo, so you can just pull that to look at them now. > That seems overly broad. NSS doesn't currently build with `-Zc:inline-`, so this is just keeping things that way. If we get the few broken things fixed in NSS we should be able to remove this. > That seems unrelated. I was getting unresolved symbol errors when linking without adding these symbols. I can do a try push without this hunk to refresh my memory. > Does this mean private headers are exported too? If so, that kind of sucks for standalone NSS. No, the gyp build system correctly exports things to public/private, like: https://hg.mozilla.org/projects/nss/file/faab2bb21f8e/lib/nss/exports.gyp We have just been overriding the `PUBLIC_EXPORT_DIR` in our Makefile to be `$(DIST)/include/nss`: https://dxr.mozilla.org/mozilla-central/rev/7452437b3ab571b1d60aed4e973d82a1471f72b2/config/external/nss/Makefile.in#261 By default the NSS build system wants to export its headers to `$(DIST)/nss/{public,private}`, so I'm letting it do that (but with `$(DIST)` == `$(DIST)/include`). The code that handles that is `handle_copies` in gyp_reader.py in the other patch. > Please do I had enough other stuff to deal with to get to this point that I was OK not doing that at the moment. :) It shouldn't be very hard. > We shouldn't care about darwin and android here, only Linux (where we don't fold the libraries) I don't think this is true--we're still linking some NSS shared libraries like freebl, and they have mapfiles.

Comment on attachment 8798928 [details] bug 1295937 - Improvements to gyp_reader to handle NSS gyp files. https://reviewboard.mozilla.org/r/84276/#review84348 > I haven't looked at the nss gyp files yet, but this potentially smells bad for non x86-64 64-bits platforms. I have made no attempt to support non-Tier 1 platforms currently, but this little bit of weirdness is to work around this annoying bit of code: https://chromium.googlesource.com/external/gyp/+/920ee58c3d3109dea3cd37d88054014891a93db7/pylib/gyp/msvs_emulation.py#304 The ninja backend uses msvs_emulation, and it will always try to use an `_x64` target for building Win64. I think it's to support building projects with both Win32+Win64 targets, but it's a frustrating choice.

Ted, you mentioned that there were things changing in NSS repo, do you need to make changes to the patch here or can we carry on with the review

They should be fairly minimal changes, I will try building with tip of NSS tomorrow.

I had to patch NSS to fix a few things that broke in the interim, those bugs are blocking this one. I rebased my patches on top of NSS tip + those patches, and can build successfully on Linux again. Once I test OSX/Windows locally again I'll fix up the few things that were mentioned in review comments here and get new patches up.

Comment on attachment 8798928 [details] bug 1295937 - Improvements to gyp_reader to handle NSS gyp files. Resetting until next iteration.

Comment on attachment 8798928 [details] bug 1295937 - Improvements to gyp_reader to handle NSS gyp files. https://reviewboard.mozilla.org/r/84276/#review84348 > I have made no attempt to support non-Tier 1 platforms currently, but this little bit of weirdness is to work around this annoying bit of code: > https://chromium.googlesource.com/external/gyp/+/920ee58c3d3109dea3cd37d88054014891a93db7/pylib/gyp/msvs_emulation.py#304 > > The ninja backend uses msvs_emulation, and it will always try to use an `_x64` target for building Win64. I think it's to support building projects with both Win32+Win64 targets, but it's a frustrating choice. My patch on bug 1315231 removes the need for this.

Comment on attachment 8798930 [details] bug 1295937 - build NSS using gyp files. https://reviewboard.mozilla.org/r/84280/#review84350 > NSS doesn't currently build with `-Zc:inline-`, so this is just keeping things that way. If we get the few broken things fixed in NSS we should be able to remove this. This got fixed upstream in bug 1311175, so I can remove this. > I was getting unresolved symbol errors when linking without adding these symbols. I can do a try push without this hunk to refresh my memory. This try push reverts these two hunks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d6fedc4bc3d > I don't think this is true--we're still linking some NSS shared libraries like freebl, and they have mapfiles. Yeah, we still wind up using this for the NSS shared libs we build even on platforms where we fold libraries.

Comment on attachment 8798930 [details] bug 1295937 - build NSS using gyp files. https://reviewboard.mozilla.org/r/84280/#review84350 > This try push reverts these two hunks: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d6fedc4bc3d Things are pretty broken without these changes. I assume it's some difference in how we're linking things. I don't really want to spend more time chasing that down to avoid adding two exports here.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #53) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d6fedc4bc3d This try push was to test removing the changes to nss.exports mentioned above, which is why it's broken.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #59) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c77c30105837 This try push ought to be green (fingers crossed).

Comment on attachment 8798928 [details] bug 1295937 - Improvements to gyp_reader to handle NSS gyp files. https://reviewboard.mozilla.org/r/84276/#review91838

Comment on attachment 8798930 [details] bug 1295937 - build NSS using gyp files. https://reviewboard.mozilla.org/r/84280/#review91846 ::: build/gyp_base.mozbuild:34 (Diff revision 2) > gyp_vars['OS'] = flavors.get(os) > > arches = { > 'x86_64': 'x64', > 'x86': 'ia32', > + 'aarch64': 'arm64', This change should be in the first patch. ::: security/moz.build:58 (Diff revision 2) > + gyp_vars['sqlite_libs'] = CONFIG['SQLITE_LIBS'] > + else: > + gyp_vars['sqlite_libs'] = 'sqlite' There's a "dummy" sqlite library defined in config/external/sqlite/moz.build when using MOZ_SYSTEM_SQLITE that just pulls SQLITE_LIBS, shouldn't using 'sqlite' work in that case too? ::: security/moz.build:62 (Diff revision 2) > + if CONFIG['MOZ_SYSTEM_SQLITE']: > + gyp_vars['sqlite_libs'] = CONFIG['SQLITE_LIBS'] > + else: > + gyp_vars['sqlite_libs'] = 'sqlite' > + # pkg-config won't reliably find this, so just force it. > + gyp_vars['zlib_libs'] = '-lz' Please put that together with ssl_enable_zlib and add a comment that zlib_libs is only used for modutil and signtool, and not libraries (because if that was to happen, they should be using mozz instead) ::: security/moz.build:64 (Diff revision 2) > + else: > + gyp_vars['sqlite_libs'] = 'sqlite' > + # pkg-config won't reliably find this, so just force it. > + gyp_vars['zlib_libs'] = '-lz' > + #XXX: this won't work with system NSPR > + gyp_vars['nspr_include_dir'] = CONFIG['NSPR_INCLUDE_DIR'] why not use NSPR_CFLAGS? Presumably that would cover system and in-tree nspr (beware it's a list). ::: security/moz.build:74 (Diff revision 2) > + GYP_DIRS['nss'].variables = gyp_vars > + sandbox_vars = { > + 'NO_VISIBILITY_FLAGS': True, > -# XXX: We should fix these warnings. > + # XXX: We should fix these warnings. > -ALLOW_COMPILER_WARNINGS = True > + 'ALLOW_COMPILER_WARNINGS': True, > + 'NO_PGO': True, Mmmmm this is interesting. Back when I worked on the horrible Makefile.in we have to build NSS, I'm pretty sure I got it to do PGO. It's interesting that, in practice, there's no PGO there now. I guess that's related to the mess with CFLAGS being inherited in some cases and not others. ::: security/nss.symbols:85 (Diff revision 2) > CERT_EncodeAndAddBitStrExtension > CERT_EncodeAuthKeyID > CERT_EncodeBasicConstraintValue > CERT_EncodeCertPoliciesExtension > CERT_EncodeCRLDistributionPoints > +CERT_EncodeGeneralName Looking at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d6fedc4bc3d&selectedJob=30765839 it looks like this is due to the static-library part of nss becoming a fake library, and all the objects being linked when it used to be a real library and thus object files with no symbol used were not actually linked. Try forcing no fake library with NO_EXPAND_LIBS = True instead of adding the symbols. ::: old-configure.in:2130 (Diff revision 2) > fi > > if test -n "$MOZ_SYSTEM_NSS"; then > NSS_LIBS="$NSS_LIBS -lcrmf" > else > - NSS_CFLAGS="-I${DIST}/include/nss" > + NSS_CFLAGS="-I${DIST}/include/public/nss" The path to nss headers in dist/include should not change. That has an impact on the SDK. ::: security/generate_mapfile.py:12 (Diff revision 2) > +import buildconfig > +import subprocess > + > +def main(output, input): > + # linux > + if buildconfig.substs['OS_TARGET'] not in ('Linux', 'Android', 'Darwin'): It would be better to reject building in-tree nss on the platforms that match this at configure time, than letting them go on with the build and fail when running generate_mapfile. ::: security/generate_mapfile.py:15 (Diff revision 2) > +def main(output, input): > + # linux > + if buildconfig.substs['OS_TARGET'] not in ('Linux', 'Android', 'Darwin'): > + print "Error: unhandled OS_TARGET %s" % buildconfig.substs['OS_TARGET'] > + return 1 > + is_linux = buildconfig.substs['OS_TARGET'] in ('Linux', 'Android') OS_ARCH is Linux on Android too. ::: security/generate_mapfile.py:20 (Diff revision 2) > + is_linux = buildconfig.substs['OS_TARGET'] in ('Linux', 'Android') > + > + with open(input, 'rb') as f: > + for line in f: > + line = line.rstrip() > + # Remove all lines containing ';-' Please add a comment that this gymnastic is explained in each of the def files in the nss tree.

Speaking of the SDK, we should ensure the SDK is not modified by this change. Other than the includes, the files included in the SDK should not change either, and unfortunately, we don't produce a SDK by default before beta, so try builds don't produce one by default. Please do try builds before/after with MOZ_AUTOMATION_SDK set to 1.

Looking at the last release SDK, one obvious change that would happen is that libcrmf.a would not be there, since it doesn't exist as a library. NO_EXPAND_LIBS would fix that, although there's also a likely missing SDK_LIBRARY = True for things that gyp builds.

> Please put that together with ssl_enable_zlib and add a comment that zlib_libs is only used for modutil and signtool, and not libraries (because if that was to happen, they should be using mozz instead) Please also mention that ssl_enable_zlib disables using zlib_libs in the one library that does use it.

Comment on attachment 8798930 [details] bug 1295937 - build NSS using gyp files. https://reviewboard.mozilla.org/r/84280/#review91992 ::: old-configure.in:2130 (Diff revision 2) > fi > > if test -n "$MOZ_SYSTEM_NSS"; then > NSS_LIBS="$NSS_LIBS -lcrmf" > else > - NSS_CFLAGS="-I${DIST}/include/nss" > + NSS_CFLAGS="-I${DIST}/include/public/nss" I had to tweak the gyp files to make this work, but I have a patch in bug 1316604.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #67) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c35f90d85964 I missed a spot when rebasing to NSS tip.

Comment on attachment 8798930 [details] bug 1295937 - build NSS using gyp files. https://reviewboard.mozilla.org/r/84280/#review91846 > why not use NSPR_CFLAGS? Presumably that would cover system and in-tree nspr (beware it's a list). Because this variable wants an actual path, not a set of CFLAGS. I struggled with this a bit--it's hard to write something for gyp that works across platforms, this was the simplest way. I added a little glue to nspr-build.m4 to set `NSPR_INCLUDE_DIR` when we use system NSPR. It's not great but it ought to work. > Mmmmm this is interesting. Back when I worked on the horrible Makefile.in we have to build NSS, I'm pretty sure I got it to do PGO. It's interesting that, in practice, there's no PGO there now. I guess that's related to the mess with CFLAGS being inherited in some cases and not others. Yeah. I'm sure we could build it with PGO but I have enough things to worry about with these patches that I don't want to throw that into the mix. > Looking at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d6fedc4bc3d&selectedJob=30765839 it looks like this is due to the static-library part of nss becoming a fake library, and all the objects being linked when it used to be a real library and thus object files with no symbol used were not actually linked. > > Try forcing no fake library with NO_EXPAND_LIBS = True instead of adding the symbols. I had to put that into gyp_reader because it's only valid to set in a `STATIC_LIBRARY` context. I'll fold that into the other patch. > It would be better to reject building in-tree nss on the platforms that match this at configure time, than letting them go on with the build and fail when running generate_mapfile. I added a check in old-configure.in.

I landed all my outstanding NSS patches, the latest patches here build atop NSS tip: https://hg.mozilla.org/projects/nss/rev/c2b5a69382c4

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #70) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=3316f249377f All the Linux builds are broken here. I only built OS X locally, so I'll build Linux locally and see what's up. (Presumably it's a difference between MOZ_FOLD_LIBS and non-folded-libs.)

The Linux bustage is fallout from changing things to use `NO_EXPAND_LIBS = True`. This is kind of a nightmare.

Comment on attachment 8798930 [details] bug 1295937 - build NSS using gyp files. https://reviewboard.mozilla.org/r/84280/#review92960 ::: security/moz.build:58 (Diff revisions 2 - 3) > gyp_vars['mozilla_client'] = 1 > + # We run shlibsign as part of packaging, not build. > gyp_vars['sign_libs'] = 0 > gyp_vars['python'] = CONFIG['PYTHON'] > gyp_vars['nss_dist_dir'] = '$PRODUCT_DIR/dist' > + # NSS wants to put public headers in dist/include/public/nss by default, It puts them in dist/public, not dist/include/public. ::: security/moz.build:67 (Diff revisions 2 - 3) > gyp_vars['ssl_enable_zlib'] = 0 > - gyp_vars['use_system_sqlite'] = 1 > - if CONFIG['MOZ_SYSTEM_SQLITE']: > - gyp_vars['sqlite_libs'] = CONFIG['SQLITE_LIBS'] > - else: > - gyp_vars['sqlite_libs'] = 'sqlite' > # pkg-config won't reliably find this, so just force it. > + # System zlib is only used for modutil and signtool. > gyp_vars['zlib_libs'] = '-lz' You should make it clearer what the trick with ssl_enable_zlib is.

Comment on attachment 8798928 [details] bug 1295937 - Improvements to gyp_reader to handle NSS gyp files. https://reviewboard.mozilla.org/r/84276/#review92962 ::: python/mozbuild/mozbuild/frontend/gyp_reader.py:246 (Diff revisions 2 - 3) > if spec['type'] == 'shared_library': > context['FORCE_SHARED_LIB'] = True > + elif spec['type'] == 'static_library' and no_chromium: > + # NSS will fail to link if we use library descriptors instead > + # of static libraries. > + context['NO_EXPAND_LIBS'] = True Building NSS standalone uses real libraries, so it must work. The question is what is the difference that makes it break. OTOH, it doesn't /really/ matter, except for libcrmf. I'm sure your build errors would go away if you limited NO_EXPAND_LIBS to libcrmf. Interestingly, libcrmf is not installed in dist/ in a standalone NSS build (but it is with the old make build system)

OK, I just limited it to crmf and it builds on my mac. I added a little hacky bit in NSS to support that: https://hg.mozilla.org/projects/nss/rev/cda9e9031df4 bsmedberg says the SDK doesn't matter anymore, so I'm not going to worry about fixing that. I'll make sure I've addressed the rest of the review comments here and push an updated patch.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #79) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=89cd31997c13 This is pretty green.

I don't know if you got flagged for re-review, but the latest version of these patches should have addressed all your comments. I'd love to get this landed!

Comment on attachment 8798928 [details] bug 1295937 - Improvements to gyp_reader to handle NSS gyp files. https://reviewboard.mozilla.org/r/84276/#review94136 ::: python/mozbuild/mozbuild/frontend/gyp_reader.py:243 (Diff revision 4) > - context['LIBRARY_NAME'] = name.decode('utf-8') > + context['LIBRARY_NAME'] = name.decode('utf-8') > + else: > + context['PROGRAM'] = name.decode('utf-8') > + if spec['type'] == 'shared_library': > + context['FORCE_SHARED_LIB'] = True > + elif spec['type'] == 'static_library' and spec.get('variables', {}).get('no_expand_libs', '0') == '1': In the long term, I think we should have a way to do that kind of thing from moz.build instead of having to change gyp files from third parties.

Comment on attachment 8798930 [details] bug 1295937 - build NSS using gyp files. https://reviewboard.mozilla.org/r/84280/#review94140 ::: security/moz.build:59 (Diff revision 4) > + # NSS wants to put public headers in $nss_dist_dir/public/nss by default, > + # which would wind up being mapped to dist/include/public/nss. > + # This forces it to put them in dist/include/nss. > + gyp_vars['nss_public_dist_dir'] = '$PRODUCT_DIR/dist' looking at nss_dist_dir and nss_public_dist_dir in the nss repository, it looks like this should be: # NSS wants to put public headers in $nss_dist_dir/public/nss by default, # which would wind up being mapped to dist/public/nss. # This forces it to put them in dist/include/nss. gyp_vars['nss_public_dist_dir'] = '$PRODUCT_DIR/dist/include'

Comment on attachment 8798930 [details] bug 1295937 - build NSS using gyp files. https://reviewboard.mozilla.org/r/84280/#review94140 > looking at nss_dist_dir and nss_public_dist_dir in the nss repository, it looks like this should be: > > # NSS wants to put public headers in $nss_dist_dir/public/nss by default, > # which would wind up being mapped to dist/public/nss. > # This forces it to put them in dist/include/nss. > gyp_vars['nss_public_dist_dir'] = '$PRODUCT_DIR/dist/include' No, because the logic in gyp_reader.py's handle_copies special-cases copies to `$PRODUCT_DIR/dist` and treats them as `EXPORTS`. I can try to clarify the comment again.

Comment on attachment 8798928 [details] bug 1295937 - Improvements to gyp_reader to handle NSS gyp files. https://reviewboard.mozilla.org/r/84276/#review94136 > In the long term, I think we should have a way to do that kind of thing from moz.build instead of having to change gyp files from third parties. It's kind of an edge case, really. If it comes up again we can figure out a nicer way to handle it.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #87) > Comment on attachment 8798928 [details] > bug 1295937 - Improvements to gyp_reader to handle NSS gyp files. > > https://reviewboard.mozilla.org/r/84276/#review94136 > > > In the long term, I think we should have a way to do that kind of thing from moz.build instead of having to change gyp files from third parties. > > It's kind of an edge case, really. If it comes up again we can figure out a > nicer way to handle it. We actually have plenty of hacks around in webrtc gyp files and therefore maintaining patches against them. (In reply to Ted Mielczarek [:ted.mielczarek] from comment #86) > Comment on attachment 8798930 [details] > bug 1295937 - build NSS using gyp files. > > https://reviewboard.mozilla.org/r/84280/#review94140 > > > looking at nss_dist_dir and nss_public_dist_dir in the nss repository, it looks like this should be: > > > > # NSS wants to put public headers in $nss_dist_dir/public/nss by default, > > # which would wind up being mapped to dist/public/nss. > > # This forces it to put them in dist/include/nss. > > gyp_vars['nss_public_dist_dir'] = '$PRODUCT_DIR/dist/include' > > No, because the logic in gyp_reader.py's handle_copies special-cases copies > to `$PRODUCT_DIR/dist` and treats them as `EXPORTS`. I can try to clarify > the comment again. Aah. This would be much less confusing if it used EXPORTS for dist/include/** instead of dist/**.

(In reply to Mike Hommey [:glandium] from comment #88) > We actually have plenty of hacks around in webrtc gyp files and therefore > maintaining patches against them. For this issue specifically? If you can point me at one I can take a look. > Aah. This would be much less confusing if it used EXPORTS for > dist/include/** instead of dist/**. It's sort of arbitrary--I just wanted to handle copies as EXPORTS, but wanted to put a little sanity check in to make sure it wasn't doing something unexpected. I had originally written the NSS gyp files to copy to `<(PRODUCT_DIR)/dist`, but it got changed over a few iterations. There's no clear correct answer here--we're making up values for the `PRODUCT_DIR` and `nss_dist_include_dir` gyp variables anyway, so the strings we get out start with whatever we put in.

https://hg.mozilla.org/integration/mozilla-inbound/rev/165a160a0f689ab1e0143376d7ee8ce07c300fb6 bug 1295937 - Improvements to gyp_reader to handle NSS gyp files. r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/ee70776759bf296c951a9805d5b79169291be4d3 bug 1295937 - build NSS using gyp files. r=glandium

https://hg.mozilla.org/mozilla-central/rev/165a160a0f68 https://hg.mozilla.org/mozilla-central/rev/ee70776759bf

https://hg.mozilla.org/mozilla-central/rev/165a160a0f68 https://hg.mozilla.org/mozilla-central/rev/ee70776759bf