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

VERIFIED FIXED in mozilla14

Status

()

Core
Build Config
--
minor
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

Trunk
mozilla14
x86
Windows Server 2003
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(status2.0 wontfix)

Details

(URL)

Attachments

(4 attachments, 7 obsolete attachments)

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
(Assignee)

Description

7 years ago
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).
}
(Assignee)

Comment 1

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
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.
(Assignee)

Comment 4

7 years ago
(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.
(Assignee)

Comment 6

7 years ago
Created attachment 483173 [details] [diff] [review]
(Av1) Remove --disable-installer option from configure.in

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+
(Assignee)

Comment 9

7 years ago
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?
(Assignee)

Updated

7 years ago
Blocks: 462361
Flags: in-testsuite-
Attachment #483173 - Flags: approval2.0? → approval2.0-
(Assignee)

Updated

6 years ago
No longer blocks: 462361
status2.0: --- → wontfix
(Assignee)

Comment 10

6 years ago
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 :-|
Attachment #526230 - Flags: review?(kairo)
(Assignee)

Comment 11

6 years ago
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.
Attachment #526232 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #526232 - Flags: review? → review?(philipp)
(Assignee)

Updated

6 years ago
Depends on: 650322
(Assignee)

Comment 12

6 years ago
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
Attachment #483173 - Attachment is obsolete: true
Attachment #526499 - Flags: review+
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-
(Assignee)

Comment 14

6 years ago
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).
Attachment #526230 - Attachment is obsolete: true
Attachment #526528 - Flags: review?(bugspam.Callek)
(Assignee)

Updated

6 years ago
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.
(Assignee)

Updated

6 years ago
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+
(Assignee)

Updated

6 years ago
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
(Assignee)

Comment 18

6 years ago
Created attachment 568413 [details] [diff] [review]
(Av1a2) Remove --disable-installer option from configure.in

Av1a, (context-)unbitrotted.
Attachment #526499 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #526499 - Flags: review+
(Assignee)

Comment 19

6 years ago
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.
Attachment #568413 - Attachment is obsolete: true
Attachment #568419 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 20

6 years ago
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.
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+
(Assignee)

Updated

6 years ago
Depends on: 697147

Updated

6 years ago
Attachment #568421 - Flags: review?(bugspam.Callek) → review+

Updated

5 years ago
Whiteboard: [blocked by bug 650322 :-<]
(Assignee)

Updated

5 years ago
Whiteboard: [checkin blocked by bug 650322]
Target Milestone: --- → Future
(Assignee)

Updated

5 years ago
Depends on: 736739
(Assignee)

Comment 22

5 years ago
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 → ---
Please do.
(Assignee)

Comment 24

5 years ago
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]
(Assignee)

Comment 25

5 years ago
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]
(Assignee)

Comment 26

5 years ago
Created attachment 608559 [details] [diff] [review]
(Cv1) Remove MOZ_INSTALLER support
[Checked in: Comment 30]

Includes fix for bug 697147.
Attachment #608559 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 27

5 years ago
Created attachment 608560 [details] [diff] [review]
(Dv1-CC) Remove MOZ_INSTALLER support
[Checked in: Comment 36]

Also fixes a nit in /suite file.
Attachment #608560 - Flags: review?(bugspam.Callek)

Comment 28

5 years ago
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
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+
(Assignee)

Updated

5 years ago
Attachment #608560 - Flags: review?(mbanner)
(Assignee)

Comment 30

5 years ago
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]
(Assignee)

Comment 31

5 years ago
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)
(Assignee)

Comment 32

5 years ago
(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).
(Assignee)

Comment 33

5 years ago
(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
(Assignee)

Comment 34

5 years ago
(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+
(Assignee)

Comment 36

5 years ago
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]
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
(Assignee)

Comment 37

5 years ago
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
(Assignee)

Comment 38

5 years ago
(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. 
}
(Assignee)

Comment 39

5 years ago
V.Fixed, per MXR search.
Status: RESOLVED → VERIFIED

Updated

5 years ago
Depends on: 739188
(Assignee)

Updated

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