Closed Bug 105271 Opened 18 years ago Closed 17 years ago

Compile source files with absolute pathnames to ease debugging

Categories

(NSPR :: NSPR, defect, P3)

x86
OS/2
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julien.pierre, Assigned: julien.pierre)

Details

Attachments

(2 files, 31 obsolete files)

2.13 KB, patch
Details | Diff | Splinter Review
1.76 KB, patch
Details | Diff | Splinter Review
The IBM debugger can't find source files on OS/2 if the objects are built with
the -Fo option, as they are in the OS/2 build.
The only exception is if an absolute path for the source is used.
I'm proposing changes to rules.mk that use "shell pwd" to fix the problem. See
attached patch.
Component: Build → NSPR
Product: NSS → NSPR
Version: 3.4 → other
Target Milestone: --- → 4.2
Julien,

I have to say your patch is too ugly for my taste.
Isn't there a better way to make the IBM debugger
find source files?
Assignee: wtc → jpierre
Wan-Teh,

It's the only solution that I'm aware of with the IBM compiler. I only got this 
to work through much trial and error a few weeks ago and I have been building 
that way since. The only other way that the debugger found the source files was 
if the object files were placed in the same directory as the source files (ie. 
not in an OBJ_DIR directory). I don't think we want to try that.

FYI, I think doing this would also fix problems on other platforms where source 
files aren't automatically found by the debugger (eg. Windows). I find that it 
saves a lot of time in debugging to be able to see the code right away without 
having to locate files individually accross units and types paths manually.

I'm not sure the changes work when using vpath or builds where the Object tree
is split out.

bob
Bob, it works fine for those cases too - all the change does is give a 
fully-qualified path to the source files to the compiler, it does not change 
anything for the target directory where the files are built.
I have done the NSPR build with autoconf on OS/2 successfuly using these changes 
and all the objects are in mozilla/nsprpub/OS22.45_icc_DBG.OBJ/xxx , along 
with the Makefiles, separated from the source tree.
Have you tried a browser objtree build?
I just tried a browser objtree build, this change does break it - the fully 
qualified path is there twice in the compiler command.

However, in the NSS build with NSPR_AUTOCONF, the path to the NSPR configure 
script is a relative path (../../.. etc). This is what's passed to the compiler 
to find the source files. As a result the debugger does not find the source. 
Maybe what needs to be changed is the build_nspr rule in the NSS build instead, 
to pass a fully-qualified path to the NSPR configure on OS/2.

I confess that I checked in a change to the NSS 
mozilla/security/coreconf/rules.mk in coreconf last week that does the same 
thing (shell pwd prefix passed to the compiler) to solve the same problem. As 
far as I know, NSS coreconf build does not do objtree builds yet, so perhaps 
that change may not hurt that build. Bob, what is the vpath you were referring 
to ? Is this an issue for NSS or not ?
Did you try a build with BUILD_TREE set to some fixed path? your changes only
work if the source is always built with a relative path (which isn't true for
sources with are constructed as part of the build processes and stored in a
BUILD_TREE). Michael, do you have any suggection on how to make the debug
environment work in these cases?

bob
Bob,

What target / Makefile should I try building with BUILD_TREE set ? Is that set 
in the environment or passed to gmake ?
Either in the Environement variable or on the command line:

bob
Marking NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: patch, review
Component: NSPR → Build
Product: NSPR → NSS
Target Milestone: 4.2 → 3.4
Version: other → 3.3.1
A better fix for the NSPR part of the build is to pass absolute paths to the
configure script for NSPR. This is done in mozilla/security/nss/Makefile the
following way :

ifeq ($(OS_ARCH),OS2)
NSPR_PREFIX = $(shell pwd)/../../dist/$(OBJDIR_NAME)
else
NSPR_PREFIX = $$(topsrcdir)/../dist/$(OBJDIR_NAME)
endif

I have just checked that fix in . This way the NSPR rules.mk does not need to be
changed.

However, the NSS coreconf rules.mk has the shell pwd hack in it right now and
that will break the browser build once 3.4 is integrated in the client, so one
more fix is needed.
On second thought, I think this is better:

#
# Some pwd commands on Windows (for example, the pwd
# command in Cygwin) return a pathname that begins
# with a (forward) slash.  When such a pathname is
# passed to Windows build tools (for example, cl), it
# is mistaken as a command-line option.  Therefore on
# Windows we use a relative pathname as NSPR's prefix.
#
ifeq ($(OS_ARCH),WINNT)
NSPR_PREFIX = $$(topsrcdir)/../dist/$(OBJDIR_NAME)
else
NSPR_PREFIX = $(shell pwd)/../../dist/$(OBJDIR_NAME)
endif

> However, the NSS coreconf rules.mk has the shell pwd
> hack in it right now and that will break the browser
> build once 3.4 is integrated in the client, so one
> more fix is needed.

You may want to back that shell pwd hack out until it
is fully working.  Although it is okay to check in
work in progress into NSS, we should keep coreconf in
good condition at all times because it is used by
other products too (DBM and JSS).
Wan-Teh,

There may actually exist a more clever fix involving a check for $(find
':',$(shell pwd)). This was suggested by Javier. I'm going to try it.

That way even on NT we can identify that the tool is not returning a drive
letter and  will break the code, and use the relative path only in that case.
It's always beneficial to use the absolute path, even for the Microsoft debugger
to find the source files on NT.

As far as the changes to rules.mk I made, they are all within ifdef
XP_OS2_VACPP, and I'm presumed to be the only user of that on the tip for 3.4 at
this time. I will make sure this gets fixed properly before RTM.

> As far as the changes to rules.mk I made, they are all within ifdef
> XP_OS2_VACPP, and I'm presumed to be the only user of that on the tip for 3.4 at
> this time. I will make sure this gets fixed properly before RTM.

My point is that coreconf can be considered a component in
its own right.  It is not only used by NSS but also used
by JSS and DBM.  There may be JSS or DBM releases before
NSS 3.4 RTM, and they would pick up a coreconf with the
work-in-progress shell pwd hack.

I have checked in a change to mozilla/security/coreconf/rules.mk that uses the
shell pwd in one of the C compile rules but not the other, so that the build
will work in both the objtree and coreconf cases and prepend the path as needed.

The mozilla/security/nss/Makefile still needs to be optimized in the way
suggested by Wan-Teh so that all platforms that can will use absolute paths in
the build.
FYI, about the pwd issue, on OS/2 , it is part of the build instructions that a
certain version of pwd.exe must be used, one that returns a drive letter.
Otherwise the mozilla OS/2 build is unsupported. Would it be a reasonable
requirement on NT as well ?In that case we could just use the shell pwd without
a test. Otherwise we need to test for a drive letter only on NT and use relative
path if there isn't one.

Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
The following is now checked into the mozilla/security/nss/Makefile and should
work regardless of the version of pwd used. I'm marking this defect resolved.

#
# Some pwd commands on Windows (for example, the pwd
# command in Cygwin) return a pathname that begins
# with a (forward) slash.  When such a pathname is
# passed to Windows build tools (for example, cl), it
# is mistaken as a command-line option.  If that is the case,
# we use a relative pathname as NSPR's prefix on Windows.
#

USEABSPATH=':'
ifeq ($(OS_ARCH),WINNT)
USEABSPATH=$(findstring :,$(shell pwd))
endif
ifeq ($(USEABSPATH),':')
NSPR_PREFIX = $(shell pwd)/../../dist/$(OBJDIR_NAME)
else
NSPR_PREFIX = $$(topsrcdir)/../dist/$(OBJDIR_NAME)
endif

Unfortunately those changes don't suffice for NSPR to be built debug. I still
had some other changes in my tree. Without them the absolute paths are still not
used in the compile step.

I have implemented a new patch for the NSPR rules.mk that conditionally calls
shell pwd if the source file does not contain a colon, assuming a drive letter.
Wan-Teh, I hope this new patch is not too ugly for you ... Please review.
Status: RESOLVED → REOPENED
Component: Build → NSPR
Product: NSS → NSPR
Resolution: FIXED → ---
Target Milestone: 3.4 → 4.2
Version: 3.3.1 → other
Comment on attachment 55652 [details] [diff] [review]
new proposed patch with conditional shell pwd

1. The statement
       USEABSPATH=$(findstring :,$<)
sets USEABSPATH as a shell variable.  Therefore
the makefile variable USEABSPATH will always be
undefined, and you will always end up using the
first compilation command (which calls $(shell pwd)).

I suggest removing the use of USEABSPATH and just use
the first compilation command.

2. This patch will make the 'pwd' command a required tool for building NSPR.  I assume this is not a problem for OS/2
developers.
Attachment #55652 - Flags: needs-work+
Wan-Teh,

I have attached a new patch which actually works properly.

The mozilla/security/nss/Makefile was using the same incorrect approach and I am
correcting it too.
We can't use shell pwd all the time because in some cases (browser objtree
build) we ended up with 2 paths. The check is to prevent that from happening so
as to only call shell pwd when necessary.
As far as the OS/2 build, pwd.exe returning a drive letter is actually required
by the Warpzilla build. See http://www.mozilla.org/ports/os2/setup.html .
Comment on attachment 56200 [details] [diff] [review]
new patch not using intermdiate variable

I think this patch works.  Two suggested changes.

1. Add a comment explaining why you look for : in the source file name and
why you want to construct the full path name
of the source file.

2. I believe the / to \ substitution is not necessary here because 
the full pathname always starts with a
drive letter, never a /.
Attachment #56200 - Flags: needs-work+
Attachment #55652 - Attachment is obsolete: true
Attachment #56201 - Attachment is obsolete: true
Unfortunately, the change for rules.mk does not work even with this patch. The :
is never found in the findstring check, and the shell pwd always gets called.
This results in my standalone build working, but not the NSPR build in the
browser when these changes are in. I'm at a loss as to why this does not work.

Note that the similar check in mozilla/security/nss/Makefile does work ...
I found why the patch wasn't working.
Apparently it is not possible to use the $< macro as part of an "ifeq" test. The
macro always resolves to an empty string, and therefore the findstring does not
match. It may be a gmake bug.

This explains why the check in mozilla/security/nss/Makefile works - in that
case, I'm searching $(shell pwd) for a colon, rather than searching $< as I'm in
rules.mk .
Unfortunately while I identified the cause of the problem, I have not found a
solution for this. I need to be able to look at the filename of the file
argument passed to the compile rule and make a decision based on whether it
contains a colon ...

Priority: -- → P2
It's not a gmake bug.

From gmake doc, about ifeq conditionals :

Conditionals control what make actually "sees" in the makefile, so they cannot
be used to control shell commands at the time of execution.

There is a section on functions, and it looks like they might do what is needed
here, but they work differently.
The $(if condition,then,else) solves the problem.
Unfortunately, it is a new feature of gmake 3.79.1 . The current OS/2 port gmake
that I have and that mozilla is using is 3.76.1 . I'm not even sure if there is
a 3.79.1 OS/2 port of gmake at this point. I could not locate a binary for it on
the Internet.
An alternative - solution would be to spawn a script to do the if check, but
that would be even more of a hack.
I don't have a solution either.  (I am a gmake guru.)
Wan-Teh,

The fixed command rule that prepends the path conditionally would look like this
with 3.79.1 :

	$(CC) -Fo$@ -c $(CFLAGS) $(if $(findstring :,$<),$<,$(shell pwd)/$<)

The above is for NT - I tested it and it works with gmake 3.79.1 .
Unfortunately the $(if is not available in the 3.76.1 OS/2 version of gmake, and
it appears there are no newer ports.

The only alternative (as I mentioned in my last comment) would be to do
something like :

	$(CC) -Fo$@ -c $(CFLAGS) $(shell expandpath $<)

where "expandpath" would be a script that would check for : and add pwd as
needed ... But even I consider that too much of a hack to implement it that way :).
Since the change using ifeq ($(findstring : does not work at runtime, I am
removing it from mozilla/security/nss/coreconf/rules.mk , until a common
solution is found that works for both NSPR & NSS builds. Hopefully it will be in
the form of a gmake 3.79.1 OS/2 port. In the meantime the standalone NSS / NSPR
build cannot be debugged properly under OS/2, only the browser objdir build can.
Priority: P2 → P3
Summary: rules.mk change for OS/2 build → rules.mk change for OS/2 build to fix debugger
Javier,

I saw your name and e-mail address in the readme for the gmake 3.76.1 OS/2 port.
I was wondering if you would be able to build a 3.79.1 OS/2 version of gmake :)
It would appear that Windows even has some of these problems.

With NSPR, you must select the file in Visual C++ - it can't find it 
automatically.
Mike,

That is in fact the case. The same patch would solve it as well for Windows
builds of NSS, but would require the latest version of gmake.
Summary: rules.mk change for OS/2 build to fix debugger → rules.mk change for OS/2 and Windows build to ease debugging
Actually, that solution wouldn't work for NSPR on win32.  Since we use cygwin
utils on win32, $(shell pwd) would return a /cygdrive path which would cause the
native win32 tools to break.  The proper solution, in both cases I believe, is
just to use  $(srcdir)/$(<F) if you require a full path to the srcfile but that
assumes that your VPATH only contains $(srcdir) which, currently, is true.

Also, you could probably avoid this entire problem by just calling configure
with the full path.  Then it would put the full path in $(srcdir)which would be
used when expanding $< via VPATH.  That's wrt NSPR.  I'd have to take a closer
look for NSS.

Btw we already require the latest gmake, 3.79.1, for win32.


NSS does use multiple vpaths in a number of places: This was to replace symbolic
links created in the source tree at build time so we could build in Read Only trees.

bob
Attachment #53949 - Attachment is obsolete: true
Attachment #56200 - Attachment is obsolete: true
Attachment #56203 - Attachment is obsolete: true
gmake 3.79 has been ported to OS/2, so this problem can be resolved now for the
NSS side by using the $(if support . This will work for both OS/2 & Win32 and
will finally give us the correct paths. We can enable the $shell pwd only for
debug builds. This will make the Windows and OS/2 builds of NSS slower, but IMO
it is well worth it in the time it saves trying to locate each source file when
debugging.

For the NSPR side, we will need to use the fix suggested by Chris Seawood to get
the correct pathnames.
One thing to think about: for release builds it's actually better not to have
the hard coded path of the source files. This is because the release  files are
built and executed on 2 different machines. The path to the source files will
likely vary between the machine that is running the binary and the machine that
built them.

bob
Javier Pedemonte of IBM informed me that he had trouble using the new OS/2 port
of gmake 3.79 to build mozilla . I tried it and I was also unable to build,
getting very strange errors. The only rule that I could get to work with it for
the NSS build was "clobber".
FYI, I had already tried passing the fullly qualified path to the NSPR configure
script. Unfortunately, that doesn't work. The generated Makefiles still call the
compiler with a relative path to the source files.
OK, we've switched to 3.79 now. Can you put together a patch?
Attached patch Patch for coreconf - NSS / DBM (obsolete) — Splinter Review
I tested it on Win32 with the standalone NSS build, and the paths were properly
expanded, which allowed the resulting executables and libraries to be debugged
without entering the PATHs.

This still needs to be tested with a full browser build on both Win32 and OS/2.
Attached patch Patch for NSPR (obsolete) — Splinter Review
I tested it on Win32 with the standalone NSS build, and the paths were properly
expanded, which allowed the resulting executables and libraries to be debugged
without entering the PATHs.

This still needs to be tested with a full browser build on both Win32 and OS/2.
Comment on attachment 117777 [details] [diff] [review]
Patch for coreconf - NSS / DBM

We support building NSPR and NSS under Cygwin bash.
The Mozilla client is built using Cygwin bash on
Windows.  I predict that this patch won't work if
Cygwin bash is used because its pwd command returns
"cygdrive" pathnames (for example, /cygdrive/d/tmp
for d:\tmp).
Wan-Teh,

How would I get the real path of the current directory in the cygwin build ?
I suppose it's possible to do automatic translation with gmake string functions,
but it'll be a very complicated expression.
Cygwin has a cygpath command that can convert pathnames, but this
seems to do the trick:

  $(shell pwd | sed -e 's|/cygdrive/\(.\)/|\1:/|g;')

The sed script is taken from
http://lxr.mozilla.org/nspr/source/nsprpub/build/cygwin-wrapper.

Mozilla may have implemented a solution to the problem already.
Attached patch Patch for coreconf (obsolete) — Splinter Review
Per Wan-Teh's feedback - fix path if pwd output contains cygdrive
Attachment #117777 - Attachment is obsolete: true
Attached patch Patch for NSPR (obsolete) — Splinter Review
Per Wan-Teh's feedback - fix path if pwd output contains cygdrive
Attachment #117778 - Attachment is obsolete: true
Attachment #117973 - Flags: superreview?(wtc)
Attachment #117973 - Flags: review?(mkaply)
Attachment #117972 - Flags: superreview?(wtc)
Attachment #117972 - Flags: review?(mkaply)
I finally got my OS/2 system back on the network, and I installed the new gmake
today. Unfortunately my patch doesn't work with it. But I believe it is a gmake bug.

The problem is with the expression that Wan-Teh posted :
 $(shell pwd | sed -e 's|/cygdrive/\(.\)/|\1:/|g;')

It doesn't get processed properly.
When I use the current patch to build NSPR, I get :

pr/src/io'
icc -Fopriometh.obj -c   /Q /W1 /Q /qlibansi /Gd+ /Gm+ /Su4 /Mp /Tl9 /Ti+    -UN
DEBUG -DDEBUG_oops   -DDEBUG=1  -DXP_OS2=1  -DXP_PC=1  -DBSD_SELECT=1  -D_PR_GLO
BAL_THREADS_ONLY=1  -DXP_OS2_VACPP=1  -DOS2=4  -DTCPV40HDRS=1  -D_X86_=1  -DHAVE
_STRERROR=1   -DFORCE_PR_LOG -D_NSPR_BUILD_ -ID:/nss/38/mozilla/security/nss/../
../dist/OS22.45_icc_DBG.OBJ/include -I../../../../pr/include -I../../../../pr/in
clude/private  /nss/38/mozilla/nsprpub/OS22.45_icc_DBG.OBJ/pr/src/io/../../../..
/pr/src/io/priometh.c
warning EDC1011: Bad number ss/38/mozilla/nsprpub/OS22.45_icc_DBG.OBJ/pr/src/io/
../../../../pr/src/io/priometh.c has been ignored in option /n.


As you can see, the C source filename is missing the first two characters - the
drive letter and : . They should be "d:" .

If I replace the expression designed to replace cygdrive with simply $(shell
pwd), things work as expected. But of course they will break cygwin .

On winnt, gmake behaves properly. Both OS/2 and Windows versions are 3.79.1 ...

There are a couple of solutions :
1) get another fix for gmake 3.79 on OS/2
2) add more ifdefs to the makefile to separate the rules for OS/2 and Win32 .
This might be preferable for performance, as it will avoid a call to sed that's
never necessary. On the other hand, it will obfuscate the rules files even more .
It is the easiest option for us as the patch to the rules is under my control,
unlike the patch to gmake in the other option.
Attached patch Patch for NSPR (obsolete) — Splinter Review
Updated . Works in OS/2
Attachment #117973 - Attachment is obsolete: true
Attached patch Patch for coreconf (NSS & DBM) (obsolete) — Splinter Review
Updated . Works in OS/2
Attachment #117972 - Attachment is obsolete: true
Attached patch patch for coreconf - NSS/DBM (obsolete) — Splinter Review
fix typo
Attachment #118518 - Attachment is obsolete: true
Attachment #118517 - Flags: superreview?(wtc)
Attachment #118517 - Flags: review?(mkaply)
Attachment #118588 - Flags: superreview?(wtc)
Attachment #118588 - Flags: review?(mkaply)
Julien:  What exactly is the sed line trying to do?
Javier,

The sed line is supposed to convert pathnames in the cygdrive format to regular
windows pathnames, when cygwin is in use. This is applicable only to win32.
However, it shouldn't hurt anything when one doesn't use cygwin. In fact, it
behaves just fine on my win32 system that doesn't use cygwin.
But on OS/2 the line gets mangled. The latest patches take care of the problem,
by separating rules between OS/2 & Win32. This was already separated in NSPR,
but not in coreconf.
Please review the patches and let me know if the browser builds work with them.
Mike / Javier / Wan-Teh,

Please review these patches. I tested them with standalone builds of NSPR & NSS
on both Win32 & OS/2. I didn't test the full browser builds, though. I would
really like to bring this defect to closure.
Comment on attachment 118517 [details] [diff] [review]
Patch for NSPR

I really don't like that construct for the win32 builds.  win32 already uses
the cygwin wrapper script to convert the cygdrive path on each line to a
dos-path. CC='$(CYGWIN_WRAPPER) cl' .  Also, in the browser build, $< should
return a full /cygdrive path so this sed expression won't work anyway.	It'll
convert $pwd/file.c to $pwd/$pwd/file.c because $pwd won't contain any :.  

The problem with $< not returning a full path can be resolved by forcing VPATH
to contain a full path in the Makefiles rather than the relative path being
passed by configure (as either ./configure or ../configure).   That's how we
resolved this problem for the main browser build.  Of course, this also means
using the perl script to generate Makefiles rather than rely upon autoconf's
default sed-replace every AC_SUBST symbol in the Makefiles.  The makefiles only
use @top_srcdir@ &
@srcdir@ so the rest of the seds aren't necessary.  

Actually, if you wanted to mandate using a full path to configure, you could
avoid this problem entirely.
Attachment #118517 - Flags: superreview?(wtc)
Attachment #118517 - Flags: review?(mkaply)
Attachment #118517 - Flags: review-
Chris,

I already am passing a full pathname to configure, and the problem still exists
in my standalone NSS build. NSPR is still getting compiled with relative
pathnames. This is why my patch was required.

Wan-Teh told me that I didn't need to do the cygdrive path conversion for NSPR
because its build system already took care of it. I will regenerate a patch
without it. I'm unable to test if this works as I still cannot get cygwin to
work on my windows box (damn installer from hell).

For the NSS coreconf patch, the cygdrive translation is still required, however.
Comment on attachment 118588 [details] [diff] [review]
patch for coreconf - NSS/DBM

r=wtc.	Could you see if you can turn this $(if $(findstring ... expression
into a function because it's repeated four times?  See
http://www.gnu.org/manual/make/html_mono/make.html#SEC89.

Maybe this will work?

Define the function as:

ifeq ($(OS_TARGET),OS2)
abspath = $(if $(findstring :,$(1)),$(1),$(shell pwd)/$(1))
else
abspath = $(if $(findstring :,$(1)),$(1),$(shell pwd | sed -e
's|/cygdrive/\(.\)/|\1:/|g;')/$(1))
endif

Then the rule for compilation would look like:

	$(CC) -Fo$@ -c $(CFLAGS) $(call abspath $<)
Attachment #118588 - Flags: superreview?(wtc) → superreview+
Attached patch patch for NSPR (obsolete) — Splinter Review
One more.
Don't try to process cygdrive again.
Don't call shell pwd if cygdrive is in $<
Attachment #118517 - Attachment is obsolete: true
Attachment #118599 - Flags: superreview?(seawood)
Attachment #118599 - Flags: review?(mkaply)
Julien, Argh.  Lemme guess, you're doing a srcdir build?  If so, you're right;
that won't work.  It should be a bug that VPATH implicitly searches the current
dir first.

Anyway, for that case, I'd suggest using a similar solution to what we've done
in the client rules.mk,
http://lxr.mozilla.org/seamonkey/source/config/rules.mk#97 , which is basically
the same thing that wtc has suggested.  Btw, try using $(srcdir) instead of
invoking the extra $(shell pwd) if possible.


Chris,

I'm not sure what you mean by "srcdir build". What I'm doing is a standalone
NSPR/NSS build , not including the browser.

To build NSPR & NSS I go to mozilla/security/nss and "gmake nss_build_all".
Doing a substitution on "/cygdrive/" also won't work for cygwin mount points
that aren't under /cygdrive/.
Bryan,

What would those mount points be ? Are there ever any source files in them ?

With cygwin, you can mount a drive anywhere.  For example, I could mount my C
drive in /c instead of /cygdrive/c.
OK, I see the problem. Sounds like the cygpath utility is the only way to go then.
I'll have yet another patch on its way for NSS.
Comment on attachment 118599 [details] [diff] [review]
patch for NSPR

Can you use $(srcdir) instead of $(shell pwd)? I'm wary of just grepping for :
rather than ^.:/ but :s in path names probably don't work for other reasons.
Attachment #118599 - Flags: superreview?(seawood) → superreview-
Attachment #118588 - Attachment is obsolete: true
Attachment #118599 - Attachment is obsolete: true
Attached patch Patch for NSPR (obsolete) — Splinter Review
Attached patch Patch for coreconf - NSS/DBM (obsolete) — Splinter Review
Chris : $(srcdir) wasn't working for my builds . Only $(shell pwd) did it.

I generated new patches, yet again. I'm desperate for someone with a working
cygwin install to test them.

I have tested them only with standalone NSS builds, on Win32 and OS/2, and they
worked fine.

They need to be tested with browser builds on Win32 and OS/2.
Attachment #118731 - Flags: superreview?(wtc)
Attachment #118731 - Flags: review?(seawood)
Attachment #118732 - Flags: superreview?(wtc)
Attachment #118732 - Flags: review?(mkaply)
To explain things in these latest patches :
Due to comment #65 , I realized there was no way I could do the processing
correctly without knowing whether I was running under cygwin or not.
So I added a test for cygwin, suggested by Wan-teh. Wan-teh said cygwin uses :
as separator in the PATH , whereas regular Win32 and OS/2 shells use semicolon.
So I test for a semicolon . If a semicolon is present, then I assume I'm not
running under CYGWIN.

For the non-cygwin case, I then just test for : in the filename to see if it is
fully qualified, and prepend $(shell pwd) if not.

For the cygwin case, I use cygpath, as suggested by Wan-Teh.

I believe this covers all the bases, but I am unable to actually test under cygwin.
Comment on attachment 118731 [details] [diff] [review]
Patch for NSPR

Sorry, Julien, you're right.  $(srcdir) won't work because we're not forcing
srcdir to be an absolute path at configure time.  

A srcdir build is a build where the generated object files wind up in the same
directory/tree as the source.

Can you create a custom function or macro for the $(if) & $(shell) magic so
that those complex expressions & ifdefs aren't repeated for each ruleset?
Comment on attachment 118732 [details] [diff] [review]
Patch for coreconf - NSS/DBM

1. Define USE_CYGWIN with the $(if) function to avoid the
intermediate variable NO_CYGWIN.

2. coreconf does not need this complexity:

>+ifdef USE_CYGWIN
>+	$(CC) -Fo$@ -c $(CFLAGS) $(shell cygpath -w $< | sed -e 's|\\|/|g' | grep ^.:/ || cygpath -w `pwd`/$< | sed -e 's|\\|/|g')

In coreconf, $< is guaranteed to be a Windows path (with
the possible exception of / instead of \), so you do not
need to convert $<.  You only need to use a sed script or
cygpath to convert `pwd` to a Windows path.
Attachment #118732 - Flags: superreview?(wtc) → superreview-
Comment on attachment 118731 [details] [diff] [review]
Patch for NSPR

> In coreconf, $< is guaranteed to be a Windows path (with
> the possible exception of / instead of \), so you do not
> need to convert $<.

This confused me until I realized that $< is a relative path when building NSS.
 Cygwin make will convert $< into a cygwin path regardless of paths set in
VPATH.
Attachment #118731 - Flags: superreview?(wtc)
Attachment #118731 - Flags: review?(seawood)
Attachment #118731 - Flags: review-
Attached patch Patch for coreconf - NSS/DBM (obsolete) — Splinter Review
This time I really think this is the one.
I'm using cygpath -a -w which resolves any path to an absolute Windows path.
Attachment #118732 - Attachment is obsolete: true
Attached patch Patch for NSPR (obsolete) — Splinter Review
Updated patch incorporating Wan-Teh's comments :
1. Use $if to evaluate USE_CYGWIN
2. Considerably simplify the expression for the Cygwin case, by using cygwin -a
-w which resolves any path to an absolute Windows path.
Attachment #118731 - Attachment is obsolete: true
Attachment #119236 - Flags: superreview?(wtc)
Attachment #119236 - Flags: review?(relyea)
Attachment #119237 - Attachment is obsolete: true
Attachment #119236 - Attachment is obsolete: true
Attachment #119236 - Flags: superreview?(wtc)
Attachment #119236 - Flags: review?(relyea)
Attached patch Patch for NSPR (obsolete) — Splinter Review
Take feedback from Wan-Teh into account :
1. Evaluate USE_CYGWIN using $if and $findstring
2. Simplify cygpath expression . Use cygpath -a -u to return an absolute Unix
path . This path then gets fed to the cygwin wrapper for conversion
3. Only evaluate USE_CYGWIN on Windows platforms
Attached patch Patch for coreconf - NSS/DBM (obsolete) — Splinter Review
1. Use cygpath -a -w to simplify expression
2. Only evaluate USE_CYGWIN on Windows platforms
Is cygpath -a supported by all versions of cygpath?  I thought it was a recent
addition like the -m option.  If we're going to use that option, we should
probably add a minimum version check for cygwin/cygpath in configure.
Chris : I don't know. The version I have is from 2/25/2002, just over a year old.
I just saw the option in the help screen and tried to use it. It seemed to work
well.

Incidently, I'd like to report that I have been able to do both an MKS and a
cygwin build of standalone NSS on NT . I'm still unable to do a browser build on
NT with cygwin.

I have also done OS/2 build of standalone NSS. The patch works fine with all of
them.

Mozilla browser builds still need to be tested on both NT & OS/2 . I can test
the later, but my OS/2 machine is very slow and may not finish the build by
midnight, the deadline to check into NSS 3.8 .

So I need help to test this patch with the browser build. Any help is appreciated.
Attachment #119242 - Flags: superreview?(wtc)
Attachment #119242 - Flags: review?(seawood)
Attachment #119243 - Flags: superreview?(wtc)
Attachment #119243 - Flags: review?(mkaply)
My full OS/2 build of the browser completed, so I'm going to assume that the NSS
change is fine and check it in. Unfortunately there is no one available for
reviewing at this hour.

Checking in rules.mk;
/cvsroot/mozilla/security/coreconf/rules.mk,v  <--  rules.mk
new revision: 1.49; previous revision: 1.48
done
Comment on attachment 119243 [details] [diff] [review]
Patch for coreconf - NSS/DBM

This patch broke the NSS nightly build on NT this morning,
so I've backed it out.	The error message is:

------------
cd util; gmake libs
gmake[2]: Entering directory
`D:/nss/nsstip/builds/20030403.1/spd06_NT4/mozilla/security/nss/lib/util'
process_begin: CreateProcess((null), cygpath -a -w quickder.c, ...) failed.
cl -FoWINNT4.0_DBG.OBJ/quickder.obj -c -Od -Z7 -MD -W3 -nologo -GT -DXP_PC
-DDEBUG -D_DEBUG -UNDEBUG -DDEBUG_ -DWIN32 -D_WINDOWS -D_X86_ -DWINNT
-I../../../../dist/WINNT4.0_DBG.OBJ/include  -I../../../../dist/public/nss
-I../../../../dist/private/nss -I../../../../dist/public/dbm  
Command line error D2003 : missing source filename
gmake[2]: *** [WINNT4.0_DBG.OBJ/quickder.obj] Error 2
gmake[2]: Leaving directory
`D:/nss/nsstip/builds/20030403.1/spd06_NT4/mozilla/security/nss/lib/util'
gmake[1]: *** [libs] Error 2
------------

Apparently USE_CYGWIN is set under MKS Korn shell and seems
to have the value ")".	Under Cygwin bash USE_CYGWIN seems
to have the value "1)", which is also strange.	I haven't
figured out why.  But it's clear that this test needs work:

ifeq (,$(filter-out WIN%,$(OS_TARGET)))
USE_CYGWIN := $(if $(findstring ;,$(PATH)),,1))
endif

This test seems to work:

ifeq (,$(filter-out WIN%,$(OS_TARGET)))
ifeq (,$(findstring ;,$(PATH)))
USE_CYGWIN = 1
endif
endif

I will attach a patch that uses the latter test and the
"call" function.
Attachment #119243 - Flags: superreview?(wtc)
Attachment #119243 - Flags: superreview-
Attachment #119243 - Flags: review?(mkaply)
Attached patch Patch for coreconf - NSS/DBM (obsolete) — Splinter Review
This patch is merely a rewrite of Julien's patch (attachment 119243 [details] [diff] [review])
using the "call" function.  I implemented the "abspath" function and
call it.  I also rewrote the test for Cygwin because the old test,
which uses the "if" function, seems to set USE_CYGWIN to strange
values, with an extraneous ")".

So the two things in this patch that need review are:
1. the Cygwin (USE_CYGWIN) test; and
2. the definition of the abspath function, which is the same as
Julien's patch.
Attachment #119243 - Attachment is obsolete: true
Attached patch Patch for coreconf - NSS/DBM (obsolete) — Splinter Review
The previous patch (attachment 119298 [details] [diff] [review]) does not work under Cygwin
because cygpath -w generates a patch like C:\foo\bar.c, which
is passed by make to the compiler as C:foobar.c -- apparently '\'
is an escape character in make.  Here is an example:

--------
cl -FoWIN954.0_OPT.OBJ/quickder.obj -c -O2 -MD -W3 -nologo -DXP_PC -UDEBUG
-U_DE
BUG -DNDEBUG -DWIN32 -D_WINDOWS -D_X86_ -DWIN95
-I../../../../dist/WIN954.0_OPT.
OBJ/include  -I../../../../dist/public/nss -I../../../../dist/private/nss
-I../.
./../../dist/public/dbm 
d:\wtc\nss-tip\mozilla\security\nss\lib\util\quickder.c

wtcnss-tipmozillasecuritynsslibutilquickder.c
fatal error C1083: Cannot open source file:
'd:wtcnss-tipmozillasecuritynsslibut
ilquickder.c': No such file or directory
make[2]: *** [WIN954.0_OPT.OBJ/quickder.obj] Error 2
--------

We could fix this by either using cygpath -m or substituting
\ with \\ in the output of cygpath -w.	This patch uses
cygpath -m.  The compile command line now looks like:

--------
cl -FoWIN954.0_OPT.OBJ/quickder.obj -c -O2 -MD -W3 -nologo -DXP_PC -UDEBUG
-U_DE
BUG -DNDEBUG -DWIN32 -D_WINDOWS -D_X86_ -DWIN95
-I../../../../dist/WIN954.0_OPT.
OBJ/include  -I../../../../dist/public/nss -I../../../../dist/private/nss
-I../.
./../../dist/public/dbm 
d:/wtc/nss-tip/mozilla/security/nss/lib/util/quickder.c

quickder.c
--------
Attachment #119298 - Attachment is obsolete: true
See previous note about 'cygpath -m' being a recent feature of cygpath.  People
were complaining about their builds breaking the last time we tried using it on
the trunk.  I don't remember if a tinderbox broke as well.
cls: could you tell me the cygpath options that we can use?
If you have an older version of cygpath around, you can just
post its usage message here.  Thanks.

Perhaps we should write a simple C program to call the Win32
function GetFullPathName().
+2002-06-22  Joshua Daniel Franklin <joshuadfranklin@yahoo.com>
+
+	* cygpath.cc (long_options): Add "dos" and "mixed", correct "close",
+	"file" and "type" to use NULL flag.
+	(usage): Clean up usage output (more), accomodate new options.
+	(main): Add --dos and --mixed options; accomodate all output forms in
+	--type.  Make UNIXy output default.

The cygwin 1.3.11 release was made the next day so I'm guessing that was first
one with the change.  My laptop's hd died so I can't test that or tell you what
options were available for prior releases.  

(Note that I'm not saying that you can't use those options.  I'm saying that if
you do, you need to make people aware of the additional requirement and add a
configure test for NSPR.  For mozilla, we opt'd for using sed instead though I'm
beginning to see the need for a minimum version test.)
The version of cygpath I have doesn't have -m . But it does have -w .
I reproduced the build problem by switching my build to use cygwin's make
instead of the other GNU gmake I had.
I guess it would be OK to use sed here . I'm going to make a new patch.
I was having some issues with mixed environments. My "MKS" and "cygwin" build
were sharing some tools. I have now finally resolved these issues. I now have to
distinct environments for MKS and cygwin, where no MKS tools are available in
the cygwin environment and vice-versa. In addition, I have a brand new install
of cygwin that works well.

I was able to complete a full WINNT build of the Mozilla browser using my latest
patches.

I also did NSS builds under both cygwin and MKS with the same patches.

I did not do any OS/2 testing yet. Hopefully it will be OK too. I will attach
the patches now for review.
Attached patch Patch for coreconf - NSS/DBM (obsolete) — Splinter Review
Tested under Win32 for both standalone NSS under MKS & cygwin, and full browser
build (cygwin)
Attachment #119303 - Attachment is obsolete: true
Attached patch Patch for NSPR (obsolete) — Splinter Review
Tested under Win32 for both standalone NSS under MKS & cygwin, and full browser
build (cygwin)
Attachment #119242 - Attachment is obsolete: true
Attachment #119407 - Flags: superreview?(seawood)
Attachment #119407 - Flags: review?(wtc)
Attachment #119408 - Flags: superreview?(seawood)
Attachment #119408 - Flags: review?(wtc)
I would much prefer it if you would do something like the following:
+ 
+ifeq (,$(filter-out WIN%,$(OS_TARGET)))
+ifeq (,$(findstring ;,$(PATH)))
+_ABS_VPATH_SRCS = $(if $(findstring :,$<),$<,$(shell cygpath -a -u $<))
+else
+_ABS_VPATH_SRCS = $(if $(findstring :,$<),$<,$(shell pwd)/$<)
+endif
+endif
 
-	$(CC) -Fo$@ -c $(CFLAGS) $<
+	$(CC) -Fo$@ -c $(CFLAGS) $(_ABS_VPATH_SRCS)

if you can figure out some way to make that work for the OS/2 case as well, all
the better.
Attached patch Patch for NSPR (obsolete) — Splinter Review
Incorporate feedback from comment #92 . Tested with cygwin & MKS on NT.
Attachment #119408 - Attachment is obsolete: true
Comment on attachment 119407 [details] [diff] [review]
Patch for coreconf - NSS/DBM

Julien,

Why don't you use the "call" function, as in attachment 119303 [details] [diff] [review]?
Wan-Teh,

It would appear that the $(call function) is unnecessary.
$(function) seems to work fine, as in attachment 119409 [details] [diff] [review] .
Comment on attachment 119409 [details] [diff] [review]
Patch for NSPR

1. I believe that in NSPR, the test $(findstring :,$<) is not
sufficient to determine that $< is an absolute pathname;
you should also test $(filter /%,$<).

2. In NSPR, you can use $(shell pwd)/$< for Cygwin too because
cygwin-wrapper will take care of the Unix-to-Windows pathname
conversion.

3. I think it is better to use the "call" function than the
_ABS_VPATH_SRCS variable.
Here is a patch for coreconf (NSS/DBM) that does not
use cygpath.  It uses a simple batch file (pwd.bat)
to get the current directory as a Windows pathname.
It only gets the current directory once per directory,
as opposed to once per .c file.

Because it doesn't use cygpath, it doesn't need to
determine whether we are using Cygwin or MKS.
This is in reponse to comment #96 :

1. In which cases would the path begin with / under WINNT & OS/2 ?

From my experience, the source file path was always a relative path for NSPR,
never a path from the root. Do you have an example of invoking the NSPR build
that would cause the source paths to begin with / (or \) ?

Also, note that if we have a path without the prepending drive letter, MSVC &
the IBM debugger will still prompt to locate the source file, so the problem
wouldn't be resolved. 

This brings another issue that I hadn't thought about - UNC support - network
names beginning with \\ . I would have to check if the compiler & debugger
support these filenames. Hopefully the rest of the build environment does not so
we don't have to worry about these cases.

2. Possibly. Doing so may have the advantage of running on older versions of
cygwin. But I still would prefer to run cygpath .
2a. Because the cygwin wrapper is always invoked, windows filenames with a colon
are never passed. If I use the -w option on cygpath to convert to windows
pathnames, it breaks the build because the wrapper tries to convert it. So I
shouldn't be looking for a : in the cygwin case at all, because there should
never be one. This means the test will always be true, and cygpath (or $(shell
pwd)) will always be invoked to fully-qualify the path.

3. Yes, I'll convert it.


Comment on attachment 119411 [details] [diff] [review]
A patch for coreconf (NSS/DBM) that does not require cygpath

This may work, however I would prefer avoiding running the command interpreter
and a batch file for every source file. It will increase build time more than
just running pwd.exe as I do in the other patches.
Comment on attachment 119411 [details] [diff] [review]
A patch for coreconf (NSS/DBM) that does not require cygpath

This patch works.

pwd.bat is only executed once per directory, not per source file.
This optimization can be made to your patch, too, if you use
    PWD := $(shell cygpath -w `pwd` | sed -e 's|\\|/|g')
and
    $(PWD)/$<
instead of
    $(shell cygpath -a -w $< | sed -e 's|\\|/|g')

In NSPR, the source file path may be something like
/cygdrive/d/nspr-tip/mozilla/nsprpub/pr/src/io/prio.c, which
begins with /.
This is based on attachment 119411 [details] [diff] [review], except that :
- It does not use a pwd.bat batch file . Instead, the native OS shell, cmd.exe,
is called directly . The shell has the same name under both NT and OS/2
- There is no platform filtering, only USE_NT_C_SYNTAX is checked

I have tested this pach successfully with standalone NSS builds under
Win32/MKS, Win32/Cygwin, and OS/2.
Attachment #119407 - Attachment is obsolete: true
Attachment #119411 - Attachment is obsolete: true
I just want to add that I eliminated the batch file because the files have
different extensions under NT and OS/2.

In OS/2, a .BAT file will invoke the DOS shell COMMAND.COM in a VDM (virtual DOS
machine). I'm not sure if its output can be piped to an OS/2 process. If DOS
support isn't installed in OS/2, as is the case on my machine and many others,
the .BAT will fail to execute. Also, the VDM is limited to 8.3 filenames, so it
cannot see long filenames (HPFS, JFS, NFS) so the paths returned from
COMMAND.COM may be incorrect if they are passed back to the OS/2 gmake.

To invoke native bach files in OS/2 that run under the 32-bit CMD.EXE OS/2
shell, the files need to have the extension .CMD .
Comment on attachment 119508 [details] [diff] [review]
Simpler patch for coreconf - NSS/DBM

cmd.exe does not exist on Win 9x.  It is possible to build
Mozilla on Win 9x (see http://www.mozilla.org/build/win32.html#ss2.1),
so I am marking this patch review-.  Note: I did not test
pwd.bat on Win 9x so I don't know if it works there.

On OS/2, you can use the pwd command.  The pwd command is
only a problem under Cygwin.  My pwd.bat file is intended
to avoid that problem and the need for cygpath.  (Under MKS,
we can use either the pwd command or pwd.bat.)

I don't object to using cygpath, but when I landed NSS 3.8
in NSS_CLIENT_TAG, the Mozilla tinderbox machine "beast"
had a strange problem with the cygpath command I used in
mozilla/security/nss/cmd/shlibsign/sign.sh (see the cvs
commit comment for rev. 1.8).  So I had to back it out.
Attachment #119508 - Flags: review-
I didn't know we supported Win9x as a build environment. I would suspect that
pwd.bat will not report the true path under those environments in the case of
long filenames eg. a name with a ~ in it.
This is what happened when I tried to run PWD.BAT under Win2K's COMMAND.COM,
which is probably similar in Win9x . However with CMD.EXE under Win2K, it worked
fine.
This means we can't avoid if statements per platform for the PWD variable.
I looked at the cvs logs and diffs for sign.sh , but I couldn't make up exactly
what it was that the tinderbox machine didn't like.

I'm of the opinion that cygwin is the root of all evil, and that if we are going
to use it and even require it for Mozila, it needs to provide us with a way to
return the full current directory , ie. working -a and -w options . Without
that, there is no way that I can resolve this bug. Win9x introduces another
whole set of potential problems.

I'd like to use the following approach :
1) if under cygwin (on Win*.*), use cygpath
2) for all other cases (ie. MKS on Win*.*, and OS/2 VACPP), use shell pwd

If the tinderbox breaks, I think it would be time to update the tools on it. We
can't stay on broken tools forever. Even the OS/2 build is moving away from
VACPP after all.

Speaking of that last change, does the IBM debugger find the source files
automatically in the OS/2 gcc build, or do you have to locate them manually ?
If OS/2 gcc behaves like on other platforms, it automatically expands any source
path to absolute into the object files when compiling so we don't have to do
anything hopefully.
Attached patch Patch for coreconf - NSS/DBM (obsolete) — Splinter Review
- don't use cmd.exe since it doesn't exist on Win9x
- use shell pwd except on cygwin
- invoke path resolution using $(call)
Attachment #119508 - Attachment is obsolete: true
Attached patch Patch for NSPR (obsolete) — Splinter Review
- don't use cmd.exe since it doesn't exist on Win9x
- use shell pwd except on cygwin
- invoke path resolution using $(call
I tested attachments 119521 and 119522 successfully with standalone NSS builds
under NT / MKS, NT / Cygwin, and OS/2.
Attachment #119521 - Flags: superreview?(wtc)
Attachment #119521 - Flags: review?(mkaply)
Attachment #119522 - Flags: superreview?(seawood)
Attachment #119522 - Flags: review?(wtc)
Comment on attachment 119522 [details] [diff] [review]
Patch for NSPR

sr=cls

I still want to see a configure test for the cygpath feature so that people
know why it broke rather than us getting bug reports about bustage later.
Attachment #119522 - Flags: superreview?(seawood) → superreview+
Comment on attachment 119522 [details] [diff] [review]
Patch for NSPR

(Not sure if I stopped the first submit in time.)

PWD should be assigned using := so that it is only invoked once per makefile
target rather than once per file.  This will cause pwd to be invoked even when
we're not compiling but that should be less than the number of files compiled.

Fix that && sr=cls

I still want to see a configure test for the cygpath feature so that people
know why it broke rather than us getting bug reports about bustage later.
Chris,

I don't know how to write a configure test. Is it possible that you could help
with it ?
I'll fix the patch to assign PWD with := .
Attached patch Patch for NSPR (obsolete) — Splinter Review
Use PWD := as suggested by cls .
Attachment #119409 - Attachment is obsolete: true
Attachment #119522 - Attachment is obsolete: true
Comment on attachment 119743 [details] [diff] [review]
Patch for NSPR

You should use

+PWD := $(shell pwd)
+abspath = $(if $(findstring :,$(1)),$(1),$(if $(filter
/%,$(1)),$(1),$(PWD)/$(1)))

for Cygwin as well because cygpath -a prevents you from
using the optimization of calling the shell function
only once per directory.

It is better to use

  ifeq ($(NS_USE_GCC)_$(OS_ARCH),_WINNT)

to match the ifdef used in the makefile rules.
Comment on attachment 119521 [details] [diff] [review]
Patch for coreconf - NSS/DBM

PWD should be defined using := so that the shell function is
invoked only once per directory.

The use of cygpath -a makes it necessary to call the shell
function once per source file.	I think it is better to define
PWD as
  PWD :=  $(subst \,/,$(shell cygpath -w `pwd`))
under Cygwin and use the same definition of abspath for all
three cases (Cygwin, MKS, and OS/2).
Comment on attachment 119521 [details] [diff] [review]
Patch for coreconf - NSS/DBM

By the way, in the current coreconf, $< is always a filename or a
relative pathname, so we could take advantage of that
and omit the test for absolute pathnames (i.e., the nested if
function calls) and simply replace $< by $(PWD)/$<.
Attachment #117972 - Flags: superreview?(wtc)
Attachment #117972 - Flags: review?(mkaply)
Attachment #117973 - Flags: superreview?(wtc)
Attachment #117973 - Flags: review?(mkaply)
Attachment #119242 - Flags: superreview?(wtc)
Attachment #119242 - Flags: review?(seawood)
Attachment #119407 - Flags: superreview?(seawood)
Attachment #119407 - Flags: review?(wtc)
Attachment #119408 - Flags: superreview?(seawood)
Attachment #119408 - Flags: review?(wtc)
Chris,

Thanks for contributing the cygwin version check to the autoconf build.

I'd like to use a similar test for the NSS build, but it doesn't use autoconf,
so I'm not sure how I can do that without adding lots of unnecessary calls to
cygcheck. So I'm thinking of leaving attachment 1195218 alone. Mike, Wan-Teh,
could you please review it ?

Thanks.
Your current NSPR patch uses the -a and -u options of
cygpath.  If you make the changes I suggested in comment
113, your NSPR patch will not use cygpath at all.

Your current coreconf patch uses the -a and -w options
of cygpath. If you make the changes I suggested in comment
114, your coreconf patch will use the -w option only.

Since you are not using the recently added -m option in
either patch, I don't think you need the Cygwin version
check.  If you make my suggested changes, you will only
be using the -w option, which Mozilla is already using
in several places.
I think Javier or Chris Seawood are the right people to review this.
Answering my own question in comment #105 : after I built NSS today with gcc on
OS/2, the debugger prompted me to locate modutil.c .
Therefore, we also need to pass absolute filenames for the source files to gcc
on OS/2 . This is not just for the VACPP build.
This means the patches have to be reworked once more, and no longer apply only
to the USE_NT_C_SYNTAX case .
Summary: rules.mk change for OS/2 and Windows build to ease debugging → Compile source files with absolute pathnames to ease debugging
Comment on attachment 119521 [details] [diff] [review]
Patch for coreconf - NSS/DBM

Moving coreconf patches to bug 202058
Attachment #119521 - Attachment is obsolete: true
Attachment #119743 - Attachment is obsolete: true
Attachment #120544 - Flags: superreview?(seawood)
Attachment #120544 - Flags: review?(wtc)
One small suggestion:  I would change "ifdef MOZ_OS2_TOOLS" to be "ifeq
($(OS_ARCH),OS2)", since the former may be going away when we go GCC only. 
Also, why not make that first "ifeq ... else" into a simple "ifeq ... endif"? 
No sense in creating more nested ifs.
Comment on attachment 120544 [details] [diff] [review]
Patch to compile with absolute pathnames where needed

>+ifeq ($(NS_USE_GCC)_$(OS_ARCH),_WINNT)
>+NEED_ABSOLUTE_PATH := 1
>+ifeq (,$(findstring ;,$(PATH)))
>+PWD := $(shell cygpath -a .)
>+else
>+PWD := $(shell pwd)
>+endif
>+endif

"cygpath -a ." doesn't work on my system.  You
can simply use "pwd", which would avoid the
test for Cygwin bash altogether.
Responding to comment #123 from Javier :

I hope VACPP keeps working for NSPR & NSS, even if it's not supported for the
browser build. But I'll make the change to use OS_ARCH .

As far as the "else" style, I did it that way because I'm providing a default
implementation of the abspath function that maps to $(1) . I wanted to make sure
there were no overlapping cases, which might cause further problems with gmake.
So I'd like to keep it that way. I think with proper blank lines the rules still
remain readable - at least they are in logical order, by platform.

However, I see that this becomes irrelevant with Wan-Teh's comment - there are
only two possibilities, NEED_ABSOLUTE_PATH=1 or zero, and no other differences.
So I will simplify it the way you suggest.

About comment #124 from Wan-Teh :
I will put this suggestion into the next patch.
Attached patch Updated patch (obsolete) — Splinter Review
Simplify patch by incorporating suggestions from Javier & Wan-Teh.
Attachment #120544 - Attachment is obsolete: true
Attached patch Proposed patchSplinter Review
Julien, your latest patch (attachment 120646 [details] [diff] [review]) is fine
except that it requires a version of gmake that supports
the call function.  I did some minor editing on your patch
and changed it not to use the call function on platforms
that don't need absolute pathnames.
Attachment #120646 - Attachment is obsolete: true
Comment on attachment 120657 [details] [diff] [review]
Proposed patch

Attachment 120657 [details] [diff] looks good.
I checked in the latest patch on the NSPR trunk and
NSPRPUB_PRE_4_2_CLIENT_BRANCH yesterday.
Status: REOPENED → RESOLVED
Closed: 18 years ago17 years ago
Resolution: --- → FIXED
Reopening because it looks like the cygwin test patch ( attachment 120057 [details] [diff] [review] )
wasn't checked in. Chris, could you please do that at the appropriate time ? Thanks.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Do not check in the Cygwin version test patch. It is
not necessary.
this patch totally breaks my windows builds.

sh ../build/cygwin-wrapper cl -Fonow.obj -c     -W3 -nologo -GF -Gy -MD   -
UDEBUG -U_DEBUG -UWINNT  -DMOZILLA_CLIENT=1 -DNDEBUG=1 -DXP_PC=1 -DWIN32=1 -
DWIN95=1 -D_PR_GLOBAL_THREADS_ONLY=1 -D_X86_=1  -
DFORCE_PR_LOG   /builds/trunk/mozilla/nsprpub/config/now.c
Command line warning D4002 : ignoring unknown 
option '/builds/trunk/mozilla/nsprpub/config/now.c'
Command line error D2003 : missing source filename

I use cygwin mount points...
$ mount
C:\cygwin\bin on /usr/bin type system (binmode)
C:\cygwin\lib on /usr/lib type system (binmode)
C:\builds on /builds type system (textmode)
C:\cygwin on / type system (binmode)
c: on /cygdrive/c type user (binmode,noumount)
m: on /cygdrive/m type user (binmode,noumount)

Can this patch please be backed out until it actually works?
cls, do you know why NSPR's cygwin-wrapper does not
handle the pathname /builds/trunk/mozilla/nsprpub/config/now.c
properly?
I don't understand what that bug comment has to do with the patch from this bug
that was checked in and broke my builds..
The patch from this bug assumes that NSPR's cygwin-wrapper
is working.  The bug comment cls pointed out says that
cygwin-wrapper does not handle some cases.

cls, how should we fix Pavlov's build failure?  Should I
define PWD as
    PWD := $(subst \,/,$(shell cygpath -w `pwd`))
under Cygwin, or should we fix cygwin-wrapper's problems?
The cygwin-wrapper has always not handled some cases.  That was the reason bug
158920 was opened to begin with.  Until bug 158920 was fixed, the only supported
place to build was on a drive that was mounted using /cygdrive.  If you managed
to build anywhere else, you got lucky.  Bug 158920 makes it so that you can
replace /cygdrive/ with the mount point of your choice but you still cannot
place the build anywhere.  That is why the comment from bug 158920#c11 is
relevant here.  Pavlov's using a unsupported build setup.  Look at the mount
points that he used.  If c:\builds wasn't mounted as /builds, the build would
work fine.  The fact that this patch triggered that error condition is
irrelevant.  Unless there is actually something wrong with the patch, I'd say
Pavlov's out of luck.

I'm not sure if running cygpath on `pwd` would actually fix that problem.  Is
/builds/... being returned by the $(PWD) or by $< ?
Since Pavlov said he was able to build before this patch
was checked in, I guess /builds/... was returned by the
$(PWD).
Attachment #120544 - Flags: superreview?(seawood)
Attachment #120544 - Flags: review?(wtc)
Attachment #119521 - Flags: superreview?(wtc)
Attachment #119521 - Flags: review?(mkaply)
I haven't changed my cygwin setup for over a year now.  I've been lucky this
whole time?  Give me a break.

'pwd' returns /builds
'cygpath -d /builds' returns D:\builds
Pavlov, out of curiosity, what version of gmake are you running ? Do a gmake -v
. The patch requires gmake 3.79 which is already mandated by mozilla.

Stuart,

Also, could you paste the output of your build without the patch - ie. when it
works. I'm interested in the line with cl that's compiling now.c .
sorry for taking so long...

GNU Make version 3.79.1

building without the patch now.
make[3]: Entering directory `/builds/trunk/mozilla/nsprpub/config'
sh ../build/cygwin-wrapper cl -Fonow.obj -c     -W3 -nologo -GF -Gy -MD  
-UDEBUG -U_DEBUG -UWINNT  -DMOZILLA_CLIENT=1 -
DNDEBUG=1 -DXP_PC=1 -DWIN32=1 -DWIN95=1 -D_PR_GLOBAL_THREADS_ONLY=1 -D_X86_=1 
-DFORCE_PR_LOG   now.c
sh ../build/cygwin-wrapper cl  now.obj -DEBUG -DEBUGTYPE:CV  /Fenow.exe
Microsoft (R) 32-bit C/C++ Optimizing Compiler Version 12.00.8804 for 80x86
Copyright (C) Microsoft Corp 1984-1998. All rights reserved.

Microsoft (R) Incremental Linker Version 6.00.8447
Copyright (C) Microsoft Corp 1992-1998. All rights reserved.

/out:now.exe
now.obj
Stuart,

You are building from /builds, and Chris Seawood already stated that this was
unsupported - the only mount point that we support for the build is /cygdrive .
I agree with Chris' comment #137 . To build mozilla, you have to use a supported
build setup. The patch was tested on a multitude of setup and machines. I'm
marking it again RESOLVED FIXED . You should file a different bug if you want
the build system to support different mount points under cygwin.
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
I don't understand how you can say this is unsupported?  It worked prior to your
patch.  Your patch broke it.  It seems pretty clear that this is a regression.
You need to log in before you can comment on or make changes to this bug.