If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

ssltunnel fails to build in system-nspr / system-nss environments

RESOLVED FIXED

Status

Testing
Mochitest
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

10 years ago
In some environments the NSPR and NSS libraries are not used from the mozilla tree, but instead installed as system libraries, both both headers and libraries, e.g. /usr/include/nss3

In such an environment we built firefox and failed, because ssltunnel seems to be hardcoded to use the header files (and probably library files) from in-tree.

The ssltunnel makefile should make use of the information provided by the configuration scripts.

There are NSPR_CFLAGS, NSPR_LIBS, NSS_CLFAGS, NSPR_LIBS, etc.

For reference see the use of those flags in mozilla/configure.in and mozilla/security/manager/ssl
(Assignee)

Updated

10 years ago
Depends on: 426867

Comment 1

10 years ago
You're welcome to fix it, but ssltunnel is test-only code so I think --disable-tests should avoid building it altogether (and you're not building the RPMs with tests, right?)
(Assignee)

Comment 2

10 years ago
I think we used --disable-tests but still failed...

According to my logfile (non-rpm) of an opt build (--disable-tests) it indeed built ssltunnel.cpp
It's using {NSPR,NSS}_LIBS, just not the CFLAGS bits. Should be easy enough to fix. I kind of thought it was in a test-only block as well, but apparently mochitest gets built regardless of whether tests are enabled. There is a --disable-mochitest configure flag that will disable it though.
Yeah, we were using --disable-tests.  --disable-mochitest lets the build proceed, though it seems counterintuitive that --disable-tests doesn't disable all tests.
Version: 1.8 Branch → Trunk
(Assignee)

Comment 5

10 years ago
Created attachment 316851 [details] [diff] [review]
patch v1

like this?

Copied from mozilla/security/manager/ssl/src/Makefile.in
Attachment #316851 - Flags: superreview?(benjamin)
Attachment #316851 - Flags: review?(ted.mielczarek)

Updated

10 years ago
Attachment #316851 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Comment 6

10 years ago
Chris, do you have a chance to test this in one of your next RPM builds?
Comment on attachment 316851 [details] [diff] [review]
patch v1

Do we need to fix this for NSPR_CFLAGS as well? Also, I think you can remove the REQUIRES lines if you explicitly use the _CFLAGS.
Attachment #316851 - Flags: review?(ted.mielczarek) → review+

Updated

10 years ago
Duplicate of this bug: 433198
Whiteboard: [RC2?]
Comment on attachment 316851 [details] [diff] [review]
patch v1

The Linux distro guys have taken this patch upstream, as without out, their system-nss build (such as for Songbird) breaks, so if we have an RC2, can we take this build fix? Thanks.
Attachment #316851 - Flags: approval1.9?

Updated

10 years ago
Duplicate of this bug: 436088
Assignee: nobody → kaie
Not taking churn for unused test code with a workaround, and we're not taking patches _because_ distros decided to do so themselves already.  3.0.1 may welcome it more.
Flags: wanted1.9.0.x?
Whiteboard: [RC2?]
Comment on attachment 316851 [details] [diff] [review]
patch v1

This is NPOTB. No reason why it can't land right now.
Attachment #316851 - Flags: approval1.9?
Not true, it gets built unless you --disable-mochitests. It doesn't affect anything we ship though.

Comment 14

10 years ago
Do we really need a bug which says that --disable-tests should disable all tests? I can file one if it is needed....
(In reply to comment #14)
> Do we really need a bug which says that --disable-tests should disable all
> tests? I can file one if it is needed....

Certainly nothing should get fixed without a patch in a bug so yes please do.
I would certainly review that. I'm not sure why mochitest falls outside of --disable-tests.
(Assignee)

Comment 17

9 years ago
Created attachment 323598 [details] [diff] [review]
Patch v1 plus NSPR added

(In reply to comment #7)
> Do we need to fix this for NSPR_CFLAGS as well?

That's a good idea, updated patch attached.


> Also, I think you can remove
> the REQUIRES lines if you explicitly use the _CFLAGS.
 
Does it harm to keep the REQUIRES lines?
If not, I guess it would be nice to keep those entries for clarity.
Attachment #323598 - Flags: approval1.9?
Pushed to mozilla-central.
Should still get this on 1.9.0 once Firefox 3 ships.
Comment on attachment 323598 [details] [diff] [review]
Patch v1 plus NSPR added

Moving approval request out to an active flag. Kai, I assume you still want this for 1.9.0.x?
Attachment #323598 - Flags: approval1.9? → approval1.9.0.2?
Comment on attachment 323598 [details] [diff] [review]
Patch v1 plus NSPR added

Clearing approval request. I'll wait for Kai to say that he needs this.
Attachment #323598 - Flags: approval1.9.0.2?
(Assignee)

Comment 22

9 years ago
Comment on attachment 323598 [details] [diff] [review]
Patch v1 plus NSPR added

Yes we should take this for 1.9.0, thanks!
Attachment #323598 - Flags: approval1.9.0.3?
(Assignee)

Comment 23

9 years ago
Marking fixed per comment 18.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Oh, is this just a test change? If so, it doesn't require approval to land. I didn't look at the patch before...
Flags: wanted1.9.0.x?
Comment on attachment 323598 [details] [diff] [review]
Patch v1 plus NSPR added

Approved for 1.9.0.4, a=dveditz for release-drivers
Attachment #323598 - Flags: approval1.9.0.4? → approval1.9.0.4+
Comment on attachment 323598 [details] [diff] [review]
Patch v1 plus NSPR added

Didn't make 1.9.0.4. Renominating for 1.9.0.5, though if this is a test change it can just land when the tree is open for 1.9.0.5 development.
Attachment #323598 - Flags: approval1.9.0.5?
Attachment #323598 - Flags: approval1.9.0.4-
Attachment #323598 - Flags: approval1.9.0.4+
Comment on attachment 323598 [details] [diff] [review]
Patch v1 plus NSPR added

Approved for 1.9.0.5, a=dveditz for release-drivers
Attachment #323598 - Flags: approval1.9.0.5? → approval1.9.0.5+
Comment on attachment 323598 [details] [diff] [review]
Patch v1 plus NSPR added

Please request approval when you intend to land this.
Attachment #323598 - Flags: approval1.9.0.5+
Attachment #323598 - Flags: approval1.9.0.4-
Component: Testing → Mochitest
Product: Core → Testing
QA Contact: testing → mochitest
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.