Closed Bug 421787 Opened 12 years ago Closed 12 years ago

NSS portion of build uses continuation lines in variable definition that is used in an AC_SUBST

Categories

(Firefox Build System :: General, defect, blocker)

defect
Not set
blocker

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: wgianopoulos, Assigned: wgianopoulos)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch patch v1 (obsolete) — Splinter Review
The code that defines NSS_LIBS and NSS_DEP_LIBS in configure.in uses continuation lines in the variable definition that is later used in an AC_SUBST( ...).  This is not allowed because the build system splits the sed script used to process all the AC_SUBST( ... )'s at 90 line boundaries which could end up being in the middle of a continuation line.

THe current situation is that adding any AC_SUBST( ... ) before the ones for NSS_LIBS and NSS_DEP_LIBS will result in a very difficult to figure out build failure.

This issue is currently preventing the code for bug 305782, which is targeted for beta5, from landing.
Flags: blocking1.9?
Attachment #308293 - Flags: superreview?(benjamin)
Attachment #308293 - Flags: review?(kengert)
So the += doesn't work. You'll want to either do:

NSS_DEP_LIBS='...'
NSS_DEP_LIBS=$NSS_DEP_LIBS' ...'
NSS_DEP_LIBS=$NSS_DEP_LIBS' ...'

or (my favourite):

   NSS_DEP_LIBS="\
        "'$(LIBXUL_DIST)/lib/$(LIB_PREFIX)crmf.$(LIB_SUFFIX)'" \
        "'$(LIBXUL_DIST)/lib/$(DLL_PREFIX)smime'$NSS_VERSION'$(DLL_SUFFIX)'" \
        "'$(LIBXUL_DIST)/lib/$(DLL_PREFIX)ssl'$NSS_VERSION'$(DLL_SUFFIX)'" \
        "'$(LIBXUL_DIST)/lib/$(DLL_PREFIX)nss'$NSS_VERSION'$(DLL_SUFFIX)'" \
        "'$(LIBXUL_DIST)/lib/$(DLL_PREFIX)nssutil'$NSS_VERSION'$(DLL_SUFFIX)'" \
        "'$(LIBXUL_DIST)/lib/$(DLL_PREFIX)softokn'$NSS_VERSION'$(DLL_SUFFIX)'

The double quotes around the \ make the continuation handled before assigning to NSS_DEP_LIBS instead of making them part of the value. Make sure there are no spaces between the single and double quotes.
For full effect:

NSS_DEP_LIBS='$(LIBXUL_DIST)/lib/$(LIB_PREFIX)crmf.$(LIB_SUFFIX)'
NSS_DEP_LIBS=$NSS_DEP_LIBS' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)smime'$NSS_VERSION'$(DLL_SUFFIX)'
NSS_DEP_LIBS=$NSS_DEP_LIBS' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)ssl'$NSS_VERSION'$(DLL_SUFFIX)'
NSS_DEP_LIBS=$NSS_DEP_LIBS' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)nss'$NSS_VERSION'$(DLL_SUFFIX)'
NSS_DEP_LIBS=$NSS_DEP_LIBS' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)nssutil'$NSS_VERSION'$(DLL_SUFFIX)'
NSS_DEP_LIBS=$NSS_DEP_LIBS' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)softokn'$NSS_VERSION'$(DLL_SUFFIX)'

I think I prefer the other version, though bugzilla's wrapping doesn't help either one.
Let me try again, and see if I can trick bugzilla into giving me a bit more space to play in:

   NSS_DEP_LIBS="\
       "'$(LIBXUL_DIST)/lib/$(LIB_PREFIX)crmf.$(LIB_SUFFIX)'" \
       "'$(LIBXUL_DIST)/lib/$(DLL_PREFIX)smime'$NSS_VERSION'$(DLL_SUFFIX)'" \
       "'$(LIBXUL_DIST)/lib/$(DLL_PREFIX)ssl'$NSS_VERSION'$(DLL_SUFFIX)'" \
       "'$(LIBXUL_DIST)/lib/$(DLL_PREFIX)nss'$NSS_VERSION'$(DLL_SUFFIX)'" \
       "'$(LIBXUL_DIST)/lib/$(DLL_PREFIX)nssutil'$NSS_VERSION'$(DLL_SUFFIX)'" \
       "'$(LIBXUL_DIST)/lib/$(DLL_PREFIX)softokn'$NSS_VERSION'$(DLL_SUFFIX)'

NSS_DEP_LIBS='$(LIBXUL_DIST)/lib/$(LIB_PREFIX)crmf.$(LIB_SUFFIX)'
NSS_DEP_LIBS=$NSS_DEP_LIBS' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)smime'$NSS_VERSION'$(DLL_SUFFIX)'
NSS_DEP_LIBS=$NSS_DEP_LIBS' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)ssl'$NSS_VERSION'$(DLL_SUFFIX)'
NSS_DEP_LIBS=$NSS_DEP_LIBS' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)nss'$NSS_VERSION'$(DLL_SUFFIX)'
NSS_DEP_LIBS=$NSS_DEP_LIBS' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)nssutil'$NSS_VERSION'$(DLL_SUFFIX)'
NSS_DEP_LIBS=$NSS_DEP_LIBS' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)softokn'$NSS_VERSION'$(DLL_SUFFIX)'
Nope, drat! Anyway, looking at both versions locally, I think the first one is easier to read and understand.
This seems to work on my Mac box:

   NSS_DEP_LIBS=\
' $(LIBXUL_DIST)/lib/$(LIB_PREFIX)crmf.$(LIB_SUFFIX)'\
' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)smime'$NSS_VERSION'$(DLL_SUFFIX)'\
' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)ssl'$NSS_VERSION'$(DLL_SUFFIX)'\
' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)nss'$NSS_VERSION'$(DLL_SUFFIX)'\
' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)nssutil'$NSS_VERSION'$(DLL_SUFFIX)'\
' $(LIBXUL_DIST)/lib/$(DLL_PREFIX)softokn'$NSS_VERSION'$(DLL_SUFFIX)'

as does this, which is the simplest version yet:

   NSS_DEP_LIBS="\
       \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)crmf.\$(LIB_SUFFIX) \
       \$(LIBXUL_DIST)/lib/\$(DLL_PREFIX)smime$NSS_VERSION\$(DLL_SUFFIX) \
       \$(LIBXUL_DIST)/lib/\$(DLL_PREFIX)ssl$NSS_VERSION\$(DLL_SUFFIX) \
       \$(LIBXUL_DIST)/lib/\$(DLL_PREFIX)nss$NSS_VERSION\$(DLL_SUFFIX) \
       \$(LIBXUL_DIST)/lib/\$(DLL_PREFIX)nssutil$NSS_VERSION\$(DLL_SUFFIX) \
       \$(LIBXUL_DIST)/lib/\$(DLL_PREFIX)softokn$NSS_VERSION\$(DLL_SUFFIX)"

The advantage for the former is that the final string doesn't have a whole bunch of white space. But the latter is easier to understand (if you're familiar with sh double quote variable substitution and escaping of $)
Attached patch patch v2 (obsolete) — Splinter Review
This method in this patch has been tested suing the Solaris 7 Bourne shell and I was also able to get the windows build to successfully complete using this method.
Attachment #308293 - Attachment is obsolete: true
Attachment #308301 - Flags: superreview?(jag)
Attachment #308301 - Flags: review?(benjamin)
Attachment #308293 - Flags: superreview?(benjamin)
Attachment #308293 - Flags: review?(kengert)
Comment on attachment 308301 [details] [diff] [review]
patch v2

I think I prefer the simpler version from comment 5.
Attachment #308301 - Flags: superreview?(jag) → superreview-
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #308301 - Attachment is obsolete: true
Attachment #308302 - Flags: superreview?(jag)
Attachment #308302 - Flags: review?(benjamin)
Attachment #308301 - Flags: review?(benjamin)
Attached patch patch v4Splinter Review
Oops!
Attachment #308302 - Attachment is obsolete: true
Attachment #308303 - Flags: superreview?(jag)
Attachment #308303 - Flags: review?(benjamin)
Attachment #308302 - Flags: superreview?(jag)
Attachment #308302 - Flags: review?(benjamin)
Comment on attachment 308303 [details] [diff] [review]
patch v4

r=jag, Benjamin should give (or deny!) the sr
Attachment #308303 - Flags: superreview?(jag)
Attachment #308303 - Flags: superreview?(benjamin)
Attachment #308303 - Flags: review?(benjamin)
Attachment #308303 - Flags: review+
Attachment #308303 - Flags: superreview?(benjamin) → superreview+
Attachment #308303 - Flags: approval1.9?
Comment on attachment 308303 [details] [diff] [review]
patch v4

a1.9+=damons
Attachment #308303 - Flags: approval1.9? → approval1.9+
Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1951; previous revision: 1.1950
done
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
I was just in the middle of landing this, and CVS kept saying there were no changes to commit... lol
I took the easy one (not Bug 305782 ;-).
Comment on attachment 308303 [details] [diff] [review]
patch v4

mcsmurf: boy were you wrong! ;-)

+        \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)smime$NSS_VERSION.\$IMPORT_LIB_SUFFIX) \
+

That of course is missing a ( between $ and IMPORT_...
Comment on attachment 308303 [details] [diff] [review]
patch v4

>+        \$(LIBXUL_DIST)/lib/\$(LIB_PREFIX)smime$NSS_VERSION.\$IMPORT_LIB_SUFFIX) \

just for the record: A ( was missing before IMPORT_LIB_SUFFIX.
With the help of a mess of people on IRC, both my Windows and Linux builds are now building configure locally form configure.in so that i can actually test configure.in patches before I attach them to a bug (as I have always tried to do with all of my patches).

So, this should not happen again (or if it does, I no longer have an excuse). :-)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.