Closed Bug 1186917 Opened 5 years ago Closed 5 years ago

Allow package maintainers to optionally build utils without the rest of nss

Categories

(NSS :: Build, defect)

defect
Not set
normal

Tracking

(firefox42 affected)

RESOLVED FIXED
Tracking Status
firefox42 --- affected

People

(Reporter: elio.maldonado.batiz, Assigned: elio.maldonado.batiz)

Details

Attachments

(2 files, 13 obsolete files)

11.58 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
1.31 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=835919#c1 for some background information. Our build system is flexible enough to allow downstream package maintainers to build nss without softoken and conversely to build softoken/freebl without the rest of nss. The part that's missing is the ability to build utils by itself, well not quite as it will be building utils plus coreconf. I'm working on adapting a patch uses on fedora and RHEL to accomplish this.
s/uses/used/
QA Contact: emaldona
Summary: Allow package maintainers to optionally building utils without the rest of nss → Allow package maintainers to optionally build utils without the rest of nss
Assignee: nobody → emaldona
QA Contact: emaldona
First cut of a work in progress which I'm still testing.
Attachment #8638023 - Flags: review?(rrelyea)
Comment on attachment 8638023 [details] [diff] [review]
support optionally building util only - V1

Tested on fedora and OS X. I'm able to build util only by setting the build time environment variable, export NSS_BUILD_UTIL_ONLY=1, for build and test execution. Normal build are working fine. As I stated in comments the NSS_BUILD_SOFTOKEN_ONLY still requires the downstream to do some work as before. That's outside the scope of this effort, which is to reduce the number patches applied downstream and when that's not possible at least make them very small and easy to carry forward and maintain.
Attachment #8638023 - Attachment description: support optionally building util only V1 → support optionally building util only - V1
Comment on attachment 8638023 [details] [diff] [review]
support optionally building util only - V1

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

r+

Please have feedback from mozilla or google before checking in.

bob
Attachment #8638023 - Flags: review?(rrelyea) → review+
Attachment #8638023 - Flags: feedback?(ryan.sleevi)
Comment on attachment 8638023 [details] [diff] [review]
support optionally building util only - V1

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

Gonna flag this for ekr to look at. I'm no good with makefiles, and #ifdefs always make me sad.

::: lib/Makefile
@@ +65,5 @@
>  #######################################################################
>  # (7) Execute "local" rules. (OPTIONAL).                              #
>  #######################################################################
>  
>  ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)

Does this imply there's going to be an NSS_BUILD_WITHOUT_UTIL as well?

@@ +74,2 @@
>  else
> +    ifeq ($(NSS_BUILD_SOFTOKEN_ONLY),1)

Why this? This seems unrelated to your change?
Attachment #8638023 - Flags: feedback?(ryan.sleevi) → feedback?(ekr)
(In reply to Ryan Sleevi from comment #5)
> Comment on attachment 8638023 [details] [diff] [review]
> support optionally building util only - V1
> 
> >  # (7) Execute "local" rules. (OPTIONAL).                              #
> >  #######################################################################
> >  
> >  ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)
> 
> Does this imply there's going to be an NSS_BUILD_WITHOUT_UTIL as well?

No, that isn't needed. There are 4 types of build.
1) The default is to build with everything in it as we do now and continue to do. If no xx_BUILD_this_away build time are set that what we get.

2) NSS_BUILD_wITHOUT_SOFTOKEN (which should aherv been called means ..._WITHOUT_SOFTOKEN_oOR_UTIL) which downstream we do since we have previously build softoken(freebl) and the results (libraries and headers) arew available in the system.

3) NSS_BUILD_SOFTOKEN_ONLY which we do before nss higher layers build. On RHEL 6/7 that happens rather infrequently as we keep it at the last FIPS-140 validated version.

4) NSS_BUILD_UTIL_ONLY which downstream is the first one as for nss higher layer and softoken depend on this one being available.
 

Waht make s
> 
> @@ +74,2 @@
> >  else
> > +    ifeq ($(NSS_BUILD_SOFTOKEN_ONLY),1)
> 
> Why this? This seems unrelated to your change?

It's actually closely related. It will make more sense once you read Bug 835919. This bug is to completes (and amends) the build system changes that were done there.
Erik, Is there any additional info I should provide to help with the feedback? -Elio
Flags: needinfo?(ekr)
No, I jut need to get to this. Will try to do so this week.
Flags: needinfo?(ekr)
Comment on attachment 8638023 [details] [diff] [review]
support optionally building util only - V1

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

Elio,

I believe that this works as advertised, but I also think it could be a
somewhat cleaner. I don't want to block you checking this in if it's urgent,
especially given that the make system is already kind of a mess, so it's OK
with me if you land this as-is if you have to, but I'd be happier if you
cleaned up some of this stuff.

::: cmd/manifest.mn
@@ +10,5 @@
> +REQUIRES = nspr
> +
> +DIRS =  \
> + $(NULL)
> +else

Nit: Suggest you close up the vertical whitespace to remove that between REQUIRES and DIRs and add whitespace around #else, so it makes it clearer which clauses go with which.

::: lib/Makefile
@@ +74,5 @@
>  else
> +    ifeq ($(NSS_BUILD_SOFTOKEN_ONLY),1)
> +      UTIL_SRCDIR = util
> +      FREEBL_SRCDIR = freebl
> +      SOFTOKEN_SRCDIR = softoken

Doesn't this duplicate the all branch?

I would suggest that instead you put in a test that generates an error if BUILD_UTIL_ONLY and BUILD_SOFTOKEN_ONLY are both set.

::: lib/manifest.mn
@@ +23,5 @@
> +else
> +ifdef NSS_BUILD_SOFTOKEN_ONLY
> +# Building softoken (and freebl) only requires that the paths
> +# to the locations where the util headers and libraries were
> +# previuously installed by a prior util-only build, likely in

Nit: misspelling of previously.

@@ +50,5 @@
>  	crmf jar \
>  	ckfw $(SYSINIT_SRCDIR) \
>  	$(NULL)
> +endif
> +endif

Nit: this looks correct but I would suggest that there's a lot of duplication here. Could you instead do

DIRS = $(UTIL_SRCDIR)\
       $(NULL)
 

#ifndef NSS_BUILD_UTIL_ONLY
DIRS += $(UTIL_SRCDIR) \
		$(FREEBL_SRCDIR) \
		$(SQLITE_SRCDIR) \
	$(DBM_SRCDIR) \
		$(SOFTOKEN_SRCDIR) \
		$(NULL)
#ifndef NSS_BUILD_SOFTOKEN_ONLY
DIRS += base
        ...
#endif
#endif

::: tests/all.sh
@@ +317,5 @@
> +    TESTS=" "
> +    ALL_TESTS=${TESTS}
> +    NSS_SSL_TESTS=" "
> +    NSS_SSL_TESTS=" "
> +  else

Do we really want to put this test inside here? If we do have util tests, won't
we want to check that the build had been run? Suggest we test for NSS_BUILD_UTIL_ONLY outside of the test for the build.
Attachment #8638023 - Flags: feedback?(ekr) → feedback+
This version addresses Eric's suggestions from Comment 9. Caveat, I'm still testing the different build variants.
Attachment #8638023 - Attachment is obsolete: true
Attachment #8663099 - Flags: review?(ekr)
Attachment #8663099 - Flags: review?(rrelyea)
Comment on attachment 8663099 [details] [diff] [review]
support optionally bulding util only V2

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

::: lib/Makefile
@@ +80,5 @@
> +      FREEBL_SRCDIR =
> +      SOFTOKEN_SRCDIR =
> +    else
> +     ifeq ($(NSS_BUILD_SOFTOKEN_ONLY),1)
> +        FREEBL_SRCDIR = freebl

Shouldn't you set UTIL_SRCDIR here one way or the otehr?

::: tests/all.sh
@@ +316,5 @@
> +  TESTS=" "
> +  ALL_TESTS=${TESTS}
> +  NSS_SSL_TESTS=" "
> +  NSS_SSL_TESTS=" "
> +fi

This will still cause an error on the check on line 322. Is that the intended behavior?
Attachment #8663099 - Flags: review?(ekr)
(In reply to Eric Rescorla (:ekr) from comment #11)
Good questions and I better not rush an answer. I just did the default build and the tests passed but that's the default full nss build. As mention in my caveat, I'm still in the process of building and testing the other build options: NSS_BUILD_WITHOUT_SOFTOKEN, NSS_BUILD_SOFTOKEN_ONLY and NSS_BUILD_UTIL_ONLY builds. For these I emulate what's done downstream for fedora where out of the full nss source tar ball I create smaller source tar balls for nss-util and nss-softoken where only what's needed is kept. It's a tricky set of patches to review, or for me to explain, so let me verify my assumptions as the proof of the pudding is the testing.
(In reply to Eric Rescorla (:ekr) from comment #11)
> Comment on attachment 8663099 [details] [diff] [review]
> support optionally bulding util only V2
> 
> Review of attachment 8663099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/Makefile
> @@ +80,5 @@
> > +      FREEBL_SRCDIR =
> > +      SOFTOKEN_SRCDIR =
> > +    else
> > +     ifeq ($(NSS_BUILD_SOFTOKEN_ONLY),1)
> > +        FREEBL_SRCDIR = freebl
> 
> Shouldn't you set UTIL_SRCDIR here one way or the otehr?

UTIL_SRCDIR is set to empty for NSS_BUILD_WITHOUT_SOFTOKEN, etherwise it it set to util.
For NSS_BUILD_UTIL_ONLY and the default of course but for NSS_BUILD_WITHOUT_SOFTOEN (wich means without softoken and also without util) is woulld also be unset, or set to empty as long as the build can access the files it needs (headers and shared libraries) from specified paths set via environment variables. I do it this way in fedora where the source tree is modified to remove the source directories for the pieces that have already been built.

> 
> ::: tests/all.sh
> @@ +316,5 @@
> > +  TESTS=" "
> > +  ALL_TESTS=${TESTS}
> > +  NSS_SSL_TESTS=" "
> > +  NSS_SSL_TESTS=" "
> > +fi
> 
> This will still cause an error on the check on line 322. Is that the
> intended behavior?
Yes, there are mistakes: 
+  CYCLES=" "
+  TESTS=" "
+  ALL_TESTS=${TESTS}
should be
+  NSS_CYCLES=" "
+  NSS_TESTS=" "
+  ALL_TESTS=${TESTS}
After that in all.sh I should be
if [ ! -f ${DIST}/${OBJDIR}/bin/${LAST_FILE_BUILT}${PROG_SUFFIX} ]; then
  if [ "${NSS_BUILD_UTIL_ONLY}" = "1" ]; then
    # Currently no tests are run or built when building util only.
    # This may change in the future, atob and bota are
    # possible candidates.
    echo "No tests were built"
  else
    echo "Build Incomplete. Aborting test." >> ${LOGFILE}
    html_head "Testing Initialization"
    Exit "Checking for build"
  fi
fi
which skips the check for the NSS_BUILD_UTIL_ONLY (at the present we don't have tests) and perform it otherwise.

I have tested the default build and util-only builds. The softoken-only build requires a lot of work as I must point to the paths were I deposited the results on the util-only build. The util-only, softoken-only, and nss-without-softokn-or-util builds require quite a bit preparation which is done downstream via the spec files we use for Fedora and Fedora-derived distributions.
Attachment #8663099 - Attachment is obsolete: true
Attachment #8663099 - Flags: review?(rrelyea)
I should mention that all.sh invoked the conventional way doesn't work in in my environments. This is regardless of this or other work I do. I use this convenience makefile installed above the nss directory along with some scripts.
https://fedorapeople.org/~emaldonado/makefiles4upstream/Makefile
I rum run all.sh via the the "make test" target that invokes the following script
https://fedorapeople.org/~emaldonado/scripts/alltests.sh
Additionally, the makefile also supports gtests via the all4gtest and gtests targets with the help of https://fedorapeople.org/~emaldonado/scripts/gtests.sh
More info when I get back next week.
The additional info is the couple of scripts that we use downstream in fedora were we take the upstream source tar ball and split of from it nss-util and nss-softoken to create new source tarball with only what's needed.
https://fedorapeople.org/~emaldonado/scripts/nss-split-util.sh
https://fedorapeople.org/~emaldonado/scripts/nss-split-softokn.sh
changes just what's needed, follows a more consistent style, conditionals going from the more narrow builds to the less narrow, easier to maintain and more importantly this one actually worked when I tested it downstream.
Attachment #8665027 - Attachment is obsolete: true
Attachment #8669024 - Flags: review?(ekr)
Target Milestone: --- → 3.22
Attachment #8669024 - Attachment description: support optional util-only builds - V2 → support optional util-only builds - V4
As of NSS 3.21 we build and execute the external tests by default, which is a good thing, but causes problems when exersizing the NSS_BUILD_UTIL_ONLY and NSS_BUILD_SOFTOKEN_ONLY options as this test are ssl/tls oriented. A new version of the patch is needed.

On nss/lib/Makile I see

ifdef NSS_DISABLE_GTESTS
DIRS := $(filter-out external_tests,$(DIRS))
endif

So when using the aforementioned NSS_BUILD_{UTIL|SOFOKEN}_ONLY build options I need to do 'export NSS_DISABLE_GTESTS=1', which likely be in the downstream spec files as well as in that meta Makefile I use when experimenting here. Investigating other options. A new patch should be coming soon.
Comment on attachment 8669024 [details] [diff] [review]
support optional util-only builds - V4

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

::: lib/manifest.mn
@@ +45,5 @@
>  	base dev pki \
>  	libpkix \
>  	certdb certhigh pk11wrap cryptohi nss \
>  	$(ZLIB_SRCDIR) ssl \
>  	pkcs12 pkcs7 smime \

This line should be changed to
	pkcs7 pkcs12 smime \
Attachment #8669024 - Flags: review?(emaldona)
Comment on attachment 8669024 [details] [diff] [review]
support optional util-only builds - V4

r-. Though it builds with NSS_BUILD_UTIL_ONLY it fails with the NSS_BUILD_SOFTOKEN_ONLY option. This needs more work and possibly looking at some steps that are downstream in fedora such as removing unwanted source directories when doing softoken only or nss without softoken.
Attachment #8669024 - Flags: review?(emaldona) → review-
Attachment #8669024 - Attachment is obsolete: true
Attachment #8669024 - Flags: review?(ekr)
Attachment #8692634 - Flags: review?(ekr)
Comment on attachment 8692634 [details] [diff] [review]
support optional util-only builds - V5

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

There are a bunch of style comments about having less duplication. I trust you to follow your judgement there and take them as appropriate.

You should fix things so that the variables are either compared against 1 or whether they are defined
consistently.

::: cmd/platlibs.mk
@@ +198,5 @@
> +	-lplc4 \
> +	-lplds4 \
> +	-lnspr4 \
> +	$(NULL)
> +else

There's a lot of duplication here. Can we instead have a shared arm and a !SOFTOKEN_ONLY arm.

Also above..

::: lib/Makefile
@@ +47,5 @@
>  endif
>  
> +ifeq ($(NSS_BUILD_UTIL_ONLY),1)
> +SYSINIT_SRCDIR=
> +endif

Can you explain why this is needed? Where else is it set?

@@ +78,5 @@
> +  else
> +    ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)
> +      # Not included when building nss without softoken
> +      # This build type uses the build results of the prior
> +      # NSS_BUILD_UTIL_ONLY and NSS_BUILD_SOFTOKEN_ONLY builds

I am a bit confused by this comment. Does that mean you have to do separate builds with those settingS?

::: lib/manifest.mn
@@ +15,5 @@
>  #  ssl
>  #  smime
>  #  ckfw (builtins module)
>  #  crmf jar (not dll's)
> +ifdef NSS_BUILD_UTIL_ONLY

Elsewhere you compare this to 1. Mixing these seems like it's going to have some inconsistent effects.

@@ +35,5 @@
> +	$(DBM_SRCDIR) \
> +	$(SOFTOKEN_SRCDIR) \
> +	$(NULL)
> +  else
> +    DIRS = \

This is a matter of taste, but do you think it would be better to instead of repeating the directories do:


	  DIRS = \
		$(UTIL_SRCDIR) \
		$(NULL)
	
ifndef NSS_BUILD_UTIL_ONLY
          DIRS += ...

::: tests/all.sh
@@ +309,5 @@
>    LAST_FILE_BUILT=modutil
>  fi
>  
>  if [ ! -f ${DIST}/${OBJDIR}/bin/${LAST_FILE_BUILT}${PROG_SUFFIX} ]; then
> +  if [ "${NSS_BUILD_UTIL_ONLY}" = "1" ]; then

I am OK with this being 1, but do you perhaps want to instead have it be compared against "" and 0?

I just note that if I do NSS_BUILD_UTIL_ONLY=Yes that will not have the expected consequences.
Attachment #8692634 - Flags: review?(ekr)
(In reply to Eric Rescorla (:ekr) from comment #22)
> Comment on attachment 8692634 [details] [diff] [review]
> support optional util-only builds - V5
> 
> Review of attachment 8692634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are a bunch of style comments about having less duplication. I trust
> you to follow your judgement there and take them as appropriate.

I agree with that the style is inconsistent and I'm working on it. I'll switch to conditionals using ifdef ... or ifndef instead of ifeq ($(something), 1) or ifneq ($(something),1). Will try to follow existing practice in the various files I patch. 

You rightly point out duplication but when I tried to avoid it I ran into build problems. See below.

> 
> You should fix things so that the variables are either compared against 1 or
> whether they are defined
> consistently.
> 
> ::: cmd/platlibs.mk
> @@ +198,5 @@
> > +	-lplc4 \
> > +	-lplds4 \
> > +	-lnspr4 \
> > +	$(NULL)
> > +else
> 
> There's a lot of duplication here. Can we instead have a shared arm and a
> !SOFTOKEN_ONLY arm.

When I tried that I ran into
> 
> Also above..
> 
> ::: lib/Makefile
> @@ +47,5 @@
> >  endif
> >  
> > +ifeq ($(NSS_BUILD_UTIL_ONLY),1)
> > +SYSINIT_SRCDIR=
> > +endif
> 
> Can you explain why this is needed? Where else is it set?
That's because when doing the util-only build the SYSINIT_SRCDIR is not present. This is to support proper NSS initialization via a system-wide shared database, currently available only for Fedora derived distributions. See https://wiki.mozilla.org/NSS_Shared_DB_And_LINUX

> 
> @@ +78,5 @@
> > +  else
> > +    ifeq ($(NSS_BUILD_WITHOUT_SOFTOKEN),1)
> > +      # Not included when building nss without softoken
> > +      # This build type uses the build results of the prior
> > +      # NSS_BUILD_UTIL_ONLY and NSS_BUILD_SOFTOKEN_ONLY builds
> 
> I am a bit confused by this comment. Does that mean you have to do separate
> builds with those settingS?

That's a comment I will remove. In other places I have similar comments but in those files there is sufficient surrounding context that there is no confusion. Such is not the case here. Yrs we do separate build with those setting but we do it downstream only with the help of the fedora/RHEL rpm based build s along with other support that's not available here. There we do
export NSS_BUILD_UTIL_ONLY=1 and export NSS_BUILD_SOFTOKEN_ONLY=1 
in the nss-{util|softokn}.spec files when building the nss-util and nss-softokn packages, respectively.

As stated earlier, the comment is best removed.
> 
> ::: lib/manifest.mn
> @@ +15,5 @@
> >  #  ssl
> >  #  smime
> >  #  ckfw (builtins module)
> >  #  crmf jar (not dll's)
> > +ifdef NSS_BUILD_UTIL_ONLY
> 
> Elsewhere you compare this to 1. Mixing these seems like it's going to have
> some inconsistent effects.

Yes, I'm switching to the style ifdef or ifdnef wherever I can and consistent with surrounding usage.

> 
> @@ +35,5 @@
> > +	$(DBM_SRCDIR) \
> > +	$(SOFTOKEN_SRCDIR) \
> > +	$(NULL)
> > +  else
> > +    DIRS = \
> 
> This is a matter of taste, but do you think it would be better to instead of
> repeating the directories do:
> 
> 
> 	  DIRS = \
> 		$(UTIL_SRCDIR) \
> 		$(NULL)
> 	
> ifndef NSS_BUILD_UTIL_ONLY
>           DIRS += ...

Here is my attempt to do that in cmd/manifest.mn

-------- begin excerpt --------------
DIRS = \
 $(NULL)
ifndef NSS_BUILD_UTIL_ONLY
REQUIRES = nss nspr libdbm

DIRS += \
 lib  \
 $(BLTEST_SRCDIR) \
 $(FIPSTEST_SRCDIR)  \
 $(LOWHASHTEST_SRCDIR)  \
 $(SHLIBSIGN_SRCDIR) \
 $(NULL)
ifndef NSS_BUILD_SOFTOKEN_ONLY
DIRS +=  \
 addbuiltin \
 atob  \
 btoa  \
<< omitted for brevity >>
 vfyserv \
 modutil \
 $(NULL)
endif
endif
-------- end excerpt --------------
When I tried build it failed when it it came to bltest.

cd bltest; make libs
make[3]: Entering directory '/home/emaldona/work4nss/upstream/bug1186917nodupes/nss/cmd/bltest'
make: *** No rule to make target ../../../dist/Linux4.2_x86_64_cc_glibc_PTH_64_DBG.OBJ/lib/libnssutil3.a.  Stop.
../../coreconf/rules.mk:882: recipe for target '../../../dist/Linux4.2_x86_64_cc_glibc_PTH_64_DBG.OBJ/lib/libnssutil3.a' failed
make[3]: *** [../../../dist/Linux4.2_x86_64_cc_glibc_PTH_64_DBG.OBJ/lib/libnssutil3.a] Error 1
make[3]: Leaving directory '/home/emaldona/work4nss/upstream/bug1186917nodupes/nss/cmd/bltest'
../coreconf/rules.mk:101: recipe for target 'libs' failed
make[2]: *** [libs] Error 2
make[2]: Leaving directory '/home/emaldona/work4nss/upstream/bug1186917nodupes/nss/cmd'
coreconf/rules.mk:101: recipe for target 'libs' failed

For some reason it tries to build libnssutil3.a as a dependency which is wrong. The static library is actually named libnssutil.a and that's already built. 

> 
> ::: tests/all.sh
> @@ +309,5 @@
> >    LAST_FILE_BUILT=modutil
> >  fi
> >  
> >  if [ ! -f ${DIST}/${OBJDIR}/bin/${LAST_FILE_BUILT}${PROG_SUFFIX} ]; then
> > +  if [ "${NSS_BUILD_UTIL_ONLY}" = "1" ]; then
> 
> I am OK with this being 1, but do you perhaps want to instead have it be
> compared against "" and 0?
> 
> I just note that if I do NSS_BUILD_UTIL_ONLY=Yes that will not have the
> expected consequences.

IMO, trying to support Yes|yes vs. No|no along with 1 vs 0 is not worth the extra effort, so I'm glad that you are  okay with 1 :-)

Thank you Eric for such thorough feedback.

-Elio
You may have noticed my commits to the BUG1168917_BRANCH which I created to help me experiment a bit.
See comments on https://hg.mozilla.org/projects/nss/rev/e7bcbbb36684. This allows me to create experimental source tar balls and do testing in a fedora which is where it counts. The benefits for downstream maintenance simplification are rather modest. I was able to get rid of three downstream local patches, one for nss-util and two for nss-softokn, which is not a great deal but with the proposed changes upstream maintenance in the future as the intentions are better documented.
As I mentioned earlier
..........
cd bltest; make libs
make[2]: Entering directory '/home/emaldona/work4nss/upstream/bug1186917nodupes/nss/cmd/bltest'
make: *** No rule to make target ../../../dist/Linux4.2_x86_64_cc_glibc_PTH_64_DBG.OBJ/lib/libnssutil3.a.  Stop.
../../coreconf/rules.mk:882: recipe for target '../../../dist/Linux4.2_x86_64_cc_glibc_PTH_64_DBG.OBJ/lib/libnssutil3.a' failed
make[2]: *** [../../../dist/Linux4.2_x86_64_cc_glibc_PTH_64_DBG.OBJ/lib/libnssutil3.a] Error 1
make[2]: Leaving directory '/home/emaldona/work4nss/upstream/bug1186917nodupes/nss/cmd/bltest'
../coreconf/rules.mk:101: recipe for target 'libs' failed
make[1]: *** [libs] Error 2
make[1]: Leaving directory '/home/emaldona/work4nss/upstream/bug1186917nodupes/nss/cmd'
coreconf/rules.mk:101: recipe for target 'libs' failed

How do I prevent that unwanted 3, LIBRARY_VERSION I guess, from getting appended to the static library name when the rules are applied.
Flags: needinfo?(rrelyea)
Comment on attachment 8696077 [details] [diff] [review]
patch without duplication which fails when builting bltest

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

A new patch is coming once I'm done testing. I will commit it to the BUG1168917_BRANCH first so I can create an archive to test in fedora.

::: cmd/platlibs.mk
@@ +106,5 @@
>  EXTRA_LIBS += \
> +	$(NSSUTIL_LIB_DIR)/$(LIB_PREFIX)nssutil3.$(LIB_SUFFIX) \
> +	$(NSPR_LIB_DIR)/$(NSPR31_LIB_PREFIX)plc4.$(LIB_SUFFIX) \
> +	$(NSPR_LIB_DIR)/$(NSPR31_LIB_PREFIX)plds4.$(LIB_SUFFIX) \
> +	$(NSPR_LIB_DIR)/$(NSPR31_LIB_PREFIX)nspr4.$(LIB_SUFFIX) \

Remove these lines, they aren't needed and are the cause of the failure

@@ +113,5 @@
> +        EXTRA_LIBS += \
> +	$(DIST)/lib/$(LIB_PREFIX)sectool.$(LIB_SUFFIX) \
> +	$(SOFTOKENLIB) \
> +	$(CRYPTOLIB) \
> +	$(SQLITE_LIB_DIR)/$(LIB_PREFIX)$(SQLITE_LIB_NAME).$(LIB_SUFFIX) \

remove this line also

@@ +145,2 @@
>  
>  ifeq ($(OS_ARCH), AIX) 

ws
Flags: needinfo?(rrelyea)
Addresses most feedback comments from Eric such as avoiding duplication.
Attachment #8692634 - Attachment is obsolete: true
Attachment #8696077 - Attachment is obsolete: true
Attachment #8696305 - Flags: feedback?(ekr)
Same as V6 plus some indentation cleanup
Attachment #8696305 - Attachment is obsolete: true
Attachment #8696305 - Flags: feedback?(ekr)
Attachment #8696339 - Flags: feedback?(ekr)
As V7 but the platlibs.mk changes apply to the Windows section as well. This I do  strictly for the sake of consistency as no one will ever want to take advantage of this on Windows.
Attachment #8696339 - Attachment is obsolete: true
Attachment #8696339 - Flags: feedback?(ekr)
Attachment #8696485 - Flags: review?(rrelyea)
Comment on attachment 8696485 [details] [diff] [review]
support optional util-only or softoken-only builds - V8

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

So I would prefer to see things use the make variables rather the the +=, but I'm not going to die on that hill.

I will insist that you make keep the link order for the normal library linking which I identified below., that is the only issue that generated an r-. Addressing the rest is extra credit.

bob

::: cmd/manifest.mn
@@ +18,5 @@
> + $(BLTEST_SRCDIR) \
> + $(FIPSTEST_SRCDIR)  \
> + $(LOWHASHTEST_SRCDIR)  \
> + $(SHLIBSIGN_SRCDIR) \
> + $(NULL)

The other way to do this is to create additional variables:

$(SOFTOKEN_SRCDIRS)
$(NSS_SRCDIRS)


So you'd have

SOFTOKEN_SRCDIRS=
NSS_SRCDIRS=

ifndef NSS_BUILD_UTIL_ONLY
SOFTOKEN_SRCDIRS= \
   lib \
   $(BLTEST_SRCDIR)
    ...
   $(NULL)

#ifndef NSS_BUILD_SOFTOKEN_ONLY
NSS_SRCDIRS = \
   addbuiltin \
     ....
   $(NULL)
#endif
#endif

DIRS = \
   $(SOFTOKEN_SRCDIRS) \
   $(NSS_SRCDIRS) \
   $(NULL)

::: cmd/platlibs.mk
@@ +211,5 @@
> +	-lssl3 \
> +	-lsmime3 \
> +	-lnss3 \
> +	$(NULL)
> +endif

I'm a little worried about this here. we want to make sure symbols supplied by the other libraries are properly picked up even if they are only referenced by these lower ones. You may need to use the variable trick to make sure these libraries are included in the correct order if they are included.

::: lib/Makefile
@@ +73,5 @@
>  else
> +  ifeq ($(NSS_BUILD_SOFTOKEN_ONLY),1)
> +      UTIL_SRCDIR =
> +      FREEBL_SRCDIR = freebl
> +      SOFTOKEN_SRCDIR = softoken

good use of variables here.

::: lib/manifest.mn
@@ +25,5 @@
> +	# previously installed by a prior util-only build, likely in
> +	# in a system location that varies with the distribution. This
> +	# cannot be addressed here and requires that downstream package
> +	# mantainers add suitable modifications. Building fill nss will
> +	# not have that problem everything is available.

Another good place to use variables.
Attachment #8696485 - Flags: review?(rrelyea) → review-
This version preserves the link order as requested in the review. It also removes some duplicated lines in the list. Opting out of the extra credit for the time being.
Attachment #8696485 - Attachment is obsolete: true
Attachment #8698110 - Flags: review?(rrelyea)
Comment on attachment 8698110 [details] [diff] [review]
support optional util-only or softoken-only builds - V9

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

::: lib/manifest.mn
@@ +26,5 @@
> +	# in a system location that varies with the distribution. This
> +	# cannot be addressed here and requires that downstream package
> +	# mantainers add suitable modifications. Building fill nss will
> +	# not have that problem everything is available.
> +	DIRS += \

don't indent this line

@@ +34,5 @@
>  	$(SOFTOKEN_SRCDIR) \
> +	$(NULL)
> +ifndef NSS_BUILD_SOFTOKEN_ONLY
> +	# default, add the rest of nss
> +	DIRS += \

don't indent this line
Status: NEW → ASSIGNED
Address Bob's review comments. Preserve original link order and use variables to avoid += usage. Opting in for the extra credit.
Attachment #8698110 - Attachment is obsolete: true
Attachment #8698110 - Flags: review?(rrelyea)
Attachment #8699553 - Flags: review?(rrelyea)
Comment on attachment 8699553 [details] [diff] [review]
support optional util-only or softoken-only builds - V10

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

I've only done a partial review, basically of what I asked you to change before.. Since it wasn't up to snuff, I'm r- it here.

::: cmd/platlibs.mk
@@ +104,5 @@
>  	$(DIST)/lib/$(LIB_PREFIX)nsspki.$(LIB_SUFFIX) \
>  	$(DIST)/lib/$(LIB_PREFIX)nssdev.$(LIB_SUFFIX) \
>  	$(DIST)/lib/$(LIB_PREFIX)nssb.$(LIB_SUFFIX) \
>  	$(PKIXLIB) \
>  	$(DBMLIB) \

This still looks like it reorders the libraries.

@@ +136,5 @@
> +	$(DIST)/lib/$(LIB_PREFIX)pk11wrap.$(LIB_SUFFIX) \
> +	$(DIST)/lib/$(LIB_PREFIX)certhi.$(LIB_SUFFIX) \
> +	$(NULL)
> +endif
> +endif

you need to split NSS_LIBS up so that you can keep the same link order.

@@ +164,1 @@
>  	$(NULL)

The resulting right side should have exactly the same libraries with exactly the same order as the left. It doesn't
You need more NSS_LIBS variables to accomplish this (probably 4 in total).
Attachment #8699553 - Flags: review?(rrelyea) → review-
Addresses the latest review comments. Uses more NSS_LIBS variables in order to have exactly the same libraries with exactly the same order as before the patch. One breakdown of NSS_LIBS_1 though NSS_LIBS_4 is for Windows and other for the other platforms.
Attachment #8699553 - Attachment is obsolete: true
Attachment #8700064 - Flags: review?(rrelyea)
Comment on attachment 8700064 [details] [diff] [review]
support optional util-only or softoken-only builds - V11

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

r- from myself.

::: cmd/platlibs.mk
@@ +158,5 @@
> +	$(SECTOOL_LIB) \
> +	$(NSS_LIBS_2) \
> +	$(SOFTOKENLIB) \
> +	$(CRYPTOLIB) \
> +	$(NSS_LIBS_3) \

Need to add
	$(NSS_LIBS_4) \
otherwise the build will fail on windows. There we must link with the util and nspr static libraries. New patch comming.
Attachment #8700064 - Flags: review-
Attachment #8700064 - Attachment is obsolete: true
Attachment #8700064 - Flags: review?(rrelyea)
Attachment #8700100 - Flags: review?(rrelyea)
Comment on attachment 8700064 [details] [diff] [review]
support optional util-only or softoken-only builds - V11

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

I'm going to r+ this assuming you fix the issue you've already self-identified. Consider this an r+ for your next patch assuming the only difference is the appropriately added $(NSS_LIB_4) statement.

::: cmd/platlibs.mk
@@ +106,3 @@
>  	$(DIST)/lib/$(LIB_PREFIX)nsspki.$(LIB_SUFFIX) \
>  	$(DIST)/lib/$(LIB_PREFIX)nssdev.$(LIB_SUFFIX) \
>  	$(DIST)/lib/$(LIB_PREFIX)nssb.$(LIB_SUFFIX) \

nit: nsspki, nssdev, nssb are listed twice here.

@@ +158,5 @@
> +	$(SECTOOL_LIB) \
> +	$(NSS_LIBS_2) \
> +	$(SOFTOKENLIB) \
> +	$(CRYPTOLIB) \
> +	$(NSS_LIBS_3) \

Yes, you would need NSS_LIBS_4 here.
Attachment #8700064 - Flags: review+
Attachment #8700100 - Flags: review?(rrelyea)
Comment on attachment 8700100 [details] [diff] [review]
support optional util-only or softoken-only builds - V12

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

r+ OK, this is the patch with the change.
Attachment #8700100 - Flags: review+
pushed to the tip of default branch 
https://hg.mozilla.org/projects/nss/rev/f9c1f174215a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
fixes the duplicates in NSS_LIBS_3 that Bob detected and one more in NSS_LIBS_1 that has been there for quite some time.
Attachment #8700403 - Flags: review?(rrelyea)
Remove remaining duplicate, other ones you had mentioned are now gone.
Attachment #8700403 - Attachment is obsolete: true
Attachment #8700403 - Flags: review?(rrelyea)
Attachment #8701111 - Flags: review?(rrelyea)
Comment on attachment 8701111 [details] [diff] [review]
remove duplicates in NSS_LIB_1

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

r+ sometimes duplicates are there to handle circular dependancies, but I think we've finally expunged those out of NSS. In any case NSS  never depended on ssl so this is clearly saver
Attachment #8701111 - Flags: review?(rrelyea) → review+
^saver^safe.
Elio, there was a review from Bob, AFTER this bug got closed.

Is there anything that remains to be done/checked in?
This is just a suggestion to doublecheck. Thanks.
Flags: needinfo?(emaldona)
Yes, just pushing that last approved change which I just did.
https://hg.mozilla.org/projects/nss/rev/f49c7eac8470
Flags: needinfo?(emaldona)
You need to log in before you can comment on or make changes to this bug.