Closed Bug 604266 Opened 9 years ago Closed 8 years ago

Remove '--disable-installer' option and 'MOZ_INSTALLER' variable

Categories

(Firefox Build System :: General, defect, minor)

x86
Windows Server 2003
defect
Not set
minor

Tracking

(status2.0 wontfix)

VERIFIED FIXED
mozilla14
Tracking Status
status2.0 --- wontfix

People

(Reporter: sgautherie, Assigned: sgautherie)

References

()

Details

Attachments

(4 files, 7 obsolete files)

3.55 KB, patch
ted
: review+
Details | Diff | Splinter Review
3.64 KB, patch
Callek
: review+
Details | Diff | Splinter Review
5.70 KB, patch
ted
: review+
Details | Diff | Splinter Review
4.93 KB, patch
Callek
: review+
Details | Diff | Splinter Review
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Resolution: FIXED → WONTFIX
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
Status: RESOLVED → REOPENED
Component: Release Engineering → Build Config
Product: mozilla.org → Core
QA Contact: release → build-config
Resolution: WONTFIX → ---
Version: other → Trunk
(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.
Assignee: nobody → sgautherie.bz
Status: REOPENED → ASSIGNED
Attachment #483173 - Flags: review?(ted.mielczarek)
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?
Attachment #483173 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #483173 - Flags: approval2.0?
Blocks: 462361
Flags: in-testsuite-
Attachment #483173 - Flags: approval2.0? → approval2.0-
NB: Let's update error messages even on MOZILLA_2_0_BRANCH :-|
Attachment #526230 - Flags: review?(kairo)
Afaict, this option is already useless.
Attachment #526232 - Flags: review?
Attachment #526232 - Flags: review? → review?(philipp)
Depends on: 650322
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?
Attachment #526230 - Flags: review?(kairo) → review-
Bv1-CC, with comment 13 suggestion(s).
Attachment #526230 - Attachment is obsolete: true
Attachment #526528 - Flags: review?(bugspam.Callek)
Attachment #526230 - Flags: review-
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.
Attachment #526528 - Flags: review?(bugspam.Callek) → review-
(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.
Whiteboard: [blocked by bug 650322 :-<]
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!
Attachment #526232 - Flags: review?(philipp) → review+
Attachment #526232 - Attachment description: (Cv1-Calendar) Remove overridden 'MOZ_INSTALLER=' option from confvars.sh → (Cv1-Calendar) Remove overridden 'MOZ_INSTALLER=' option from confvars.sh [Pushed in bug 689370]
Attachment #526232 - Attachment is obsolete: true
Av1a, (context-)unbitrotted.
Attachment #526499 - Attachment is obsolete: true
Attachment #526499 - Flags: review+
Av1a2, with (2) restored default values.

NB: The option is removed, but not the variable.
Attachment #568413 - Attachment is obsolete: true
Attachment #568419 - Flags: review?(ted.mielczarek)
Bv1a-CC, with comment 15 suggestion(s),
and (context-)unbitrotted.
Attachment #526528 - Attachment is obsolete: true
Attachment #568421 - Flags: review?(bugspam.Callek)
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.
Attachment #568419 - Flags: review?(ted.mielczarek) → review+
Depends on: 697147
Attachment #568421 - Flags: review?(bugspam.Callek) → review+
Whiteboard: [blocked by bug 650322 :-<]
Whiteboard: [checkin blocked by bug 650322]
Target Milestone: --- → Future
Depends on: 736739
Ted, do you confirm I can go ahead with this now?
Summary: [Windows ?] MoCo Try 'make installer' fails with '--disable-installer' → Remove '--disable-installer' option and 'MOZ_INSTALLER' variable
Whiteboard: [checkin blocked by bug 650322]
Target Milestone: Future → ---
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.
Attachment #568419 - Attachment description: (Av1b) Remove --disable-installer option from configure.in → (Av1b) Remove --disable-installer option from configure.in [Checked in: Comment 24]
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
Attachment #568421 - Attachment description: (Bv1b-CC) Remove --disable-installer option from configure.in → (Bv1b-CC) Remove --disable-installer option from configure.in [Checked in: Comment 25]
Includes fix for bug 697147.
Attachment #608559 - Flags: review?(ted.mielczarek)
Also fixes a nit in /suite file.
Attachment #608560 - Flags: review?(bugspam.Callek)
Attached patch cross compiling fix v1.0 (obsolete) — Splinter Review
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
Attachment #608710 - Flags: feedback?(sgautherie.bz)
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?
Attachment #608559 - Flags: review?(ted.mielczarek) → review+
Attachment #608560 - Flags: review?(mbanner)
Comment on attachment 608559 [details] [diff] [review]
(Cv1) Remove MOZ_INSTALLER support
[Checked in: Comment 30]

https://hg.mozilla.org/mozilla-central/rev/df1f94b2bdee
Attachment #608559 - Attachment description: (Cv1) Remove MOZ_INSTALLER support → (Cv1) Remove MOZ_INSTALLER support [Checked in: Comment 30]
Comment on attachment 608710 [details] [diff] [review]
cross compiling fix v1.0

You should attach a "diff -w" too to ease review.
Attachment #608710 - Flags: feedback?(sgautherie.bz)
(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.
Attachment #608560 - Flags: review?(mbanner)
Attachment #608560 - Flags: review?(bugspam.Callek)
Attachment #608560 - Flags: review+
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
Attachment #608560 - Attachment description: (Dv1-CC) Remove MOZ_INSTALLER support → (Dv1-CC) Remove MOZ_INSTALLER support [Checked in: Comment 36]
Status: ASSIGNED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Comment on attachment 608710 [details] [diff] [review]
cross compiling fix v1.0

This has bitrotted: please file and move it to a blocking bug.
Attachment #608710 - Attachment is obsolete: true
(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.
Status: RESOLVED → VERIFIED
Depends on: 739188
Depends on: 741340
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.