Closed Bug 511743 Opened 11 years ago Closed 11 years ago
Add way to apply NSS patches to source at build time
We need a way to apply patches to the source tree at build time, preferably creating the source files in the objdir so as not to clobber source, and then build using those files. This is a very convoluted way to get around stupid FIPS requirements in NSS, where FIPS certification is given to source files that we then can't modify to fix bugs, even on platforms where we don't care about FIPS certification. The approach I'm thinking of here would be a nss/patches/foo directory and associated --with-build-time-patches=nss/patches/foo configure flag that would do the right thing. (That way we could reuse the same approach for other modules where we have similar problems.)
11 years ago
Depends on: 511808
I don't think trying to do this in a general way makes any sense, since NSS uses a separate build system from the rest of the tree. If we had problems in any other module (except perhaps NSPR) we would just patch the source in-tree. That being said, this is also probably going to be painfully hard to do (if possible), since NSS has a separate build system consisting only of Makefiles. Taking for investigation, no promises.
Assignee: nobody → ted.mielczarek
The easiest way would be to take the NSS sources, copy them somewhere else, patch them, and build from there instead of the normal srcdir
One downside there is that we will lose source links on crash-stats for all NSS libraries.
This is fairly easy to do. We did it for work awhile ago. Given that nss/ is generally locked down, you probably don't want to store the patches there. The generic rule would look like: apply_patches: @set -e ; \ pfiles="$(wildcard $(topsrcdir)/patches/$(MODULE)/*.patch)" ; \ if [ -n "$$pfiles" -a -x $(topsrcdir)/patches/apply-patch.sh ]; then \ $(topsrcdir)/patches/apply-patch.sh $(topsrcdir) $(topsrcdir)/patches/$(MODULE) \*.patch ; \ fi where apply-patch.sh is a script that knows to apply the patches to $(topsrcdir) using the given patchdir & regex. While I generally dislike modification of the srcdir, I don't think it's that bad in this case given that NSS is locked down so there will be no accidental commits..except, maybe in everyone's private hg repo but toplevel commits are evil anyway. The trick would be to find the correct place to plug the patching into the current build system. You could make it part of configure and just add a simple check to avoid attempting to apply a patch twice.
You should be able to put the modified/patched source files in the objdir, no? We use VPATH to find them in the srcdir, so if they exist in the objdir those will get picked up first. Making it part of configure sounds like a good plan.. could even have configure delete any of the files that might have patches applied from the objdir, if that bit works out.
As far as I know the NSS build system does not use vpath, at least not the way the mozilla build system does.
NSS uses absolute paths to source files when compiling, so I don't think there's any way we can convince it to compile from another location: http://mxr.mozilla.org/mozilla-central/source/security/coreconf/rules.mk#415 I think this limits us to one of two possibilities: 1) bsmedberg's suggestion from comment 2, copy the entire NSS tree to the objdir, patch, build from there. 2) cls' suggestion from comment 4, patch source files in-tree. This seems likely to cause people to accidentally commit these patches if they're not careful, and I'm not wild about writing to the source directory in any case.
This works, but it's not very pretty. It might be sufficient, though. This patch will let you set MOZ_NSS_PATCH in configure, confvars.sh, or a mozconfig (or the environment, I guess) to a path to a patch to apply to the NSS source. For example, with the right quoting you could say: MOZ_NSS_PATCH='$(topsrcdir)/path/to/nss.patch' It assumes that the patch applies to the top of the srcdir with -p1, which conveniently is the type of patch you get if you edit your repo and run "hg diff". The caveat here is that any patched files will get rebuilt on every dep build, since this is very dumb and just re-patches every time. Fixing that will really complicate this patch, since we'd have to try to get dependencies right between the patched files and the patch, and that will be complicated to express in make. vlad: what do you think, good enough?
That definitely looks good enough. Thanks!
Same as above, just removed unnecessary comments and added explanatory comments. This patch also includes some cleanup. This particular makefile doesn't get a lot of love. I removed the .nss.checkout / .nss.cleaned stuff since that hasn't actually done anything since we switched to Hg.
Comment on attachment 398206 [details] [diff] [review] cleaned up and commented >-ABS_DIST := $(shell cd $(DIST) && pwd) >+ABS_DIST := $(call core_abspath,$(DIST)) Did you copy the name "core_abspath" from NSS? Not that I mind. We chose the core_ prefix for "coreconf".
I think bsmedberg just copied it wholesale from NSPR or NSS in bug 483856. Probably didn't see any reason to change the name, since the obvious 'abspath' is a built-in GNU make function (but maybe only in 3.81?)
I suggest that core_abspath be renamed moz_abspath to avoid confusion. When I saw that in your patch, my first reaction was that you included some coreconf makefile by accident. Since NSS makefiles don't have header file dependencies, you may want to disallow the NSS patch to contain changes to .h files. You can check that either manually (as a checklist item before you check in a patch file) or in your makefile rule (before you apply the patch). The NSS patch should be the last resort (e.g., if you must support the Camellia TLS cipher suites on Windows CE), and it should be reviewed by the NSS team. Please always ask for an NSS CVS tag from the NSS team first.
We're already using core_abspath in a number of other places, I'm not sure why it matters. As for the patch itself, I have no opinion on that matter. I don't plan to enforce header file changes, the patch author should be aware of that. I'd like to keep the makefile machinery to enable this as simple as possible.
Comment on attachment 398206 [details] [diff] [review] cleaned up and commented >+ cp -Rp $(topsrcdir)/security $(NSS_SRCDIR) >+ cp -Rp $(topsrcdir)/dbm $(NSS_SRCDIR) Note that it might be more efficient to just copy the NSS dirs instead of security/*. From client.py: NSS_DIRS = ('dbm', 'security/nss', 'security/coreconf', 'security/dbm')
Attachment #398206 - Flags: review?(benjamin) → review+
Comment on attachment 398206 [details] [diff] [review] cleaned up and commented Good point. $(topsrcdir)/security/manager (PSM) is not part of NSS.
I admit, I took the lazy way out there. I'll fix that before checkin.
Ted: how will you specify MOZ_NSS_PATCH? As an environment variable when you run configure? Will MOZ_NSS_PATCH point to a patch that's checked in to the Mozilla source tree? We need to create a wiki page similar to https://developer.mozilla.org/en/Updating_NSPR_or_NSS_in_mozilla-central to document the procedure we should follow to use an NSS patch (such as trying to get an NSS CVS tag first and avoiding changes to .h files).
wtc: I'm leaving that deliberately open. It could be specified in configure, an application's confvars.sh, or in a mozconfig. Please direct your concerns to Vlad, I have merely provided the build machinery to make this possible.
I understand. My point is that it'd be irresponsible for us to provide build machinery that is overly flexible and makes it hard for someone who's not in the know to figure out how a build is constructed. Most Firefox issues involving NSS will be filed as NSS bugs by default. NSS developers, except me and rrelyea, don't know how to build Firefox. So whether an NSS patch is used, and the location of the patch must be easily discoverable by an "amateur" Firefox developer, e.g., by browsing the Firefox source tree using MXR.
For a concrete example, let's assume Fennec has a business requirement to support the Camellia TLS cipher suites. Vlad already asked the NSS team for an NSS CVS tag, but the NSS team refused to change any files that were just tested by the FIPS testing lab. So we need to apply the patch attachment 395784 [details] [diff] [review] in bug 508113 to NSS, but only when building for the WINCE target. How will we do this in conjunction with the patch in this bug? Will the NSS patch be checked in to a directory that's near NSS in the source tree, for example, security/nss-patch? Where will the MOZ_NSS_PATCH variable be specified? In security/manager/Makefile.in?
The patch itself would be checked in somewhere; security/nss-patches seems as good a place as any. MOZ_NSS_PATCH would be set in the mozconfig for the specific product, though; the official release versions of those are checked in, just not in mozilla-central.
(Note: I don't really expect this to get used a lot, but it's nice to have the mechanism available in case it's needed for specific platforms or during specific development periods.)
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/9dc6434fccb5 vlad: did you want this on 1.9.2 as well?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Ted: not so fast. I still need to resolve an issue about this patch with Vlad. Vlad: why don't we specify MOZ_NSS_PATCH in security/manager/Makefile.in? That's where I (an NSS developer who knows how to build Firefox) would expect to find MOZ_NSS_PATCH specified. We should try to follow the principle of least astonishment here. security/manager/Makefile.in is the only file that uses MOZ_NSS_PATCH. If we specify MOZ_NSS_PATCH there, we don't need the changes to configure.in and config/autoconf.mk.in.
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.