Clean up zlib-related fragments of NSS makefiles

RESOLVED FIXED in 3.23

Status

NSS
Build
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jld, Assigned: jld)

Tracking

Firefox Tracking Flags

(firefox47 affected)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8713336 [details] [diff] [review]
bug1243872-zlib-mk-hg0.diff
Attachment #8713336 - Flags: review?(wtc)

Comment 2

2 years ago
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
Attachment #8713336 - Flags: review?(wtc) → review-

Comment 3

2 years ago
Created attachment 8714157 [details] [diff] [review]
0001-Bug-1243872-Refactor-zlib-support-in-Makefiles.patch

Comment 4

2 years ago
Created attachment 8714185 [details] [diff] [review]
0001-Bug-1243872-Refactor-zlib-support-in-Makefiles.patch

Updated

2 years ago
Attachment #8714157 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8714185 - Flags: review?(martin.thomson)

Comment 5

2 years ago
MT, I revised this per WTC's comments.

WTC: needinfo for your visibility
Flags: needinfo?(wtc)
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 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.
Attachment #8714185 - Flags: review?(martin.thomson) → review+

Comment 8

2 years ago
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.
Attachment #8714185 - Flags: review+

Comment 9

2 years ago
(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=
Flags: needinfo?(wtc)
Landed as: https://hg.mozilla.org/projects/nss/rev/2388b32dc14f
(Assignee)

Comment 11

2 years ago
I think this is resolved now?
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
OS: Unspecified → All
Priority: -- → P2
Hardware: Unspecified → All
Target Milestone: --- → 3.23

Comment 12

2 years ago
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
Status: RESOLVED → REOPENED
Flags: needinfo?(jld)
Resolution: FIXED → ---

Comment 13

2 years ago
Created attachment 8716618 [details] [diff] [review]
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
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
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

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

Resolving this one again.
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Flags: needinfo?(jld)
Resolution: --- → FIXED
Kai thanks for addressing this on the weekend!

Comment 18

2 years ago
(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

2 years ago
Created attachment 8716658 [details] [diff] [review]
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
Attachment #8716658 - Flags: superreview?(kaie)
Attachment #8716658 - Flags: review?(ekr)
Attachment #8716658 - Flags: feedback?(martin.thomson)
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
Attachment #8716658 - Flags: review?(ekr) → review+

Comment 21

2 years ago
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).
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?
Flags: needinfo?(wtc)
Created attachment 8716665 [details] [diff] [review]
0001-Bug-1243872.-Rename-NSS_ENABLE_SSL_ZLIB-NSS_SSL_ENAB.patch
Attachment #8716665 - Flags: review?(wtc)

Comment 24

2 years ago
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!
Attachment #8716665 - Flags: review?(wtc) → review+

Comment 25

2 years ago
(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.
Flags: needinfo?(wtc)
Rename committed as: 
 https://hg.mozilla.org/projects/nss/rev/c139550de2fb

Updated

2 years ago
Attachment #8716658 - Flags: feedback?(martin.thomson) → feedback+

Comment 27

2 years ago
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.
Attachment #8716658 - Flags: superreview?(kaie)
You need to log in before you can comment on or make changes to this bug.