Closed Bug 535369 Opened 15 years ago Closed 14 years ago

Fix bsdiff linking for builds compiled with CROSS_COMPILE

Categories

(Firefox Build System :: General, defect, P2)

defect

Tracking

(status1.9.2 .2-fixed, status1.9.1 .9-fixed)

RESOLVED FIXED
Tracking Status
status1.9.2 --- .2-fixed
status1.9.1 --- .9-fixed

People

(Reporter: coop, Assigned: coop)

References

Details

Attachments

(3 files)

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.
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.
No longer blocks: 511967
Component: Windows Mobile → Build Config
Product: Fennec → Core
QA Contact: mobile-windows → build-config
Summary: Building bsdiff on WinMo fails with 2 unresolved externals → Fix bsdiff linking for builds compiled with CROSS_COMPILE
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
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).
Assignee: nobody → ccooper
Status: NEW → ASSIGNED
Priority: -- → P2
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
Blocks: 511967
(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)))" ?
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.
Attachment #424625 - Flags: review?(ted.mielczarek)
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 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.
Yeah, sorry, my review queue got a bit backed up. I started unclogging it today, I'll definitely get this by tomorrow.
Depends on: 542222
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.
Attachment #424625 - Flags: review?(ted.mielczarek) → review+
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 on attachment 424625 [details] [diff] [review]
[checked in] Compile bsdiff for host OS rather than target

http://hg.mozilla.org/mozilla-central/rev/e3ae391d249f
Attachment #424625 - Attachment description: Compile bsdiff for host OS rather than target → [checked in] 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.
Attachment #427840 - Flags: review+
Attachment #427840 - Flags: approval1.9.2.2?
Carrying forward review from Ted, this just needs to land on the 1.9.1 branch as well.
Attachment #427841 - Flags: review+
Attachment #427841 - Flags: approval1.9.1.9?
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 ;-)
Attachment #427841 - Flags: approval1.9.1.9? → approval1.9.1.9+
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 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
Attachment #427840 - Flags: approval1.9.2.2? → approval1.9.2.2+
Marking bug FIXED on mozilla-centreal per comment 12. Branch status we track with the status1.9.1/status1.9.2 fields
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
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
Attachment #427840 - Attachment description: [1.9.2] Compile bsdiff for host OS rather than target → [checked in][1.9.2] Compile bsdiff for host OS rather than target
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
Attachment #427841 - Attachment description: [1.9.1] Compile bsdiff for host OS rather than target → [checked in][1.9.1] Compile bsdiff for host OS rather than target
(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.
(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
Depends on: 547955
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: