660504 - Bug 601518's attempt to shield Android from tests it wouldn't run anyway would have broken the build, except it used an undefined ifndef

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Phil Ringnalda (:philor)]

Attachment: rm

http://hg.mozilla.org/mozilla-central/rev/b69aa5ff29d3 from bug 601518 tried to stop Android from seeing the tests it added, with a pair of ifndef ANDROID blocks in toolkit/mozapps/update/test/Makefile.in.

ANDROID isn't (at least before bug 660497) AC_SUBSTed, only AC_DEFINEd, so that ifndef is alway true, and all along Android has been doing the stuff inside those blocks, which is just fine since it's only being (not actually) shielded from some chrome tests that it doesn't run anyway.

Unfortunately, http://tinderbox.mozilla.org/showlog.cgi?log=Try/1306627388.1306628861.23143.gz is the result of building with an AC_SUBST(ANDROID) - since /config/rules.mk is included within the ifndef block, there's no targets for ifdef ANDROID.

We *could* fix it so the ifndef works, but since we've actually been building without it ever since the patch landed last October, it's hard to see why we should bother.

Comment 1

[Robert Strong (they/them - no direct email)]

Comment on attachment 535894 [details] [diff] [review]
rm

I'd prefer to fix this so it doesn't include those tests, etc. when building on Android to avoid potential future problems where things could break.

Comment 2

[Phil Ringnalda (:philor)]

Say you were going to put a comment above the ifdef explaining why it was there - what would it say?

Comment 3

[Robert Strong (they/them - no direct email)]

Something along the lines of

The mochitest-chrome and binary application update tests are skipped for Android since the updater binary and the application update user interface are not used on Android. Some xpcshell tests are excluded as well.

Comment 4

[Phil Ringnalda (:philor)]

Ow, my head.

So if you unset MOZ_UPDATER, you get nsUpdateTimerManager, test_timermanager/ and the prefs UI in content/ anyway.

If you set MOZ_UPDATER and are named Android, then you really set an imaginary MOZ_UPDATESERVICE to build in the imaginary updateservice/ with the nsUpdateService files, and ../readstrings, and the xpcshell tests that put the lie to http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/Makefile.in#69 because it's really only the compiled code and chrome tests that require the updater binary, except for the seven xpcshell tests that do, which bail out early because xpcshell manifests didn't yet exist.

If you set MOZ_UPDATER and are not named Android, then you set both MOZ_UPDATESERVICE and MOZ_UPDATER to build in updater/ and get the compiled code and chrome tests.

If we weren't also at war with excess makefiles, I'd be tempted to make that explicit with

update/
  test_timermanager/
  updateservice/
    unit/
  updater/
    chrome/
    unit/

Also, isn't ../readstrings getting built in the wrong place? It looks like an updater/ dependency, not an updateservice/ dependency.

Comment 5

[Phil Ringnalda (:philor)]

Attachment: the lite version

I got partway through doing an updateservice/ patch, and hit the part where I'd have to do a bunch of copy-pasting or awkward sharing between the updateservice/ xpcshell tests and the updater/ ones, and this started looking better :)

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 537420 [details] [diff] [review]
the lite version

This is the approach I thought you would use. Yes, readstrings is being built in the wrong place... it was changed so mobile could use it independently and it got messed up then. There is also an #ifdef ANDROID in head_update.js.in... could you add a define so it picks it up as well?

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 537420 [details] [diff] [review]
the lite version

r=me with the define so head_update.js.in knows if it is running on android

Comment 8

[Phil Ringnalda (:philor)]

Sadly, I lacked the courage of my convictions, so I actually downloaded an Android test package to make sure, but head_update.js.in is fine. We AC_DEFINE(ANDROID) in configure, which lets it be used in both C++ and the preprocessor, and only doesn't let it be used in makefiles.

(Plus you don't really *have* to do that anymore, since the whole reason xpcshell manifests now exist is so that it will be possible to do xpcshell on Android without having to repeat that preprocessed head_ defining a const that says to bail out all over the tree. Once I make sure we really are actually already using them, I'll file and patch a followup to use the manifest to control who runs what instead of all that preprocessing.)

Comment 9

[Phil Ringnalda (:philor)]

Also sadly, we apparently aren't really entirely using xpcshell manifests just yet. Shame, it's a pretty patch, and I even get to create my (xpcshell_)updater(.ini) :)

Comment 10

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/3c4b12d057ef