737303 - make package fails with the installer disabled

Status: VERIFIED DUPLICATE of bug 736739

(Firefox Build System :: General, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Due to missing ifdefs, disabling the installer now causes build failures due to |make package| failing on errors.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Attachment: patch

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: patch

Add a missing define

Comment 3

[Serge Gautherie (:sgautherie)]

This is a duplicate of bug 736739.
I don't think it's worth doing this: as well stop using that option now.

Yet, if you do want to fix this, you should update removed-files.in too.

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 607411 [details] [diff] [review]
patch

This misses maintenanceservice_installer.exe

Comment 5

[Rain [:rain1] (inactive)]

What's the use case for disabling the installer? https://mxr.mozilla.org/mozilla-central/source/configure.in#6297 looks dangerous at first glance. I think we should figure out how to get rid of the option entirely...

Comment 6

[Ryan VanderMeulen [:RyanVM]]

(In reply to Serge Gautherie (:sgautherie) from comment #3)
> This is a duplicate of bug 736739.
> I don't think it's worth doing this: as well stop using that option now.

Building on Windows is a tediously long process as it is. Why should people making their own builds be forced to make it even longer by building a useless installer?

> Yet, if you do want to fix this, you should update removed-files.in too.

If the installer isn't being built, what purpose does removed-files.in serve?

(In reply to Serge Gautherie (:sgautherie) from comment #4)
> Comment on attachment 607411 [details] [diff] [review]
> patch
> 
> This misses maintenanceservice_installer.exe

Maintenance service can be disabled separately. I'm OK making it depend on this patch too.

(In reply to Siddharth Agarwal [:sid0] from comment #5)
> What's the use case for disabling the installer?
> https://mxr.mozilla.org/mozilla-central/source/configure.in#6297 looks
> dangerous at first glance. I think we should figure out how to get rid of
> the option entirely...

See above.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to Ryan VanderMeulen from comment #6)

> Building on Windows is a tediously long process as it is. Why should people
> making their own builds be forced to make it even longer by building a
> useless installer?

I am(/was!) in that case too, but I'm not the one who made the decision.

> If the installer isn't being built, what purpose does removed-files.in serve?

Good point ;-<
(Maybe just add a reminder there for this specific case, fwiw.)

> Maintenance service can be disabled separately. I'm OK making it depend on
> this patch too.

Thanks.


(In reply to Siddharth Agarwal [:sid0] from comment #5)
> https://mxr.mozilla.org/mozilla-central/source/configure.in#6297 looks
> dangerous at first glance.

What danger do you see?

Comment 8

[Rain [:rain1] (inactive)]

(In reply to Serge Gautherie (:sgautherie) from comment #7)
> What danger do you see?

Oh, I misread the code. Never mind, sorry.

(In reply to Ryan VanderMeulen from comment #6)
> (In reply to Serge Gautherie (:sgautherie) from comment #3)
> > This is a duplicate of bug 736739.
> > I don't think it's worth doing this: as well stop using that option now.
> 
> Building on Windows is a tediously long process as it is. Why should people
> making their own builds be forced to make it even longer by building a
> useless installer?

If you're talking about helper.exe, it isn't useless. It's used at mxr.mozilla.org/mozilla-central/source/browser/components/shell/src/nsWindowsShellService.cpp#259 and in a few other places.

Comment 9

[Rain [:rain1] (inactive)]

The link should be http://mxr.mozilla.org/mozilla-central/source/browser/components/shell/src/nsWindowsShellService.cpp#259.

Comment 10

[(not currently active) Ted Mielczarek]

More configure options mean more ways for things to get broken. We should just remove --disable-installer.

Comment 11

[Serge Gautherie (:sgautherie)]

V.Duplicate

Comment 12

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ted Mielczarek [:ted] from comment #10)
> More configure options mean more ways for things to get broken. We should
> just remove --disable-installer.

Not trying to be smart here, but what's the point of allowing *anything* to be disabled in configure if that's the mindset? Why not remove the ability to disable parental controls, safe browsing, maintenance service, updater, etc?

Presumably, if someone is doing their own build with these things disabled, they know what they're doing. It's not like allowing these is a huge maintenance burden.

Comment 13

[Phil Ringnalda (:philor)]

It's just one of our standard patterns:

1. "I want to make changes to foo which require a version of Bar after n.nn, but not everyone has that yet, so I'll make a configure option to let them disable building it."
2. Affected people disable it.
3. Other people disable it.
4. "Why the hell are you people disabling foo? We should remove that option."

Comment 14

[(not currently active) Ted Mielczarek]

(In reply to Ryan VanderMeulen from comment #12)
> Not trying to be smart here, but what's the point of allowing *anything* to
> be disabled in configure if that's the mindset? Why not remove the ability
> to disable parental controls, safe browsing, maintenance service, updater,
> etc?
> 
> Presumably, if someone is doing their own build with these things disabled,
> they know what they're doing. It's not like allowing these is a huge
> maintenance burden.

This is patently false, and I could show you hundreds of mozilla.dev.builds threads where people have blindly copied mozconfig options they found somewhere (including from configure --help) for no apparent reason. The existence of this bug implies a non-zero maintenance burden. philor nailed one reason, but historically we've also accrued a lot of these for no particular reason, except that someone thought they needed a configure option. I've been fighting against adding new configure options, since they inevitably lead to random people using them and breaking themselves, but it's hard to keep them from creeping in. Getting rid of unnecessary ones is good.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ted Mielczarek [:ted] from comment #14)
> This is patently false, and I could show you hundreds of mozilla.dev.builds
> threads where people have blindly copied mozconfig options they found
> somewhere (including from configure --help) for no apparent reason.

No doubt that happens, but I've already given a reason for why this particular option is useful. Windows builds take forever and the installer is of no use to me (or the vast majority of self-builders I'd wager). Why should I have to lengthen my build time for a piece of non-core functionality that I will not use?

> The existence of this bug implies a non-zero maintenance burden.

Nobody's asking Mozilla to make a patch for this. Nobody's asking for testing support or checking for breakage in future patches. All I'm asking is for is a review and permission to land a trivial patch to make it work again.

> I've been fighting against adding new configure options, since they
> inevitably lead to random people using them and breaking themselves, but
> it's hard to keep them from creeping in. Getting rid of unnecessary ones is
> good.

As said above, I agree with the general principle you're operating under here. But in this case, I don't agree that this particular option is a bad one to have. While it's of no use to Mozilla for official builds, it is very useful to self-builders.

Comment 16

[Rain [:rain1] (inactive)]

(In reply to Ryan VanderMeulen from comment #15)
> 
> No doubt that happens, but I've already given a reason for why this
> particular option is useful. Windows builds take forever and the installer
> is of no use to me (or the vast majority of self-builders I'd wager). Why
> should I have to lengthen my build time for a piece of non-core
> functionality that I will not use?

I've already pointed out one place where the helper.exe is used. We use it for a number of things, some of which potentially apply to dev builds too: shortcut maintenance, file associations...

> As said above, I agree with the general principle you're operating under
> here. But in this case, I don't agree that this particular option is a bad
> one to have. While it's of no use to Mozilla for official builds, it is very
> useful to self-builders.

I don't think shortening the build time by a few seconds is a good enough reason to keep it around. Configure options have an exponential complexity after all.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

(In reply to Siddharth Agarwal [:sid0] from comment #16)
> I've already pointed out one place where the helper.exe is used. We use it
> for a number of things, some of which potentially apply to dev builds too:
> shortcut maintenance, file associations...
> 

Why does that functionality need to be tied to the installer?

Comment 18

[Rain [:rain1] (inactive)]

(In reply to Ryan VanderMeulen from comment #17)
> Why does that functionality need to be tied to the installer?

Maybe it doesn't (the advantage is that the same code that does things during install does them during upgrade/association), but that's how it is right now.