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

RESOLVED FIXED in mozilla7



8 years ago
8 years ago


(Reporter: philor, Assigned: philor)


Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



8 years ago
Created attachment 535894 [details] [diff] [review]

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.
Attachment #535894 - Flags: review?(robert.bugzilla)
Comment on attachment 535894 [details] [diff] [review]

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.
Attachment #535894 - Flags: review?(robert.bugzilla) → review-

Comment 2

8 years ago
Say you were going to put a comment above the ifdef explaining why it was there - what would it say?
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

8 years ago
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


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

Comment 5

8 years ago
Created attachment 537420 [details] [diff] [review]
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 :)
Attachment #535894 - Attachment is obsolete: true
Attachment #537420 - Flags: review?(robert.bugzilla)
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 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
Attachment #537420 - Flags: review?(robert.bugzilla) → review+

Comment 8

8 years ago
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

8 years ago
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

8 years ago
Last Resolved: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla7


8 years ago
Depends on: 668508
You need to log in before you can comment on or make changes to this bug.