171753 - win32 taints srcdir during objdir builds

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect, P5)

Description

[Benjamin Smedberg]

I am using one source directory to build separate object directories on both
Windows and Linux.  Right now, /uriloader/exthandler/Makefile on Windows copies
/uriloader/exthandler/win/nsOSHelperAppService.cpp to /uriloader/exthandler/ in
the **source directory**.  This then breaks the linux build.  I don't understand
why we are copying this file, but if we really need to copy this file, it should
 be copied to MOZ_OBJDIR/uriloader/exthandler, so that the source directory
stays pristine.

The code to do the copying was added by seawood to fix bug 141843:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/uriloader/exthandler/Makefile.in

I'm afraid I don't know enough about the build config to write a patch to fix
this myself.

Comment 1

[Benjamin Smedberg]

Oops, that should be Bug 141834.

Comment 2

[hacker formerly known as [e-mail]]

The srcdir copying is a feature of the win32 build system.  Sorry, but you can't
share a source tree with the win32 build.  As described in bug 141834, the
alternative to copying that file is to use the cygwin-wrapper script everytime
cl.exe is called which puts a signficant performance hit on the build times.
 (You could also hack cygwin's make or use mingw make but that brings in a
separate issue of a non-standard tool requirement.  All of this was pointed out
in bug 141834.) There is a patch in bug 141834 which shows how to use
cygwin-wrapper instead of assuming that the source file is in the srcdir.

Comment 3

[Benjamin Smedberg]

OK, I've dug into the implications of Bug 141834 more fully, and I see the
problem... the implicit *.cpp rule requires the files to be in SRCDIR for it to
work correctly. 

How about using the "cygpath" function instead?  See the example snippet below.
 I suspect that it would not cut build times significantly, because you don't
have the same process overhead.  If you think this would work wihtout cutting
build times, I would be willing to accept this bug and try to create a
functional patch. (I don't have much experience with build configurations, so it
might take a little while).

Note also that /uriloader/exthandler/nsOSHelperAppService.cpp is the only
instance where the source copying actually breaks a source tree... all of the
other instances where we copy sources are either XPIDL-generated (and therefore
cross-platform) or in windows-specific directories.

*** rules.mk line#1135 ***
%.$(OBJ_SUFFIX): %.cc Makefile.in
	$(REPORT_BUILD)
	@$(MAKE_DEPS_AUTO)
ifdef _NO_AUTO_VARS
	$(ELOG) $(CCC) $(OUTOPTION)$@ -c $(COMPILE_CXXFLAGS) $(shell cygpath -w -a
$(<D))/$(<F)
else
	$(ELOG) $(CCC) $(OUTOPTION)$@ -c $(COMPILE_CXXFLAGS) $<
endif

Comment 4

[hacker formerly known as [e-mail]]

Each extra $(shell) in the general compile rule is going to make a signficant
impact on build times (as noted in bug 167254 with reference to the patches in
bug 163207).  We saw this in  Btw, the cygwin-wrapper script essentially does
what cygpath does except the script assumes that the cygdrive mount prefix is
/cygdrive  and it overcomes the cygpath problem of only being able to
translating a single path at a time.

Comment 5

[Benjamin Smedberg]

Attachment: -u patch to worksaround nsOSHelperAppService

OK, if that won't work, then how about this patch to workaround just the
nsOSHelperAppService problem...

Comment 6

[Benjamin Smedberg]

I've built for a while on linux and win32 with this patch, with no problems...
can you review it?

Comment 7

[hacker formerly known as [e-mail]]

Comment on attachment 101286 [details] [diff] [review]
-u patch to worksaround nsOSHelperAppService

mkdir -p is a GNU fileutils feature.  It's not supported on all verisions of
mkdir.	Also, I'm not sure how the dependency system is going to handle the
extra path in the source file list.  I'll need to test that.

Comment 8

[hacker formerly known as [e-mail]]

Attachment: avoid tainting srcdir

This patch adds another makefile variable, SRCS_IN_OBJDIR, that tells rules.mk
to use cygpath & $< to generate the name of the file to compile otherwise the
build will use $(srcdir)/... .	That is on win32, of course.  Other platforms
will always use $<.  This avoids tainting the srcdir when building.  pwd is
used to ensure that a full path is given so that the object file contains
debugging info (see bug 141834).

It still doesn't fix the specific problem listed here though.  I tried adapting
the previous patch to this scheme but it didn't work on win32 for some reason. 
Also, win32 seems to have a problem searching the subsequent dirs when multiple
dirs are given to VPATH so only eliminating the copy altogether isn't an
option.

Comment 9

[hacker formerly known as [e-mail]]

I misspoke.  That patch did fix this problem.  It copies the
nsOSHelperAppService.cpp file to the objdir instead of the srcdir and that
should be enough to fix the win32/linux overlap issue.