1243872 - Clean up zlib-related fragments of NSS makefiles

Status: RESOLVED FIXED

(NSS :: Build, defect, P2)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Splitting this out of bug 1222665.  The makefile fragment that links libssl against zlib is duplicated, imperfectly, several times for tests; and bug 1222665 would have added yet another copy (and got r- for that).  So let's fix that.

This also means that overriding NSS_ENABLE_ZLIB, like the Firefox build does, will work as expected with NSS's own tests.  This isn't relevant to the Firefox build yet, but if there was interest in running NSS's tests as part of Firefox's tests, this would help.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Attachment: bug1243872-zlib-mk-hg0.diff

Comment 2

[Wan-Teh Chang]

Comment on attachment 8713336 [details] [diff] [review]
bug1243872-zlib-mk-hg0.diff

Review of attachment 8713336 [details] [diff] [review]:
-----------------------------------------------------------------

Jed: thank you for the patch. I like the consolidation of common code.
I suggest a change: only put the USE_SYSTEM_ZLIB code in the new
coreconf/zlib.mk file, and leave the NSS_ENABLE_ZLIB code in
lib/ssl/config.mk.

::: coreconf/zlib.mk
@@ +6,5 @@
> +# Configuration information for linking against zlib.
> +
> +ifdef NSS_ENABLE_ZLIB
> +
> +DEFINES += -DNSS_ENABLE_ZLIB

NSS_ENABLE_ZLIB (both the makefile variable and the C
preprocessor macro) is specific to lib/ssl and should
stay in lib/ssl/config.mk. It should not be added to
coreconf/zlib.mk and coreconf/config.mk.

@@ +14,5 @@
> +# (for example, -lz) in the platform's config file in coreconf.
> +ifdef USE_SYSTEM_ZLIB
> +OS_LIBS += $(ZLIB_LIBS)
> +else
> +ZLIB_LIBS = $(DIST)/lib/$(LIB_PREFIX)zlib.$(LIB_SUFFIX)

The following code in cmd/platlibs.mk should be removed:
 
262 # If a platform has a system zlib, set USE_SYSTEM_ZLIB to 1 and
263 # ZLIB_LIBS to the linker command-line arguments for the system zlib
264 # (for example, -lz) in the platform's config file in coreconf.
265 ifndef USE_SYSTEM_ZLIB
266 ZLIB_LIBS = $(DIST)/lib/$(LIB_PREFIX)zlib.$(LIB_SUFFIX)
267 endif

Comment 3

[Eric Rescorla (:ekr)]

Attachment: 0001-Bug-1243872-Refactor-zlib-support-in-Makefiles.patch

Comment 4

[Eric Rescorla (:ekr)]

Attachment: 0001-Bug-1243872-Refactor-zlib-support-in-Makefiles.patch

Comment 5

[Eric Rescorla (:ekr)]

MT, I revised this per WTC's comments.

WTC: needinfo for your visibility

Comment 6

[Martin Thomson [:mt:]]

Comment on attachment 8714185 [details] [diff] [review]
0001-Bug-1243872-Refactor-zlib-support-in-Makefiles.patch

Review of attachment 8714185 [details] [diff] [review]:
-----------------------------------------------------------------

EKR, if this is your patch: you need to add a little more context to your patches: `git config --global diff.context 8`

I think that I would have preferred to see this patch disable building of cmd/modutil and cmd/signtool if NSS_ENABLE_ZLIB was disabled.  That requires far fewer changes all told.  Then you could move the zlib configuration to config.mk instead of having to include yet another small .mk file.

BTW, how do you disable this?  make NSS_ENABLE_SSL_ZLIB=0 will still try to build with zlib.

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8714185 [details] [diff] [review]
0001-Bug-1243872-Refactor-zlib-support-in-Makefiles.patch

Review of attachment 8714185 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine.

Comment 8

[Wan-Teh Chang]

Comment on attachment 8714185 [details] [diff] [review]
0001-Bug-1243872-Refactor-zlib-support-in-Makefiles.patch

Review of attachment 8714185 [details] [diff] [review]:
-----------------------------------------------------------------

Eric: thank you for implementing what I outlined. I forgot
to point out that a piece of code that should also be removed
from cmd/platlibs.mk.

::: coreconf/config.mk
@@ +191,5 @@
> +
> +# Mozilla's mozilla/modules/zlib/src/zconf.h adds the MOZ_Z_ prefix to zlib
> +# exported symbols, which causes problem when NSS is built as part of Mozilla.
> +# So we add a NSS_ENABLE_SSL_ZLIB variable to allow Mozilla to turn this off.
> +NSS_ENABLE_SSL_ZLIB = 1

Just a reminder: you'll need to update Mozilla's
config/external/nss/Makefile.in when you push a new
NSS update to mozilla-central:

http://mxr.mozilla.org/mozilla-central/search?string=NSS_ENABLE_ZLIB

::: coreconf/zlib.mk
@@ +10,5 @@
> +# (for example, -lz) in the platform's config file in coreconf.
> +ifdef USE_SYSTEM_ZLIB
> +OS_LIBS += $(ZLIB_LIBS)
> +else
> +ZLIB_LIBS = $(DIST)/lib/$(LIB_PREFIX)zlib.$(LIB_SUFFIX)

IMPORTANT: please remove the following from cmd/platlibs.mk:

262 # If a platform has a system zlib, set USE_SYSTEM_ZLIB to 1 and
263 # ZLIB_LIBS to the linker command-line arguments for the system zlib
264 # (for example, -lz) in the platform's config file in coreconf.
265 ifndef USE_SYSTEM_ZLIB
266 ZLIB_LIBS = $(DIST)/lib/$(LIB_PREFIX)zlib.$(LIB_SUFFIX)
267 endif

::: external_tests/ssl_gtest/Makefile
@@ +45,5 @@
>  CFLAGS += -I$(CORE_DEPTH)/lib/ssl
> +
> +ifdef NSS_ENABLE_SSL_ZLIB
> +include $(CORE_DEPTH)/coreconf/zlib.mk
> +endif

Everything here (including the existing CFLAGS += assignment)
should be moved to section 4 of this makefile, because these
are make variable assignments rather than makefile rules.

Comment 9

[Wan-Teh Chang]

(In reply to Martin Thomson [:mt:] from comment #6)
> Comment on attachment 8714185 [details] [diff] [review]
[...]
> I think that I would have preferred to see this patch disable building of
> cmd/modutil and cmd/signtool if NSS_ENABLE_ZLIB was disabled.

Martin, I'm afraid that you misunderstood what NSS_ENABLE_ZLIB means.
NSS_ENABLE_ZLIB means "enable SSL/TLS compression" and only applies
to lib/ssl. This is why I asked Eric to rename it NSS_ENABLE_SSL_ZLIB.

cmd/modutil and cmd/signtool always need zlib (for jar file support).
 
> BTW, how do you disable this?  make NSS_ENABLE_SSL_ZLIB=0 will still try to
> build with zlib.

You need to unset NSS_ENABLE_SSL_ZLIB rather than setting it to 0.
The command line is:
    make NSS_ENABLE_SSL_ZLIB=

Comment 10

[Eric Rescorla (:ekr)]

Landed as: https://hg.mozilla.org/projects/nss/rev/2388b32dc14f

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

I think this is resolved now?

Comment 12

[Kai Engert [:KaiE:]]

I'm reopening this bug, because the changes don't work with mozilla-central.

Here is a "try" build of NSS revision b459ec1f23f7 :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f9159d67175&selectedJob=16438864

(That's an NSS revision that includes the zlib refactoring and several other changes that came after it, but it avoids the recent large changes for sqlite and clang-reformat, in order to keep the overall changes at a sane level for human inspection.)

Initially I had the plan to backout the patch from this bug, and ask you to work on a better patch for a later time, but it isn't possible to cleanly backup out this patch.

Therefore, my suggestion is to close the NSS tree until you have landed an incremental fix that works with mozilla-central.

Could you please test your attempted fix with a try build, prior to landing it into NSS?

Here's how you could do that:

- hg clone mozilla-central
- run the following script to update your mozilla tree to that revision NSS:
  python client.py update_nss b459ec1f23f7
- locally apply your local fixes on top
- hg addremove
- hg qnew try-fix-zlib
- hg qref -e
  use this as the first line:
  try: -b do -p all -u all -t none
- push to try

One it works, please add the link to the working try build in this, and let me know, then we can reopen the NSS tree.

Would that approach work for you?
Thanks

Comment 13

[Kai Engert [:KaiE:]]

Attachment: attempted, merged backout patch

Actually, it was relatively simple to manually merge and backout the patch.
This attachment has my attempted backout.

I've started a try build with the same revision as before, plus this backout patch, at https://treeherder.mozilla.org/#/jobs?repo=try&revision=42dce64f15ef

Comment 14

[Eric Rescorla (:ekr)]

Kai,

Based on Wan-Teh's comments, I believe the problem is not NSS but rather
M-C.

https://bugzilla.mozilla.org/show_bug.cgi?id=1243872#c8

Comment 15

[Eric Rescorla (:ekr)]

Kai, here is the fix to m-c:
https://hg.mozilla.org/try/rev/74d15ce93512

And here is a try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca7c3a3a1bbd&selectedJob=16447685

Comment 16

[Kai Engert [:KaiE:]]

Thanks Eric. Your try push looks good. I've attached our patch to the nss-3.23-uplift bug 1245053.

Resolving this one again.

Comment 17

[Eric Rescorla (:ekr)]

Kai thanks for addressing this on the weekend!

Comment 18

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #8)
> Just a reminder: you'll need to update Mozilla's
> config/external/nss/Makefile.in when you push a new
> NSS update to mozilla-central:
> 
> http://mxr.mozilla.org/mozilla-central/search?string=NSS_ENABLE_ZLIB

Thanks for explaining that.

In the future, it would be helpful to collect any requirements that are related to uplifting Mozilla to a newer NSS in a NSS uplift bug (such as bug 1245053), and if no such bug exists yet, file that bug and CC the people who usually do the uplifting.

It could have saved me some work today, if I had known about this earlier.

Comment 19

[Wan-Teh Chang]

Attachment: Continue to support NSS_ENABLE_ZLIB until NSS 3.25.

The simplest solution is to change]
    NSS_ENABLE_ZLIB=
to
    NSS_ENABLE_SSL_ZLIB=
in config/external/nss/Makefile.in.

But this patch may be a good solution because it allows
a transition period. It requires the |flavor| function
in GNU make 3.81. The existence of these pymake tests
shows pymake supports the |flavor| function:
http://mxr.mozilla.org/mozilla-central/source/build/pymake/tests/dynamic-var.mk
http://mxr.mozilla.org/mozilla-central/source/build/pymake/tests/var-change-flavor.mk

Comment 20

[Eric Rescorla (:ekr)]

Comment on attachment 8716658 [details] [diff] [review]
Continue to support NSS_ENABLE_ZLIB until NSS 3.25.

Review of attachment 8716658 [details] [diff] [review]:
-----------------------------------------------------------------

Wan-Teh, this appears to be correct. I'm not sure if it's necessary, but I'm fine with landing it

Comment 21

[Wan-Teh Chang]

I just discovered that NSS's existing SSL environment variables
all use the NSS_SSL_ prefix:

http://mxr.mozilla.org/nss/search?string=NSS_.*SSL_&regexp=on&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=nss

If it's not too late, I'd like to rename NSS_ENABLE_SSL_ZLIB to
NSS_SSL_ENABLE_ZLIB. I'm willing to do the work (tomorrow, Sunday).

Comment 22

[Eric Rescorla (:ekr)]

Wan-Teh, it's fine with me and I can do the work today letting you review it tomorrow.

Is there anyone else we need signoff from?

Comment 23

[Eric Rescorla (:ekr)]

Attachment: 0001-Bug-1243872.-Rename-NSS_ENABLE_SSL_ZLIB-NSS_SSL_ENAB.patch

Comment 24

[Wan-Teh Chang]

Comment on attachment 8716665 [details] [diff] [review]
0001-Bug-1243872.-Rename-NSS_ENABLE_SSL_ZLIB-NSS_SSL_ENAB.patch

Review of attachment 8716665 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Thank you, Eric!

Comment 25

[Wan-Teh Chang]

(In reply to Eric Rescorla (:ekr) from comment #22)
> Wan-Teh, it's fine with me and I can do the work today letting you review it
> tomorrow.
> 
> Is there anyone else we need signoff from?

I checked the NSS tree status. We are between NSS_3_23_BETA3
and RTM, so it should be fine to land the patch to rename
NSS_ENABLE_SSL_ZLIB.

Comment 26

[Eric Rescorla (:ekr)]

Rename committed as: 
 https://hg.mozilla.org/projects/nss/rev/c139550de2fb

Comment 27

[Kai Engert [:KaiE:]]

Comment on attachment 8716658 [details] [diff] [review]
Continue to support NSS_ENABLE_ZLIB until NSS 3.25.

I apologize.

I failed to notice this patch in time.

NSS 3.23 has already been released without this compatibility change.