66988 - Build change for security/manager/ssl/src/Makefile.in

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P3)

Description

[Colin Blake]

security/manager/ssl/src/Makefile.in

Most other Makefile's define libraries that are required when
creating a DLL (or DSO or whatever you want to call it) in
EXTRA_DSO_LDOPTS. This is how OpenVMS needs it, and not in
EXTRA_LIBS. I am about to submit a patch to this effect.

I guess we need to either conditionalize this
change for OpenVMS only, or if we're going to change it for
everyone, then delete the definition of EXTRA_LIBS.

Someone really needs to make this call and then change the
patch accordingly.

Comment 1

[Colin Blake]

Attachment: patch for security/manager/ssl/src/Makefile.in

Comment 2

[Colin Blake]

Wan-Teh says this one belongs to Javi :-)

Comment 3

[Colin Blake]

Bug #60143 are the NSS 3.2 changes that go along with this PSM fix.

Comment 4

[Colin Blake]

Attachment: Revised patch - see bug #60143 for more details

Comment 5

[Colin Blake]

Wan-Teh has checked in the NSS part of these changes. Can we now get the PSM 
changes in? I know the ssl/src/Makefile.in change in particular might need some 
work, so can we start some discussion?

Comment 6

[Javier Delgadillo]

I'm confused by the latest patch.  It doesn't seem to remove the definition of
EXTRA_LIBS in ssl/src/Makefile.in

Won't this include the NSS libraries twice in the link line?

Also, bug 60143 mentions getting rid of DIST_NSPR, I still see that in this
patch.  Did you fix moz_import or not?

Comment 7

[Colin Blake]

The patch to security/manager/Makefile.in passes the NSPR include directory
obtained from autoconf (NSPR_INCLUDE_DIR) on to the NSS build. This is needed
(I believe) for all platforms that build Mozilla using with-nspr. It was the
only way I could get NSS to know about the location of the NSPR header files.

For other change, I didn't remove EXTRA_LIBS because I didn't need to on
OpenVMS. Because a shareable (DLL) is being created, EXTRA_DSO_LDOPTS is used
but not EXTRA_LIBS. Perhaps this is platform specific; I'm not sure, but its 
why I said that this patch may take some work for compatibility with other
platforms.

OK, I just digged around and found out what's happening with EXTRA_DSO_LDOPTS
and EXTRA_LIBS. OpenVMS has a slightly different MKSHLIB and EXTRA_LIBS are not
included. This is why I needed a patch. The reason OpenVMS doesn't include
EXTRA_LIBS in its MKSHLIB is because if it does then modules such as 
libimg/gifcom end up including some libraries twice since they define the
libraries in both EXTRA_LIBS and EXTRA_DSO_LDOPTS. I guess on other UNIX
platforms this duplication doesn't matter; but for me it does. The fix (a long
time ago) was to just ignore EXTRA_LIBS since it was always a duplication of
what was in EXTRA_DSO_LDOPTS. Until pipnss came along.

So, if we really want to we can leave the libraries defined in both EXTRA_LIBS
and EXTRA_DSO_LDOPTS and there are some modules (but only a minority) doing it
this way. But personally I think it makes more sense to remove the EXTRA_LIBS
definition and just do it via EXTRA_DSO_LDOPTS. Do you want me to resubmit the
patch this way?

The other maybe subtle change here was that in the EXTRA_DSO_LDOPTS definition
I removed the duplicates that appeared in EXTRA_LIBS. As I said above, I can't
link with duplicate libraries.
just define them in EXTRA_DSO_LDOPTS.

Comment 8

[Javier Delgadillo]

If we have to include both variables, I'd rather have something like

EXTRA_DSO_LDOPTS = $(EXTRA_LIBS)

That way we don't have to add a file twice.

Comment 9

[Colin Blake]

No, we don't HAVE to include both variables. If we do, the libs get included 
twice on the command line. I think we should only use EXTRA_DSO_LDOPTS (that
seems to be the Mozilla way; using EXTRA_LIBS is the NSS way).

And don't forget I need to remove the duplicates. It that OK for the other 
platforms?

Comment 10

[Colin Blake]

Attachment: Revised patch; EXTRA_LIBS is gone and CORECONF_DIST not used any more.

Comment 11

[Colin Blake]

Any chance of this getting checked in?

Comment 12

[Javier Delgadillo]

setting priority

Comment 13

[Colin Blake]

Attachment: revised patch for M0.9 PSM2 code

Comment 14

[Javier Delgadillo]

Some of the libraries you take out will be required as more functionality lands.

Comment 15

[Colin Blake]

Nope. I am only removing duplicate libraries.

Comment 16

[Javier Delgadillo]

This link list was taken from the NSS cmd directory and I believe the multiple
inclusion of some libraries are necessary on some platforms due to circular
dependencies within the NSS libraries.  I'll give this a whirl, but I'm not sure
if this will work on all platforms.

Comment 17

[Colin Blake]

Mozilla itself had circular dependancies in its libraries a LONG time ago. They
were resolved/fixed because some O/S's don't allow such ambiguous coding
conventions.

Comment 18

[David P. Drinan]

Target Milestone --- -> 2.0.

Comment 19

[Colin Blake]

javier, did you ever get a chance to try my Makefile patch with the duplicates
removed? I'd really like to get this checked in if at all possible. I'm
attaching a new patch for use with the latest code.

Comment 20

[Colin Blake]

Attachment: For use with current code

Comment 21

[Javier Delgadillo]

Your latest patch is out of date.  We no longer have CORECONF_* variables in
mozilla/security/manager/ssl/src/Makefile.in

Comment 22

[Colin Blake]

> Your latest patch is out of date.  We no longer have CORECONF_* variables in
> mozilla/security/manager/ssl/src/Makefile.in

Huh? Please see my latest patch.

Comment 23

[Colin Blake]

I'd really like to get this checked in one day. Having to reapply the same
fix after each download is getting to be a real bore.

Comment 24

[Stephane Saux]

Mass reassigning target to 2.1

Comment 25

[Stephane Saux]

remove nsenterprise

Comment 26

[Stephane Saux]

Moving all P3 and P4 bugs targetted to 2.1 to future.

Comment 27

[Colin Blake]

Future? Come on guys, having to apply the same patch to each release is a 
royal pain. I submitted this patch MONTHS ago, can we please get it (or some
derivative) checked in in the foreseeable future?

Comment 28

[John Unruh]

Mass assigning QA to ckritzer.

Comment 29

[Javier Delgadillo]

Sorry about this.  Have you tested this patch on Linux?  Or any other platform
besides OpenVMS?  Verify that it works on Linux at a bare minimum and you've got
r=javi.

Comment 30

[Colin Blake]

Unfortunately I don't have any other build systems on which I can test the 
patch. 

However, I'm just in the process of testing a different/better patch that is
VMS specific. If this new patch works I'll post that for approval.

Comment 31

[Colin Blake]

The next patch replaces my previous patches. It is based on the current
tip (0.9.4) and is OpenVMS-specific. The way its written I shouldn't have
to change it even if the NSS_LIBS list changes. Hopefully this will more
acceptable.

Comment 32

[Colin Blake]

Attachment: Patch for 0.9.4 tip (replaces all previous patches)

Comment 33

[Javier Delgadillo]

r=javi

Comment 34

[Colin Blake]

Thanks for the r= - now who do I get an a= from (assuming that I am allowed
to check this in myself)?

Comment 35

[Javier Delgadillo]

get sr= from a super reviewer, then I'll check it in.

cls would be a good bet.

Comment 36

[Colin Blake]

cls - can I get an sr= on this one please?

Comment 37

[cls]

sr=cls

Comment 38

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

a=dbaron on behalf of drivers, since the fix clearly affects only OpenVMS.

Comment 39

[Colin Blake]

We have driver approval. Any chance we can get this one in before
we branch for 0.9.4? I just tried to check it in myself but I don't
have privs to check in to the security partition.

Comment 40

[Ian McGreer]

checked in before 0.9.4 branch

Comment 41

[bsharma]

Build 2001-09-04-Branch on WinNT

Marking verified as per above developer comments.