Closed Bug 278132 Opened 20 years ago Closed 3 years ago

NSS makefile dependencies broken, mkdepend missing

Categories

(NSS :: Build, defect, P5)

Product:
NSS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: nelson, Assigned: nelson)

References

Details

Attachments

(2 files, 2 obsolete files)

patch as checked in
6.55 KB, patch
Details | Diff | Splinter Review
Carry over ifparser.c change from mozilla-central, and rename emxinc in main.c (as checked in)
2.21 KB, patch
Details | Diff | Splinter Review 
The "coreconf" system of Makefiles has extensive support for dynamic 
contruction of makefile dependencies.  grep the *.mk files in 
coreconf for the string MKDEPEND to see that support.  The idea is
that one pulls the sources, then in the nss directory, one makes the 
"export" target, and then makes the "depend" target.  This creates a 
dependency file in each directory (IIRC).  

However, this feature depends on a program named mkdepend.  
The coreconf directory has a subdirectory named mkdepend.
Prior to the opening of the NSS source code, that directory contained
the portable source to the program that generated the makefile 
dependencies.  Makeing the target "all" in coreconf, or making the 
target build_coreconf in nss, would build the mkdepend program, and 
thereafter, making the "depend" target would create the dependnecies
files.  This worked on all unix/linux platforms and on windows.  

When the NSS source was opened, the source for mkdepend was not carried 
forward into the open source repository due to some concerns about license. 
IIRC, the mkdepend source has a BSD license, and someone thought we could 
or should not put such source into the mozilla/security tree, but rather ALL
source in the mozilla tree should be MPLed.  Consequently, the makefile
dependencies for NSS have been broken ever since the source was opened. :(

If possible, I would like to see mkdepend restored to coreconf, perhaps 
with its BSD license intact.  Comments?  Opinions?
This is fine.
Taking bug.
Assignee: wtchang → nelson
QA Contact: wtchang → build
Gerv, any comments on the proposal to put the c sources for the "mkdepend" 
program into NSS, with the original BSD license statements intact (and no 
MPL boilerplate) ?
Could someone post a copy of the licence here? There's almost certainly no problem with this (we have other BSD code in the tree) but I'd like to check to be sure.

Gerv
Priority: -- → P5
make seamonkeey-1.0.7 elimate this error:

  make[3]: Entering directory `/src/src/mozilla/security/coreconf'
  syntax error at -e line 3, near "while"
  syntax error at -e line 7, near "}"
  Execution of -e aborted due to compilation errors.

which is casued by coreconf/rules.mk, line
  open(MD,"< $(DEPENDENCIES)")

i guess the make variable $(DENPENDENCIES) was replaced by something like '?/.md', but the file cannot be found.

also config/mkdepend has nothing but a Makefile, that means the mkdepend command was not performed, in chain, the .md file was not created.

so i wanna is this a bug or i missed something?

i'm building in a seperate dir other than source dir.

thanks.
Comment 5 is irrelevant to this bug.  See bug 325148.
Please add comments to the appropriate bug.
(In reply to comment #4)
> Could someone post a copy of the licence here? There's almost certainly no
> problem with this (we have other BSD code in the tree) but I'd like to check to
> be sure.

The source code is already present at mozilla/config/mkdepend
I conclude it's not a problem to add another copy to NSS (if we intend that NSS has its own dep system, independent of the mozilla build).
This bug should be fixed with a high priority.

Last Friday I tried to upgrade Firefox to a newer snapshot, and because of the missing dependencies all the tinderbox build machines failed.
During the last couple of days Nelson and I worked on this.

According to my test, and repeating my "landing experience" locally, the dependency system fixed it for me. (With changes in my local tree to make NSS dependencies work, and having copied the mkdepend executable to the path.)

The trouble so far is building the mkdepend.exe itself as part of the NSS build process. While it easily builds on Linux, more work is necessary to get it built as part of the NSS build process.

We learned that building on Windows requires knowledge from the autoconfig-run produced in mozilla/config/autoconf.mk, for example we need -DNO_X11 and according to Nelson we need even more.

So, a solution to this bug requires:
- copy mozilla/config/mkdepend to mozilla/security/coreconf/mkdepend
- make changes to the NSS build system to allow building of mkdepend
  on all platforms
- make changes to the NSS makefiles to run mkdepend and use its output
Attached patch patch v 0.1 (obsolete) — — Splinter Review 
I decided to look at what it would take to get mkdepend back into coreconf.
The original issues were:
a) licensing - tri-license versus MIT or BSD.  
b) Windows - it just didn't work on Windows very well or at all.

So I took at stab at this.  I copied the current mkdepend files from 
mozilla/config/mkdepend to mozilla/security/coreconf/mkdepend, and went to 
work making it build and work well on Windows.  I have that working now.

Here is an explanation of the attached patch.  This patch appears to be a 
patch to the code in mozilla/config/mkdepend, but I do NOT propose to change
any code in that directory.  Instead, I propose to copy the files from 
mozilla/config/mkdepend to mozilla/security/coreconf/mkdepend and commit them, making coreconf's copy be a copy of the other one.  THEN I propose to apply
this patch to the files in mozilla/security/coreconf/mkdepend,

I know this works when built with MSVC on Windows XP (Win32).  I haven't yet 
checked to see if it is satisfactory on other platforms.  I'd appreciate help
with that testing.
Comment on attachment 361923 [details] [diff] [review]
patch v 0.1

I invite review comments from anyone who cares, especially Wan-Teh.
Attachment #361923 - Flags: review?(wtc)
Attachment #361923 - Flags: review?(wtc) → review+
Comment on attachment 361923 [details] [diff] [review]
patch v 0.1

r=wtc.

1. Makefile

>+CFLAGS 		+= -DNO_X11

In coreconf, it's better to add -DXXX to
DEFINES instead.

>+CFLAGS     += -DINCLUDEDIR=\"/usr/include\" -DOBJSUFFIX=\".$(OBJ_SUFFIX)\"

Same as above -- it's better to add these to
DEFINES instead.  Also, /usr/include is a Unix
specific pathname.

2. Makefile.in

>-ifdef GNU_CC
>-MODULE_OPTIMIZE_FLAGS = -O3
>-else
>-ifeq ($(OS_ARCH),SunOS)
>-MODULE_OPTIMIZE_FLAGS = -fast
>-endif
>-endif
>-
>-ifeq ($(OS_ARCH),WINNT)
>-ifndef GNU_CC
>-MODULE_OPTIMIZE_FLAGS = -Ox
>-endif
>-endif

We probably should do the same thing (enable
compiler optimizations) in Makefile.

>-ifdef GNU_CC
>-_GCCDIR		= $(shell $(CC) -print-file-name=include)
>-HOST_CFLAGS	+= -DPREINCDIR=\"$(_GCCDIR)\"
>-endif

You didn't carry this over to Makefile.  Not
sure what the consequences would be.

>-$(HOST_OBJS): def.h ifparser.h imakemdep.h

We may want this.

3. imakemdep.h

Do we really need to list all the compiler predefined
macros?  It's impossible to list all the predefined
macros for all compilers on all platforms.

_X86, _X86_, _X86_64, and _X86_64_ may not be predefined
macros.  They may be defined by a Windows system header.

4. main.c

> 	if (bol && whitespace) {
>+	    if (!strstr(filep->f_name, INCLUDEDIR))
> 		warning("%s:  non-portable whitespace encountered at line %d\n",
> 			filep->f_name, lineno);
> 	}

I don't understand this change.  Also, rather than
using nested ifs, you should add the new test to the
original if:
   if (bol && whitespace && !strstr(filep->f_name, INCLUDEDIR)) {
Wan-Teh, In reply to comment 12,
1. s/FLAGS/DEFINES/
  OK, I'll try it, and if it works, I'll do that
2. optimization
  I removed that stuff because it uses make variables that are not used
  in coreconf.  I can look into ensuring that mkdepend is built with 
  optimizations for gcc, MSVC and solaris
3. I was unable to get most code to build on Windows until I defined many of 
  MSVC's pre-defined symbols.  I found Microsoft's official list of them at
  http://msdn.microsoft.com/en-us/library/b0084kay.aspx
4. about INCLUDEDIR:
  - yes, /usr/include is a Unix location
  - INCLUDEDIR is not used at all on WINDOWS or OS2.  On those platforms,
    the locations of the system's root include directories are specified 
    in environment variables.
  - mkdepend is derived from lint, and its display of the warning about
    lines whose first non-blank character is a '#' after column 1 is a 
    lint-ish warning.  Frankly, I think that such warnings have no place
    in mkdepend at all, but I absolutely see no point in having it 
    complain about such issues in SYSTEM header files.  So, the line I 
    added, which calls strstr, determines if the source file being used
    comes from INCLUDEDIR, and if so, it suppresses the lint warning.
    Note that this entire section of code (quoted above) is ifdeff'ed 
    out on Windows because trying to identify the system include 
    directories on Windows and OS2 is not easy, and would require a MUCH
    bigger change.  Perhaps I should simply remove the section of code
    quoted above entirely on all platforms.  Would you prefer that?
Checking in Makefile;    initial revision: 1.1
Checking in imakemdep.h; new revision: 1.3; previous revision: 1.2
Checking in main.c;      new revision: 1.3; previous revision: 1.2
On second thought, I think it's OK to compile
mkdepend without optimizations because the NSS
source tree is small.
Comment on attachment 362341 [details] [diff] [review]
patch as checked in

Should we contribute your changes to imakemdep.h and main.c
back to mozilla/config/mkdepend or the latest copy in mozilla-central?

I'll compare what you checked in to coreconf with the copy in
mozilla-central.

It's still not clear why the predefined macros need to be
listed, but you don't need to explain it to me.  Do we need to
list, for example, all the predefined macros of GCC and Sun
Studio 12?
http://gcc.gnu.org/onlinedocs/cpp/Predefined-Macros.html
http://docs.sun.com/app/docs/doc/819-5267/6n7c46edt?a=view#bkaoe
The only difference between our mkdepend and the config/mkdepend
in mozilla-central is blassey's change to ifparser.c:

  bug 463561 - mkdepend crashes while compiling freetype
  r=bsmedberg a191=beltzner

http://hg.mozilla.org/mozilla-central/log/47b4599e95f0/config/mkdepend/ifparser.c

I also noticed the variable name 'emxinc' in the part of
main.c that we modified.  Since EMX means the version of
GCC for OS/2, the name 'emxinc' is no longer appropriate
now that we also use it for MSVC.  So I renamed it
'includeenv', for "the INCLUDE environment variable".
Attachment #362411 - Flags: review?(nelson)
Comment on attachment 362411 [details] [diff] [review]
Carry over ifparser.c change from mozilla-central, and rename emxinc in main.c

If you think 'includeenv' is a little hard to read,
'includepath' would be another good name for that
variable.
Comment on attachment 362411 [details] [diff] [review]
Carry over ifparser.c change from mozilla-central, and rename emxinc in main.c

Wan-Teh, 
I believe that, in general, we want mkdepend to "compiler" the very
same lines of code that the actual compiler will compile, with the 
same ifdefed code included or excluded.  This is especially critical
when ifdefs control the #include statements.  We have lots of code
in NSS that attempts to #include files based on compiler-defined 
symbols, and was not #including the right files until I made this 
change.  

I believe it is desirable to try to be all-inclusive of the 
compiler's built-in symbols, so as to avoid surprises at some later 
time when some new code uses some pre-defined symbol previously 
unused.  So, when I found MS's official list of pre-defined symbols,
I used it.  

As for whether we need to do more for gcc or other compilers, maybe.
But AFAIK, mkdepend is already considered to be working properly on
other platforms.  My goal for THIS bug was merely to get it working
on Windows.  If it turns out not to work on some other platform, 
then we should fix symbols on those in another bug.

BTW, as you would expect, mkdepend adds dependencies of .o's on .c's
and .h's, but does not add dependencies for .a/.lib files, nor for 
.dll/.so files, etc.  So, I found that in some cases, when I modified
a file, the .o would get rebuilt, but the .so/.lib that incorporates
that .o did not get rebuilt.  So, we have some more work to do to 
fix dependencies in NSS, but that should be the subject of another bug.  

Kai, do you have some way to test this with FF builds?  
I seem to recall that you implemented some feature that made all of 
NSS seem to depend on some pseudo-header.  Is it time to undo that?

Do we need to make the "depend" Makefile target be invoked by 
(be a dependency of) some other target?
Attachment #362411 - Flags: review?(nelson) → review+
We also need to undo the old workaround in PSM's top-level
makefile.  At
http://mxr.mozilla.org/mozilla-central/source/security/manager/Makefile.in#420,
we need to pass the 'depend' target on to NSS:

420 clean clobber clobber_all realclean distclean depend::
421         $(MAKE) -C boot $@
422         $(MAKE) -C ssl $@
423         $(MAKE) -C locales $@
424 ifdef MOZ_XUL
425         $(MAKE) -C pki $@
426 endif
427 ifndef MOZ_NATIVE_NSS
428         $(MAKE) -C $(topsrcdir)/security/coreconf $(DEFAULT_GMAKE_FLAGS) clean
429         $(MAKE) -C $(topsrcdir)/security/nss/lib $(DEFAULT_GMAKE_FLAGS) clean
430 ifndef SKIP_CHK
431         $(MAKE) -C $(topsrcdir)/security/nss/cmd/lib $(DEFAULT_GMAKE_FLAGS) clean
432         $(MAKE) -C $(topsrcdir)/security/nss/cmd/shlibsign $(DEFAULT_GMAKE_FLAGS) clean
433 endif
434 endif

Right now 'depend' is changed to 'clean' before being passed
on to NSS.
I checked in this patch on the NSS trunk (NSS 3.12.3).
(I decided to rename the 'emxinc' variable 'includepath'.)

Checking in ifparser.c;
/cvsroot/mozilla/security/coreconf/mkdepend/ifparser.c,v  <--  ifparser.c
new revision: 1.3; previous revision: 1.2
done
Checking in main.c;
/cvsroot/mozilla/security/coreconf/mkdepend/main.c,v  <--  main.c
new revision: 1.4; previous revision: 1.3
done
Attachment #362411 - Attachment is obsolete: true
Attachment #361923 - Attachment is obsolete: true
Attachment #362341 - Attachment description: patch as committed → patch as checked in
Once you declare this patch as fixed, we should undo bug 474473.
We need a PSM bug filed for the PSM change.
The good news is that the mkdepend program is now in NSS, can be built on all
supported platforms, and is thought to work on all supported platforms.
I'm not sure whether it works in cross-platform development, such as building
for Windows on a Linux box.  If you gmake the "depend" target in mozilla/security/nss, it will build mkdepend and then run it over the tree.

Presently, it is necessary to explicitly build the "depend" target, and it
must be done after the export target, e.g. 
   gmake export depend libs

The bad news is that, on at least one supported platform (Windows, and maybe 
others) after you've made the depend target, the presence of the file of 
generated dependencies somehow causes the default target to change from "all"
to the first file in the generated dependency file.  :(  

Also, the generated dependencies includes the .o/.obj files that must be 
build from .c files, but does not include dependencies of libraries on 
object files or other libraries.  That info must still be manually entered
in NSS Makefiles.

So, there's more makefile work to be done before this bug is resolved/fixed.
See Also: → 1438431

This was changed by https://bugzilla.mozilla.org/show_bug.cgi?id=1438431

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: