Last Comment Bug 535369 - Fix bsdiff linking for builds compiled with CROSS_COMPILE
: Fix bsdiff linking for builds compiled with CROSS_COMPILE
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: Chris Cooper [:coop] [away until Aug 29]
:
Mentors:
Depends on: 542222 547955
Blocks: 511967
  Show dependency treegraph
 
Reported: 2009-12-16 13:25 PST by Chris Cooper [:coop] [away until Aug 29]
Modified: 2010-02-23 05:19 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.2-fixed
.9-fixed


Attachments
[checked in] Compile bsdiff for host OS rather than target (2.43 KB, patch)
2010-02-01 10:31 PST, Chris Cooper [:coop] [away until Aug 29]
ted: review+
Details | Diff | Splinter Review
[checked in][1.9.2] Compile bsdiff for host OS rather than target (2.47 KB, patch)
2010-02-19 13:30 PST, Chris Cooper [:coop] [away until Aug 29]
coop: review+
dveditz: approval1.9.2.2+
Details | Diff | Splinter Review
[checked in][1.9.1] Compile bsdiff for host OS rather than target (2.44 KB, patch)
2010-02-19 13:31 PST, Chris Cooper [:coop] [away until Aug 29]
coop: review+
dveditz: approval1.9.1.9+
Details | Diff | Splinter Review

Description Chris Cooper [:coop] [away until Aug 29] 2009-12-16 13:25:08 PST
As part of bug 511967, I need to get the bsdiff tool building on the slaves when --enable-update-packaging is specified. This is working out-of-the-box on Linux/Mac/Windows, but is currently failing on WinMo (and WinCE, same symptoms). 

Here's the patch I ran through try-server:

http://pastebin.mozilla.org/691229

And here's the brief tinderbox log for the WinMo failure:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1260995123.1260996702.30857.gz#err4

WinCE failure is near-identical, so I'm hoping there is a single solution:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1260995122.1260996857.32737.gz#err4

This won't prevent us from getting updates running for WinMo (Bug 534025) but it will prevent us from generating those updates on the slaves.
Comment 1 Chris Cooper [:coop] [away until Aug 29] 2009-12-16 19:49:28 PST
After talking with blassey on IRC today, he determined that how we build bsdiff is pretty broken. bsdiff *should* be getting built for the host (indeed, it gets put in dist/host/bin), but it gets linked against the version of nspr built for (in this case) WinMo.

We never really planned to use bsdiff in this way, so this doesn't surprise me terribly. blassey suggested that I put the bsdiff section in toolkit-tiers.mk in an "ifndef CROSS_COMPILE." This worked on the try-server and effectively unblocks me on bug 511967.

Still, if we ever want to do updates-on-slaves for WinMo via the same code path, we'll need to fix how bsdiff gets linked.
Comment 2 Benjamin Smedberg [:bsmedberg] 2009-12-17 02:24:01 PST
I think you mean the target version of libbz2, not NSPR. It should be fairly trivial to build a host version of libbz2: just set HOST_LIBRARY_NAME and HOST_CSRCS in modules/libbz2/src/Makefile.in
Comment 3 Ted Mielczarek [:ted.mielczarek] 2009-12-17 04:07:23 PST
I do something similar in the Breakpad code:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/common/linux/Makefile.in

Since I need to build some of that code for both the host (for dump_syms) and the target (for the crashreporter itself).
Comment 4 Chris Cooper [:coop] [away until Aug 29] 2010-01-29 12:29:00 PST
I'm taking this on because I *think* I can muddle my way through the Makefile changes given enough pointers.

I have bsdiff attempting to build now by default when --enable-update-packaging is specified. However, I'm hitting the following problem when I try to compile bsdiff for WinCE (on a standard Win2K3 build slave in staging):

http://pastebin.mozilla.org/700308

I am assuming this is because of how HOST_LIBS is defined for bsdiff:

http://mxr.mozilla.org/mozilla-central/source/other-licenses/bsdiff/Makefile.in#50

What's the right way to change how the BZ2_LIBS are included to make sure I pick up host_bz2.lib when required (and not break the system vs. moz bz2 distinction also)?

Here's my current WIP patch:

http://pastebin.mozilla.org/700309
Comment 5 Chris Cooper [:coop] [away until Aug 29] 2010-01-29 18:43:52 PST
(In reply to comment #4)
> What's the right way to change how the BZ2_LIBS are included to make sure I
> pick up host_bz2.lib when required (and not break the system vs. moz bz2
> distinction also)?

Ted: I suppose the above question is meant for you.

I've successfully made host_bz2.lib with the above steps, but bsdifff linking still fails because it's trying to pull in ../../modules/libbz2/src/bz2.lib and "unresolved external symbol __imp__htonl@4" is not getting pulled in from Ws2_32 when building under WinCE.

I can get mbsdiff.exe to link for WinCE if I execute the following:

$ cd obj-firefox/other-licenses/bsdiff
$ link -NOLOGO -OUT:mbsdiff.exe host_bsdiff.obj ../../dist/host/lib/hostbz2.lib Ws2_32.lib

I guess that narrows it down to two questions:

1) How to set HOST_LIBS properly so that host_bz2.lib gets picked up? Should I be ifdef-ing on some var, and if so, which one? 

2) How to set HOST_EXTRA_LIBS in the CROSS_COMPILE case (WinCE is this case)? Do I need something like "ifeq (,$(filter WINCE WINNT,$(OS_ARCH)))" ?
Comment 6 Chris Cooper [:coop] [away until Aug 29] 2010-02-01 10:31:23 PST
Created attachment 424625 [details] [diff] [review]
[checked in] Compile bsdiff for host OS rather than target

This patch seems to work on WinCE/Win2k3, so I'm running it through the tryserver to make sure it doesn't break anything else before asking for review.
Comment 7 Chris Cooper [:coop] [away until Aug 29] 2010-02-01 12:55:54 PST
Comment on attachment 424625 [details] [diff] [review]
[checked in] Compile bsdiff for host OS rather than target

bsdiff created successfully on all platforms on try, now Ted can tell me how I'm doing it all wrong. ;)
Comment 8 Chris Cooper [:coop] [away until Aug 29] 2010-02-03 14:38:12 PST
Comment on attachment 424625 [details] [diff] [review]
[checked in] Compile bsdiff for host OS rather than target

Ted: gentle ping about the review here. 

This will have cascade unblocking effects with other nightly updates work if I can get it landed. Mostly I just want to know if I'm on the right track since I don't often muck around in Makefiles.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2010-02-03 16:25:22 PST
Yeah, sorry, my review queue got a bit backed up. I started unclogging it today, I'll definitely get this by tomorrow.
Comment 10 Ted Mielczarek [:ted.mielczarek] 2010-02-08 15:26:58 PST
Comment on attachment 424625 [details] [diff] [review]
[checked in] Compile bsdiff for host OS rather than target

This looks reasonable. I'm not sure why we haven't hoisted that HOST_NSPR_MDCPUCFG bit into config.mk or somewhere like that. I'm also not sure if you want to parrot the .NOTPARALLEL bit from a similar makefile:
http://mxr.mozilla.org/mozilla-central/source/modules/libmar/src/Makefile.in#75

I know we're not actually building with -save-temps anywhere anymore, so maybe it doesn't matter.
Comment 11 Chris Cooper [:coop] [away until Aug 29] 2010-02-12 06:59:25 PST
There was a little bit of churn in this area of code with the changes for bug 542222, so I'm going to run this through the tryserver again with .NOTPARALLEL set (in progress) before landing.
Comment 12 Chris Cooper [:coop] [away until Aug 29] 2010-02-19 09:04:03 PST
Comment on attachment 424625 [details] [diff] [review]
[checked in] Compile bsdiff for host OS rather than target

http://hg.mozilla.org/mozilla-central/rev/e3ae391d249f
Comment 13 Chris Cooper [:coop] [away until Aug 29] 2010-02-19 13:30:14 PST
Created attachment 427840 [details] [diff] [review]
[checked in][1.9.2] Compile bsdiff for host OS rather than target

Carrying forward review from Ted, this just needs to land on the 1.9.2 branch as well.
Comment 14 Chris Cooper [:coop] [away until Aug 29] 2010-02-19 13:31:18 PST
Created attachment 427841 [details] [diff] [review]
[checked in][1.9.1] Compile bsdiff for host OS rather than target

Carrying forward review from Ted, this just needs to land on the 1.9.1 branch as well.
Comment 15 neil@parkwaycc.co.uk 2010-02-21 02:19:42 PST
Comment on attachment 424625 [details] [diff] [review]
[checked in] Compile bsdiff for host OS rather than target

>+ifneq (,$(filter WINCE WINNT,$(OS_ARCH)))
$(HOST_OS_ARCH)

Guess who cross-compiles around here ;-)
Comment 16 Daniel Veditz [:dveditz] 2010-02-22 10:48:58 PST
Comment on attachment 427841 [details] [diff] [review]
[checked in][1.9.1] Compile bsdiff for host OS rather than target

Approved for 1.9.1.9, a=dveditz for release-drivers
Comment 17 Daniel Veditz [:dveditz] 2010-02-22 10:49:19 PST
Comment on attachment 427840 [details] [diff] [review]
[checked in][1.9.2] Compile bsdiff for host OS rather than target

Approved for 1.9.2.2, a=dveditz for release-drivers
Comment 18 Daniel Veditz [:dveditz] 2010-02-22 10:50:29 PST
Marking bug FIXED on mozilla-centreal per comment 12. Branch status we track with the status1.9.1/status1.9.2 fields
Comment 19 Chris Cooper [:coop] [away until Aug 29] 2010-02-22 11:04:34 PST
Comment on attachment 427840 [details] [diff] [review]
[checked in][1.9.2] Compile bsdiff for host OS rather than target

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8f07bc479476
Comment 20 Chris Cooper [:coop] [away until Aug 29] 2010-02-22 11:30:46 PST
Comment on attachment 427841 [details] [diff] [review]
[checked in][1.9.1] Compile bsdiff for host OS rather than target

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/90fbd13348e0
Comment 21 Chris Cooper [:coop] [away until Aug 29] 2010-02-22 11:42:12 PST
(In reply to comment #15)
> (From update of attachment 424625 [details] [diff] [review])
> >+ifneq (,$(filter WINCE WINNT,$(OS_ARCH)))
> $(HOST_OS_ARCH)

Fix included on branch landings, will land on m-c shortly.
Comment 22 Chris Cooper [:coop] [away until Aug 29] 2010-02-22 19:28:53 PST
(In reply to comment #21)
> Fix included on branch landings, will land on m-c shortly.

...and done:

http://hg.mozilla.org/mozilla-central/rev/ebdeb3b29736

Note You need to log in before you can comment on or make changes to this bug.