Allow initial DLLFLAGS to be specified on configure command line for Windows builds

RESOLVED FIXED

Status

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch Proposed fix (obsolete) — Splinter Review
As part of various bugs (bug 487396, bug 489975) the way jemalloc (an alternate dll for better malloc performance) is built into Thunderbird has changed.

For Firefox and NSPR/NSS the Firefox configure code sets up the jemalloc definitions in the DLLFLAGS environment variable, and passes it to NSPR/NSS's configure via the environment. This seems to work nicely.

LDAP's configure doesn't have the option for DLLFLAGS being initially set from the environment, so I'd like to add it as I can't see an alternate way of doing so.

Therefore I'd like to add reading the DLLFLAGS from the environment to the configure file. Proposed patch is attached, I'm still testing it at the moment.
Posted patch WIP v2 (obsolete) — Splinter Review
Updated fix, this removes the extra calculation in build.mk for something that is already done in configure and hence allows DLLFLAGS to be used on windows for linking.

This fixes the bug, however I've also noticed that the pdbs are incorrectly generated - PDB:NONE isn't valid on most modern visual studio versions, and actually creates a file called 'NONE'. I'm going to take a look at fixing that as it'd be nice to get debug symbols generated properly for crash stacks (and Mozilla's crash reporting system).
Attachment #375843 - Attachment is obsolete: true
Posted patch The fixSplinter Review
This patch (against CVS head) allows passing DLLFLAGS via the environment, and fixes generation of PDB files when MOZ_DEBUG_SYMBOLS is set.

The basic changes are:

1) Allow DLLFLAGS to be passed via the environment in configure
2) Don't override/extend the debug options in build.mk - just use what configure supplies via DLLFLAGS and OS_DLLFLAGS

Rich - can you review?

Once we're happy with the patch I'd like to check it in and then update the version of the c-sdk that Thunderbird/SeaMonkey are using to the latest - so I'll probably tag a new version with something.
Attachment #376002 - Attachment is obsolete: true
Attachment #376231 - Flags: review?(richm)
Looks good.  I'm not authorized to edit the attachment.  Mark Smith will have to set the review+ flag.
Comment on attachment 376231 [details] [diff] [review]
The fix

Switching reviews to get the ok.
Attachment #376231 - Flags: review?(richm) → review?(mcs)
Attachment #376231 - Flags: review?(mcs) → review+
Comment on attachment 376231 [details] [diff] [review]
The fix

Looks OK (and I trust Rich, who should've been able to do a r+... I am not sure why he was not able to do so)
Checking in build.mk;
/cvsroot/mozilla/directory/c-sdk/build.mk,v  <--  build.mk
new revision: 5.43; previous revision: 5.42
done
Checking in configure;
/cvsroot/mozilla/directory/c-sdk/configure,v  <--  configure
new revision: 5.74; previous revision: 5.73
done
Checking in configure.in;
/cvsroot/mozilla/directory/c-sdk/configure.in,v  <--  configure.in
new revision: 5.68; previous revision: 5.67
done
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 376231 [details] [diff] [review]
The fix

>-        DLLFLAGS='-OUT:"$@"'
>+        DLLFLAGS="$DLLFLAGS -OUT:'$@'"
This doesn't work for me, since $@ is "dummy lib" so this expands to " -OUT:'dummy lib'". Did you mean DLLFLAGS="$DLLFLAGS -OUT:\$@" ?
(In reply to comment #7)
> (From update of attachment 376231 [details] [diff] [review])
> >-        DLLFLAGS='-OUT:"$@"'
> >+        DLLFLAGS="$DLLFLAGS -OUT:'$@'"
> This doesn't work for me, since $@ is "dummy lib" so this expands to "
> -OUT:'dummy lib'". Did you mean DLLFLAGS="$DLLFLAGS -OUT:\$@" ?

Looking at the NSPR configure, this should have been:

DLLFLAGS="$DLLFLAGS -OUT:\"\$@\""

does that work for you?
Neil, please see comment 8.
Yes, that also works. (I don't know why we bother adding the quotes.)
(In reply to comment #10)
> Yes, that also works. (I don't know why we bother adding the quotes.)

I'm not sure either, but I'd rather go with something that we know works.
I've checked in the change as a bustage fix:

Checking in configure;
/cvsroot/mozilla/directory/c-sdk/configure,v  <--  configure
new revision: 5.75; previous revision: 5.74
done
Checking in configure.in;
/cvsroot/mozilla/directory/c-sdk/configure.in,v  <--  configure.in
new revision: 5.69; previous revision: 5.68
done

Tagged a new version: LDAPCSDK_6_0_6B_MOZILLA_RTM

Pulled into comm-central: http://hg.mozilla.org/comm-central/rev/d78acbcbd189
Duplicate of this bug: 242925
You need to log in before you can comment on or make changes to this bug.