Closed Bug 660504 Opened 12 years ago Closed 12 years ago

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


(Toolkit :: Application Update, defect)

Not set





(Reporter: philor, Assigned: philor)




(1 file, 1 obsolete file)

Attached patch rm (obsolete) — Splinter Review 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/

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, is the result of building with an AC_SUBST(ANDROID) - since /config/ 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-
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.
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 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.
Attached patch the lite versionSplinter Review
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 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 knows if it is running on android
Attachment #537420 - Flags: review?(robert.bugzilla) → review+
Sadly, I lacked the courage of my convictions, so I actually downloaded an Android test package to make sure, but 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.)
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) :)
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Depends on: 668508
You need to log in before you can comment on or make changes to this bug.