1572844 - Build broken with --disable-bits-download: undefined symbol: _new_bits_service

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P3)

Description

[(Away)]

[task 2019-08-09T19:27:00.817Z] 19:27:00 INFO - lld-link: error: undefined symbol: _new_bits_service

Comment 1

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

I think this is because we always build the bitsdownload component. I think that should be conditional on MOZ_BITS_DOWNLOAD, which is what controls whether the Rust side gets built (that's what's missing here).

This only wasn't an issue on non-Windows because the C++ side is only built on Windows, and the component requires both MOZ_BITS_DOWNLOAD and Windows.

For consistency all these should be doing the same check; I'm inclined to believe it should be MOZ_BITS_DOWNLOAD since --enable-bits-download only works on Windows.

---

At least one other wrinkle: Services.py always generates three files with Bits and nsIBits included.

I think that should be conditional as well, there's no reason to have the service, or interface, if there's no BITS support. I'm currently testing this.

• Build with --disable-bits-download: Fails as reported
• Build with --enable-bits-download on linux64: Denied
• Build with --disable-bits-download and proposed fixes: pending

Comment 2

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

References to nsIBits and Bits.jsm are all throughout UpdateService.jsm and UpdateTelemetry.jsm. It doesn't look like I'm going to be able to remove that without a lot of refactoring. The import would ideally be conditional on AppConstants.MOZ_BITS_DOWNLOAD, but I don't know how far the changes would have to spread for that to be safe.

I'm going to try just adding a MOZ_BITS_DOWNLOAD check to the moz.build to avoid bringing in the C++ that links to the missing symbol, this should be equivalent to how it works now on non-Windows targets.

try:

• normal
• --disable-bits-download

Comment 4

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Attachment: Bug 1572844 - Consistently check for MOZ_BITS_DOWNLOAD. r?bytesized

BITS download can be disabled on Windows. This patch checks for the
config/define MOZ_BITS_DOWNLOAD when including the C++ parts of the
service, exposing the service in JS, setting the pref, and including
BITS-specific tests. For consistency and simplicity it also removes
the Windows checks; the configure system won't allow
MOZ_BITS_DOWNLOAD if not on Windows.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ce6426b7653
Consistently check for MOZ_BITS_DOWNLOAD. r=bytesized

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7ce6426b7653

Comment 7

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

Comment on attachment 9088231 [details]
Bug 1572844 - Consistently check for MOZ_BITS_DOWNLOAD. r?bytesized

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: ESR is used (vs the faster rolling releases) as the basis of building a number of other projects, and they may want to build with --disable-bits-download (to avoid linking to QmgrPrxy.dll, or reduce build size or time, or for security purposes). In particular, Tor Browser needs this, see bug 1576621.
• User impact if declined: Anyone wanting to build for Windows with --disable-bits-download would have to cherry-pick this patch, or do their own work in patching out BITS support.
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This shouldn't affect our own builds at all, and the automated tests for the fallback update mechanism (incremental downloader) are still passing with --disable. I've tested grafting 7ce6426b7653 from mozilla-central and it seems ok: normal, --disable
• String or UUID changes made by this patch:

Comment 8

[Julien Cristau [:jcristau]]

Comment on attachment 9088231 [details]
Bug 1572844 - Consistently check for MOZ_BITS_DOWNLOAD. r?bytesized

build fix for windows in a non-default config, approved for 68.2

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/b32fbb17f74e