Last Comment Bug 604266 - Remove '--disable-installer' option and 'MOZ_INSTALLER' variable
: Remove '--disable-installer' option and 'MOZ_INSTALLER' variable
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86 Windows Server 2003
: -- minor (vote)
: mozilla14
Assigned To: Serge Gautherie (:sgautherie)
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 650322 697147 736739 739188 741340
Blocks: 603222
  Show dependency treegraph
 
Reported: 2010-10-13 18:30 PDT by Serge Gautherie (:sgautherie)
Modified: 2012-04-02 04:31 PDT (History)
8 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix


Attachments
(Av1) Remove --disable-installer option from configure.in (4.37 KB, patch)
2010-10-14 08:06 PDT, Serge Gautherie (:sgautherie)
ted: review+
benjamin: approval2.0-
Details | Diff | Splinter Review
(Bv1-CC) Remove --disable-installer option from configure.in, except on MOZILLA_2_0_BRANCH (3.72 KB, patch)
2011-04-15 06:33 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Cv1-Calendar) Remove overridden 'MOZ_INSTALLER=' option from confvars.sh [Pushed in bug 689370] (912 bytes, patch)
2011-04-15 06:36 PDT, Serge Gautherie (:sgautherie)
philipp: review+
Details | Diff | Splinter Review
(Av1a) Remove --disable-installer option from configure.in (4.37 KB, patch)
2011-04-16 09:01 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Bv1a-CC) Remove --disable-installer option from configure.in, except on MOZILLA_5_0_BRANCH (3.72 KB, patch)
2011-04-16 15:36 PDT, Serge Gautherie (:sgautherie)
bugspam.Callek: review-
Details | Diff | Splinter Review
(Av1a2) Remove --disable-installer option from configure.in (4.27 KB, patch)
2011-10-20 09:08 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
(Av1b) Remove --disable-installer option from configure.in [Checked in: Comment 24] (3.55 KB, patch)
2011-10-20 09:30 PDT, Serge Gautherie (:sgautherie)
ted: review+
Details | Diff | Splinter Review
(Bv1b-CC) Remove --disable-installer option from configure.in [Checked in: Comment 25] (3.64 KB, patch)
2011-10-20 09:41 PDT, Serge Gautherie (:sgautherie)
bugspam.Callek: review+
Details | Diff | Splinter Review
(Cv1) Remove MOZ_INSTALLER support [Checked in: Comment 30] (5.70 KB, patch)
2012-03-22 18:41 PDT, Serge Gautherie (:sgautherie)
ted: review+
Details | Diff | Splinter Review
(Dv1-CC) Remove MOZ_INSTALLER support [Checked in: Comment 36] (4.93 KB, patch)
2012-03-22 18:52 PDT, Serge Gautherie (:sgautherie)
bugspam.Callek: review+
Details | Diff | Splinter Review
cross compiling fix v1.0 (6.35 KB, patch)
2012-03-23 07:57 PDT, Jacek Caban
no flags Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2010-10-13 18:30:12 PDT
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).
}
Comment 1 Serge Gautherie (:sgautherie) 2010-10-13 18:32:46 PDT
See bug 603574 about a similar case.
Comment 2 Nick Thomas [:nthomas] 2010-10-13 18:37:55 PDT
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.
Comment 3 Ted Mielczarek [:ted.mielczarek] 2010-10-14 05:46:24 PDT
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.
Comment 4 Serge Gautherie (:sgautherie) 2010-10-14 07:59:00 PDT
(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
Comment 5 Ben Hearsum (:bhearsum) 2010-10-14 08:01:45 PDT
(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.
Comment 6 Serge Gautherie (:sgautherie) 2010-10-14 08:06:24 PDT
Created attachment 483173 [details] [diff] [review]
(Av1) Remove --disable-installer option from configure.in

Also:
*Sort OS list and remove cygwin.
*Fix documentation.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2010-10-14 08:23:26 PDT
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 8 Ted Mielczarek [:ted.mielczarek] 2010-10-14 11:36:26 PDT
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 9 Serge Gautherie (:sgautherie) 2010-10-14 15:34:50 PDT
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.
Comment 10 Serge Gautherie (:sgautherie) 2011-04-15 06:33:22 PDT
Created attachment 526230 [details] [diff] [review]
(Bv1-CC) Remove --disable-installer option from configure.in, except on MOZILLA_2_0_BRANCH

NB: Let's update error messages even on MOZILLA_2_0_BRANCH :-|
Comment 11 Serge Gautherie (:sgautherie) 2011-04-15 06:36:25 PDT
Created attachment 526232 [details] [diff] [review]
(Cv1-Calendar) Remove overridden 'MOZ_INSTALLER=' option from confvars.sh
[Pushed in bug 689370]

Afaict, this option is already useless.
Comment 12 Serge Gautherie (:sgautherie) 2011-04-16 09:01:05 PDT
Created attachment 526499 [details] [diff] [review]
(Av1a) Remove --disable-installer option from configure.in

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 13 Justin Wood (:Callek) (Away until Aug 29) 2011-04-16 11:31:06 PDT
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?
Comment 14 Serge Gautherie (:sgautherie) 2011-04-16 15:36:45 PDT
Created attachment 526528 [details] [diff] [review]
(Bv1a-CC) Remove --disable-installer option from configure.in, except on MOZILLA_5_0_BRANCH

Bv1-CC, with comment 13 suggestion(s).
Comment 15 Justin Wood (:Callek) (Away until Aug 29) 2011-04-20 19:17:25 PDT
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.
Comment 16 Justin Wood (:Callek) (Away until Aug 29) 2011-04-20 19:19:07 PDT
(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 17 Philipp Kewisch [:Fallen] 2011-09-07 06:57:54 PDT
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!
Comment 18 Serge Gautherie (:sgautherie) 2011-10-20 09:08:09 PDT
Created attachment 568413 [details] [diff] [review]
(Av1a2) Remove --disable-installer option from configure.in

Av1a, (context-)unbitrotted.
Comment 19 Serge Gautherie (:sgautherie) 2011-10-20 09:30:09 PDT
Created attachment 568419 [details] [diff] [review]
(Av1b) Remove --disable-installer option from configure.in
[Checked in: Comment 24]

Av1a2, with (2) restored default values.

NB: The option is removed, but not the variable.
Comment 20 Serge Gautherie (:sgautherie) 2011-10-20 09:41:08 PDT
Created attachment 568421 [details] [diff] [review]
(Bv1b-CC) Remove --disable-installer option from configure.in
[Checked in: Comment 25]

Bv1a-CC, with comment 15 suggestion(s),
and (context-)unbitrotted.
Comment 21 Ted Mielczarek [:ted.mielczarek] 2011-10-25 08:31:34 PDT
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.
Comment 22 Serge Gautherie (:sgautherie) 2012-03-21 23:44:15 PDT
Ted, do you confirm I can go ahead with this now?
Comment 23 Ted Mielczarek [:ted.mielczarek] 2012-03-22 05:52:16 PDT
Please do.
Comment 24 Serge Gautherie (:sgautherie) 2012-03-22 18:18:27 PDT
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 25 Serge Gautherie (:sgautherie) 2012-03-22 18:31:04 PDT
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
Comment 26 Serge Gautherie (:sgautherie) 2012-03-22 18:41:09 PDT
Created attachment 608559 [details] [diff] [review]
(Cv1) Remove MOZ_INSTALLER support
[Checked in: Comment 30]

Includes fix for bug 697147.
Comment 27 Serge Gautherie (:sgautherie) 2012-03-22 18:52:29 PDT
Created attachment 608560 [details] [diff] [review]
(Dv1-CC) Remove MOZ_INSTALLER support
[Checked in: Comment 36]

Also fixes a nit in /suite file.
Comment 28 Jacek Caban 2012-03-23 07:57:26 PDT
Created attachment 608710 [details] [diff] [review]
cross compiling fix v1.0

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 29 Ted Mielczarek [:ted.mielczarek] 2012-03-23 09:07:03 PDT
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 30 Serge Gautherie (:sgautherie) 2012-03-23 17:30:53 PDT
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 31 Serge Gautherie (:sgautherie) 2012-03-23 17:49:47 PDT
Comment on attachment 608710 [details] [diff] [review]
cross compiling fix v1.0

You should attach a "diff -w" too to ease review.
Comment 32 Serge Gautherie (:sgautherie) 2012-03-23 17:57:53 PDT
(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).
Comment 33 Serge Gautherie (:sgautherie) 2012-03-23 18:15:22 PDT
(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
Comment 34 Serge Gautherie (:sgautherie) 2012-03-23 18:20:47 PDT
(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 35 Justin Wood (:Callek) (Away until Aug 29) 2012-03-24 15:04:16 PDT
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 36 Serge Gautherie (:sgautherie) 2012-03-24 20:14:10 PDT
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 37 Serge Gautherie (:sgautherie) 2012-03-24 20:18:57 PDT
Comment on attachment 608710 [details] [diff] [review]
cross compiling fix v1.0

This has bitrotted: please file and move it to a blocking bug.
Comment 38 Serge Gautherie (:sgautherie) 2012-03-24 20:37:52 PDT
(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. 
}
Comment 39 Serge Gautherie (:sgautherie) 2012-03-25 02:12:21 PDT
V.Fixed, per MXR search.

Note You need to log in before you can comment on or make changes to this bug.