299407 - fix VC6 cairo/pixman builds

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'll go and mark the relevant bugs as depending on this one shortly... the gist
of this fix is that I compiled a VC7.1 pixman.lib, and plan on checking the
compiled version into the tree.  On win32-msvc builds, we just overwrite the one
we build with this one.  It seems to work fine, though I wouldn't mind some
independent verification.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: build patch

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Bugzilla won't let me attach the lib, but it's at
http://rig.vlad1.com/~vladimir/misc/pixman-vc71.lib

Needs to go into gfx/cairo/libpixman/src/

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 187980 [details] [diff] [review]
build patch

Do we really want to do that for _all_ msvc builds?  Seems like we should be
blacklisting known-evil versions.  We can tidy that up after 1.1a2/1.8b3,
though.

r+a=shaver; checking in a (temporary?) copy of pixman isn't any worse than the
copy of batik we have!

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #3)
> (From update of attachment 187980 [details] [diff] [review] [edit])
> Do we really want to do that for _all_ msvc builds?  Seems like we should be
> blacklisting known-evil versions.  We can tidy that up after 1.1a2/1.8b3,
> though.

My plan is to just undo this hack on the trunk after we branch, which is
presumably right after 1.1a2..

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

checked in.. waiting for tinderbox now.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The checkin for this broke beast, because permissions got screwed up.  Yay.  I
disabled it in CVS, but it can be reenabled by uncommenting the line in
gfx/cairo/pixman/src/Makefile.am.  I'll fix it for good once I have someone
handy who can poke at the tinderboxes to make them go.

Comment 7

[Chase Phillips]

Comment on attachment 187980 [details] [diff] [review]
build patch

>+	$(RM) $(DIST)/lib/$(LIBRARY)
>+	$(INSTALL) $(IFLAGS1) $(srcdir)/pixman-vc71.lib $(DIST)/lib/$(LIBRARY)

This nsinstall command creates a directory dist/lib/mozlibpixman.lib and copies
pixman-vc71.lib into it.  The build fails because it expects to open a file
mozlibpixman.lib which turns out to be a directory.

Comment 8

[Chase Phillips]

pacifica is affected, too.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #7)
> (From update of attachment 187980 [details] [diff] [review] [edit])
> >+	$(RM) $(DIST)/lib/$(LIBRARY)
> >+	$(INSTALL) $(IFLAGS1) $(srcdir)/pixman-vc71.lib $(DIST)/lib/$(LIBRARY)
> 
> This nsinstall command creates a directory dist/lib/mozlibpixman.lib and copies
> pixman-vc71.lib into it.  The build fails because it expects to open a file
> mozlibpixman.lib which turns out to be a directory.

I was afraid of that.  We seem to be using two different nsinstalls here -- this
works fine with mine, which is from the moztools dist (size 30208 bytes, Apr 28
1998).  The wrong one seems to always assume the destination is a directory...
nsinstall doesn't seem to have any help output or anything, so I've no idea what
the arguments it really expects are.

Comment 10

[Chase Phillips]

Local test of nsinstall on beast (which is also the same size and timestamp as
vlad's) treats the second argument as a directory, creating what parts that
don't exist, and copies the source file into the destination path.

In the log output, c:/moztools/bin/nsinstall is called directly for copying
mozlibpixman.lib.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: fix using cp instead of nsinstall

Checked in followup to just use cp instead of nsinstall.  nsinstall seems to
always assume that the target is a directory, unless the basename of that path
is identical to the source name, at which point it just copies it in. 
nsinstall still has no --help or similar.  Maybe we should just use gnu
install...

This patch should fix it, and shuold fix the tinderboxes (by deleting the
directory that was created).

Comment 12

[Chase Phillips]

Attachment: copy pixman-vc71.lib over mozlibpixman.lib on winnt builds

Tested this patch locally on beast already.  Will commit and respin.

Comment 13

[Chase Phillips]

Comment on attachment 188271 [details] [diff] [review]
copy pixman-vc71.lib over mozlibpixman.lib on winnt builds

Checking in Makefile.in;
/cvsroot/mozilla/gfx/cairo/libpixman/src/Makefile.in,v	<--  Makefile.in
new revision: 1.10; previous revision: 1.9
done

Comment 14

[Chase Phillips]

The first build issue is resolved.  A new issue has appeared.  It appears the
currently committed pixman-vc71.lib is incompatible with the vc6 build.  The error:

   Creating library firefox.lib and object firefox.exp
LINK : warning LNK4098: defaultlib "LIBC" conflicts with use of other libs; use
/NODEFAULTLIB:library
mozlibpixman.lib(renderedge.obj) : fatal error LNK1103: debugging information
corrupt; recompile module
make[4]: *** [firefox.exe] Error 79

Comment 15

[Chase Phillips]

Hmm.  vlad doesn't look to be around atm.  I will comment out the vc71 portion
of this build so we can get Windows trunk green again and will leave it to him
to turn it back on when a compatible lib is put in place.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Thanks Chase; I'm not sure why the debug information issue is showing up, but
I'll recompile without debugging information.  I wonder if I'm locally ending up
with the VC71 linker instead of the VC6 one.. I'll look tomorrow/tuesday.

Comment 17

[Chase Phillips]

If I understand the windows build process completely, we also need a copy of
mozlibpixman.pdb for talkback symbols.  If that's the case we'll need you to
commit that, too.

Comment 18

[timeless]

Comment on attachment 188271 [details] [diff] [review]
copy pixman-vc71.lib over mozlibpixman.lib on winnt builds

arg. cp screws up file permissions. please don't use it *ever*.

Comment 19

[Benjamin Smedberg]

I don't get it, why isn't it sufficient to do

libs::
  $(INSTALL) $(srcdir)/pixman-vc71.lib $(DIST)

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #18)
> (From update of attachment 188271 [details] [diff] [review] [edit])
> arg. cp screws up file permissions. please don't use it *ever*.

cp is used to set up the file to have the right name, and then nsinstall is used
to install it into dist/lib, with the right permissions.

(In reply to comment #19)
> I don't get it, why isn't it sufficient to do
> 
> libs::
>   $(INSTALL) $(srcdir)/pixman-vc71.lib $(DIST)

Because the resulting filename needs to be mozlibpixman.lib, not
pixman-vc71.lib, and nsinstall can't install to a file named differently than
the source file.

Comment 21

[timeless]

i'd rather the file be nsinstalled with the wrong name first and mv'd to rename
it second. what's controlling the name of the file? if it's in cvs, why can't
cvs have $(srcdir)/vc71/mozlibpixman.lib or something like it?

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #21)
> i'd rather the file be nsinstalled with the wrong name first and mv'd to rename
> it second. what's controlling the name of the file? if it's in cvs, why can't
> cvs have $(srcdir)/vc71/mozlibpixman.lib or something like it?

We can do any of 50 things; this is a very temporary band-aid being put in for
1.1.  As soon as we branch, this band aid is being removed from the trunk.  I'm
honestly more interested in getting this to work for our builds, than trying to
get it exactly perfect.

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ok, working version of pixman-vc71.lib checked in.  Only enabled on optimized
builds, as pixman-vc71.lib explicitly requests the multi-threaded non-debug dll
runtime (which is what we use for opt builds), and tinderbox is happy.

Comment 24

[amano]

Does that mean that Firefox should build now in VC6 with 

ac_add_options --enable-svg
ac_add_options --enable-svg-renderer=cairo    in the .mozconfig?

For me it still doesn't and gives a "cairo-win32-private.h(43) : warning C4005:
'WINVER" and a "cairo-win32-surface.c(130) : warning C4244: '=' : 'const double
' in 'long ', possible data loss" warning.

Or are there special cygwin (?) packages for cairo to be installed?

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #24)
> Does that mean that Firefox should build now in VC6 with 
> 
> ac_add_options --enable-svg
> ac_add_options --enable-svg-renderer=cairo    in the .mozconfig?
> 
> For me it still doesn't and gives a "cairo-win32-private.h(43) : warning C4005:
> 'WINVER" and a "cairo-win32-surface.c(130) : warning C4244: '=' : 'const double
> ' in 'long ', possible data loss" warning.
> 
> Or are there special cygwin (?) packages for cairo to be installed?

This isn't related to any cairo/svg/win32 issues, just solely a fix for pixman
miscompilation issues.  I'm not sure what the win32 issue is; builds fine here
and on the tinderboxes..

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #24)
> For me it still doesn't and gives a "cairo-win32-private.h(43) : warning C4005:
> 'WINVER" and a "cairo-win32-surface.c(130) : warning C4244: '=' : 'const double
> ' in 'long ', possible data loss" warning.

Those two lines do not explain any problems compiling cairo, since they are just
warnings, not errors.

Comment 27

[amano]

Sorry Chris. I just numbered the warnings and forgot the error. I don't even
know if this is related to this bug or not:

[quote]
 Konvertierung von 'long ' in 'unsigned short ', moeglicher Datenverlust
c:/mozilla/gfx/cairo/cairo/src/cairo-win32-surface.c(956) : warning C4244: '=' :
 Konvertierung von 'long ' in 'unsigned short ', moeglicher Datenverlust
make[5]: *** [cairo-win32-surface.obj] Error 2
make[5]: Leaving directory `/cygdrive/c/mozilla/gfx/cairo/cairo/src'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/cygdrive/c/mozilla/gfx/cairo'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/c/mozilla/gfx'
make[2]: *** [tier_9] Error 2
make[2]: Leaving directory `/cygdrive/c/mozilla'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/cygdrive/c/mozilla'
make: *** [build] Error 2
[\quote]

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

that still does not show the error...

Comment 29

[Michal Tuszynski]

Attachment: Makefile.in diff

Comment 30

[Michal Tuszynski]

This fix breaks VC8 builds. The problem is that pixman-vc71.lib links, as its default library, the libc.lib (check with dumpbin /directives pixman-vc71.lib) but there's no such library in VC8 (I use Visual Studio 2005 Beta 2).
A simple patch to Makefile.in which tests for VC8 solves this problem (see attachment).

Comment 31

[Michal Tuszynski]

Sorry, I forgot to add that it breaks Firefox _branch_ builds (MOZILLA_1.8_BRANCH) as a result of landing this bug on the branch in bug 307155

Comment 32

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Branch VC8 builds are uninteresting; if someone wants to make personal VC8 builds, they'll have to make the fix in the makefile directly (just get rid of pixman-vc71 stuff entirely, as it's not needed for VC8).

This workaround is already gone from the trunk, so it shouldn't be an issue there.