See http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1286873187.1286883818.7294.gz WINNT 5.2 tryserver build on 2010/10/12 01:46:27 { cd instgen && installer.nsi ./installer.nsi: line 43: syntax error near unexpected token `;' ./installer.nsi: line 43: `; Set verbosity to 3 (e.g. no script) to lessen the noise in the build logs' make[3]: Leaving directory `/e/builds/moz2_slave/tryserver-win32/build/obj-firefox/browser/installer/windows' ... make[3]: *** [instgen/setup.exe] Error 2 make[2]: *** [installer] Error 2 } I guess: *either 'make installer' should not be called. *or '--disable-installer' should be ignored/overridden, with a warning msg. *or build should error out early. As a workaround, it's easy not to use '--disable-installer'; yet, to save resources, it should be used (and maybe be default), I think. **** Or, I implicitly fully agree with the other way to fix this: Bug 603222 comment 1 { Mark Banner (:standard8) 2010-10-11 02:19:04 PDT (you could file a bug on making make installer work out nicely in the disabled installer case, but even then, I'm not sure its really necessary). }

See bug 603574 about a similar case.

This bug report is to ill-defined to action. If you're saying that --disable-installer on try doesn't work, then that's WONTFIX because we don't support that, nor do I believe it saves much in the way of resources. ie what Standard8 said. If you want the build system to play nice if you call make installer with --disable-installer then that belongs in Core::Build Config. However I don't like your chances their either.

We should probably just remove --disable-installer. MozillaBuild provides all the bits to build an installer anyway, and it's not as though we actually do much in the actual build if you don't disable it.

(In reply to comment #2) > If you're saying that --disable-installer on try doesn't work, then that's > WONTFIX because we don't support that I'm implicitly saying that unsupported features should be documented at https://wiki.mozilla.org/ReleaseEngineering/TryServer

(In reply to comment #4) > (In reply to comment #2) > > If you're saying that --disable-installer on try doesn't work, then that's > > WONTFIX because we don't support that > > I'm implicitly saying that unsupported features should be documented at > https://wiki.mozilla.org/ReleaseEngineering/TryServer I promise you that we are not going to list out everything that the try server doesn't do. If you want to add this specific thing, feel free.

Also: *Sort OS list and remove cygwin. *Fix documentation.

I think any non-standard feature is implicitly unsupported. The job of tryserver is to tell you what will happen on Tinderbox, not to check every possible build option/configuration.

Comment on attachment 483173 [details] [diff] [review] (Av1) Remove --disable-installer option from configure.in Maybe just move the AC_SUBST down to the big list of them near the bottom of configure.in?

Comment on attachment 483173 [details] [diff] [review] (Av1) Remove --disable-installer option from configure.in "approval2.0=?": Remove now superfluous NPOTDB configure option. Zero risk.

NB: Let's update error messages even on MOZILLA_2_0_BRANCH :-|

Afaict, this option is already useless.

Av1, with comment 8 suggestion(s), and unbitrotted. Succeeded as http://tbpl.mozilla.org/?tree=MozillaTry&rev=c03892af771b (I filed bug 650504.) http://tbpl.mozilla.org/?tree=MozillaTry&rev=3e17346c60bf http://tbpl.mozilla.org/?tree=MozillaTry&rev=e6e2e9e0b91d

Comment on attachment 526230 [details] [diff] [review] (Bv1-CC) Remove --disable-installer option from configure.in, except on MOZILLA_2_0_BRANCH Serge, I'll be branching comm-2.0 in the next few days for us; can you instead write this without the MOZ_2.0 switch and we can land as soon as the branch is done moving?

Bv1-CC, with comment 13 suggestion(s).

Comment on attachment 526528 [details] [diff] [review] (Bv1a-CC) Remove --disable-installer option from configure.in, except on MOZILLA_5_0_BRANCH Actually, thinking about it; I do not want to port this. We will still hit the m-c configure with a --disable-installer (which will fail), and in the near[er] future we'll be using m-c's configure directly. Adding this complexity switched off MOZILLA_5 is not worth the effort here.

(In reply to comment #13) > Comment on attachment 526230 [details] [diff] [review] > (Bv1-CC) Remove --disable-installer option from configure.in, except on > MOZILLA_2_0_BRANCH > > Serge, I'll be branching comm-2.0 in the next few days for us; can you instead > write this without the MOZ_2.0 switch and we can land as soon as the branch is > done moving? My comment here presumed that we did not need the switch for MOZILLA_5, if we don't need to switch this then I'm fine with taking it; if we do then I don't want the added complexity.

Comment on attachment 526232 [details] [diff] [review] (Cv1-Calendar) Remove overridden 'MOZ_INSTALLER=' option from confvars.sh [Pushed in bug 689370] Wow, sorry I didn't see this earlier. I need a new bugzilla view that shows me all calendar reviews in general, plus those where I am requestee!

Av1a, (context-)unbitrotted.

Av1a2, with (2) restored default values. NB: The option is removed, but not the variable.

Bv1a-CC, with comment 15 suggestion(s), and (context-)unbitrotted.

Comment on attachment 568419 [details] [diff] [review] (Av1b) Remove --disable-installer option from configure.in [Checked in: Comment 24] Review of attachment 568419 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +8290,5 @@ > AC_SUBST(MOZ_DEBUG_FLAGS) > AC_SUBST(MOZ_DEBUG_LDFLAGS) > AC_SUBST(WARNINGS_AS_ERRORS) > AC_SUBST(MOZ_EXTENSIONS) > +AC_SUBST(MOZ_INSTALLER) If we're going to unsupport this for real, can we remove MOZ_INSTALLER as well? I have no problem with doing that in a followup.

Ted, do you confirm I can go ahead with this now?

Please do.

Comment on attachment 568419 [details] [diff] [review] (Av1b) Remove --disable-installer option from configure.in [Checked in: Comment 24] https://hg.mozilla.org/mozilla-central/rev/901727f56830 Av1b, (context) unbitrotted.

Comment on attachment 568421 [details] [diff] [review] (Bv1b-CC) Remove --disable-installer option from configure.in [Checked in: Comment 25] http://hg.mozilla.org/comm-central/rev/f26d1ae04fff

Includes fix for bug 697147.

Also fixes a nit in /suite file.

The patch removing --disable-installer broke cross compilation. Ideally, cross compiling installer should be fixed. I've tried playing with that. nsis seems prepared for this. It has POSIX version that produces win32 binaries. Sadly, the Unicode NSIS fork totally broke this ability. That's why I think the solution is to make it still possible to compile without installer, but limit the code it affects so that it doesn't complicate supported configurations. The attached patch does it by making lack of Unicode NSIS not critical when cross compiling (this way no configure option is needed). Also, instead of using MOZ_INSTALLER, it uses MAKENSISU variable in a few makefile-related places, so that MOZ_INSTALLER can still be removed. I still need to test it a bit. Try submit is here: https://tbpl.mozilla.org/?tree=Try&rev=da753dc007d4

Comment on attachment 608559 [details] [diff] [review] (Cv1) Remove MOZ_INSTALLER support [Checked in: Comment 30] Review of attachment 608559 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/Makefile.in @@ +67,5 @@ > # uninstaller is included with the application for mar file generation. > libs:: > $(MAKE) -C installer/windows uninstaller > ifdef MOZ_MAINTENANCE_SERVICE > $(MAKE) -C installer/windows maintenanceservice_installer This feels like it could be cleaned up as a followup. If these targets all exist in installer/windows, why not just build them there instead of running an extra make?

Comment on attachment 608559 [details] [diff] [review] (Cv1) Remove MOZ_INSTALLER support [Checked in: Comment 30] https://hg.mozilla.org/mozilla-central/rev/df1f94b2bdee

Comment on attachment 608710 [details] [diff] [review] cross compiling fix v1.0 You should attach a "diff -w" too to ease review.

(In reply to Jacek Caban from comment #28) > it uses MAKENSISU The idea seems right... > https://tbpl.mozilla.org/?tree=Try&rev=da753dc007d4 It failed on Windows because packaging hasn't been fixed (yet).

(In reply to Ted Mielczarek [:ted] from comment #29) > ::: browser/Makefile.in > @@ +67,5 @@ > > # uninstaller is included with the application for mar file generation. > > libs:: > > $(MAKE) -C installer/windows uninstaller > > ifdef MOZ_MAINTENANCE_SERVICE > > $(MAKE) -C installer/windows maintenanceservice_installer > > This feels like it could be cleaned up as a followup. If these targets all > exist in installer/windows, why not just build them there instead of running > an extra make? I'm not too familiar with the installer, but would that be for (one of) the following reasons: 1) { # For Windows build the uninstaller during the application build since the # uninstaller is included with the application for mar file generation. } 2) Some other apps (like XulRunner, B2G, mobile, ...) doesn't have this "libs::" code. http://mxr.mozilla.org/mozilla-central/search?string=installer%2Fwindows&case=on&find=%2FMakefile http://mxr.mozilla.org/mozilla-central/search?string=MAKE.*installer%2Fwindows&regexp=on&case=1&find=%2FMakefile

(In reply to Serge Gautherie (:sgautherie) from comment #33) > Some other apps (like XulRunner, B2G, mobile, ...) doesn't have this > "libs::" code. Well, B2G and mobile should not be Windows targets, iiuc :-> I'm not sure whether XulRunner misses these lines or what.

Comment on attachment 608560 [details] [diff] [review] (Dv1-CC) Remove MOZ_INSTALLER support [Checked in: Comment 36] Review of attachment 608560 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Comment on attachment 608560 [details] [diff] [review] (Dv1-CC) Remove MOZ_INSTALLER support [Checked in: Comment 36] http://hg.mozilla.org/comm-central/rev/4d16576d3332

Comment on attachment 608710 [details] [diff] [review] cross compiling fix v1.0 This has bitrotted: please file and move it to a blocking bug.

(In reply to Ben Hearsum [:bhearsum] from comment #5) > > I'm implicitly saying that unsupported features should be documented at > > https://wiki.mozilla.org/ReleaseEngineering/TryServer > > I promise you that we are not going to list out everything that the try > server doesn't do. If you want to add this specific thing, feel free. No need to "promise": giving positive answer is more helpful... (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7) > I think any non-standard feature is implicitly unsupported. The job of > tryserver is to tell you what will happen on Tinderbox, not to check every > possible build option/configuration. Thanks. I added { Using a custom mozconfig [...] Note: * TryServer purpose is to tell what will happen on Tinderbox, not to check every possible build option/configuration. ** Any non-standard feature is implicitly unsupported. You may try them, but don't complain if they break. }

V.Fixed, per MXR search.