511743 - Add way to apply NSS patches to source at build time

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 1

[(not currently active) Ted Mielczarek]

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.

Comment 2

[Benjamin Smedberg]

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

Comment 3

[(not currently active) Ted Mielczarek]

One downside there is that we will lose source links on crash-stats for all NSS libraries.

Comment 4

[cls]

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.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 6

[Benjamin Smedberg]

As far as I know the NSS build system does not use vpath, at least not the way the mozilla build system does.

Comment 7

[(not currently active) Ted Mielczarek]

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.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: first pass

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?

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

That definitely looks good enough.  Thanks!

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: cleaned up and commented

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 11

[Wan-Teh Chang]

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

Comment 12

[(not currently active) Ted Mielczarek]

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

Comment 13

[Wan-Teh Chang]

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.

Comment 14

[(not currently active) Ted Mielczarek]

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 15

[Benjamin Smedberg]

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

Comment 16

[Wan-Teh Chang]

Comment on attachment 398206 [details] [diff] [review]
cleaned up and commented

Good point.  $(topsrcdir)/security/manager (PSM) is not part of NSS.

Comment 17

[(not currently active) Ted Mielczarek]

I admit, I took the lazy way out there. I'll fix that before checkin.

Comment 18

[Wan-Teh Chang]

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

Comment 19

[(not currently active) Ted Mielczarek]

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.

Comment 20

[Wan-Teh Chang]

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.

Comment 21

[Wan-Teh Chang]

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?

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 24

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/9dc6434fccb5

vlad: did you want this on 1.9.2 as well?

Comment 25

[Wan-Teh Chang]

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.