Open Bug 1309015 Opened 3 years ago Updated 2 years ago

Move MOZ_MAINTENANCE_SERVICE and MOZ_STUB_INSTALLER to Python configure

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: cmanchester, Assigned: cmanchester)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

browser/confvars.sh is pretty almost entirely standalone assignments with these moved over.
Assignee: nobody → cmanchester
Comment on attachment 8799611 [details]
Bug 1309015 - Move MOZ_MAINTENANCE_SERVICE to Python configure.

https://reviewboard.mozilla.org/r/84750/#review84364

It was still possible to disable it in the past. Whether this is something that is necessary or not is not something I can tell. But this is clearly a behavior change. As it is apparently updater code, I guess the following would be questions for rstrong:
- is disabling maintenance service on windows worth supporting?
- is there ever going to be maintenance service on non-windows?
- by extension of the above two, can't we just remove MOZ_MAINTENANCE_SERVICE tests and replaces them with platform tests? (or completely remove them for obviously windows-only stuff)
Attachment #8799611 - Flags: review?(mh+mozilla)
See comment 4.
Flags: needinfo?(robert.strong.bugs)
Comment on attachment 8799612 [details]
Bug 1309015 - Move MOZ_STUB_INSTALLER to Python configure.

https://reviewboard.mozilla.org/r/84752/#review84366

::: browser/moz.configure:24
(Diff revision 1)
> +    if all([target.os == 'WINNT',
> +            not have_64_bit,
> +            not debug,
> +            update_channel in ('nightly', 'aurora', 'beta',
> +                               'beta-dev', 'release', 'release-dev')]):
> +        return True

You could make that:

return (target.os == 'WINNT' and
        not have_64_bit and
        ...) or None
Attachment #8799612 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 8799611 [details]
> Bug 1309015 - Move MOZ_MAINTENANCE_SERVICE to Python configure.
> 
> https://reviewboard.mozilla.org/r/84750/#review84364
> 
> It was still possible to disable it in the past. Whether this is something
> that is necessary or not is not something I can tell. But this is clearly a
> behavior change. As it is apparently updater code, I guess the following
> would be questions for rstrong:
> - is disabling maintenance service on windows worth supporting?
For anyone creating distributions it is. iirc SeaMonkey doesn't use it.

> - is there ever going to be maintenance service on non-windows?
I have no plans to create something similar for other platforms but it isn't possible for me to know what the future holds.

> - by extension of the above two, can't we just remove
> MOZ_MAINTENANCE_SERVICE tests and replaces them with platform tests? (or
> completely remove them for obviously windows-only stuff)
Considering that other apps don't use it I don't think so.
Flags: needinfo?(robert.strong.bugs)
Comment on attachment 8799613 [details]
Bug 1309015 - Move MOZ_BUNDLED_FONTS to Python configure.

https://reviewboard.mozilla.org/r/84754/#review84368

::: browser/confvars.sh
(Diff revision 1)
>  MOZ_UPDATER=1
>  MOZ_PHOENIX=1
>  
> -if test "$OS_ARCH" = "WINNT" -o \
> -        "$OS_ARCH" = "Linux"; then
> -  MOZ_BUNDLED_FONTS=1

This is also in b2g/**/confvars.sh

::: browser/moz.configure:33
(Diff revision 1)
> +@depends(target)
> +def windows_or_linux(target):
> +    if target.os in ('WINNT', 'GNU'):
> +        return True

Come to think of it (reading back bug 1231701), I think this is the wrong test. Specifically, I think the intent is that all platforms except Android and Darwin should have it enabled by default.

::: browser/moz.configure:47
(Diff revision 1)
> +bundled_fonts = depends_if('--enable-bundled-fonts')(lambda _: True)
> +
> +set_define('MOZ_BUNDLED_FONTS', bundled_fonts)

Soon, it will be set_define('MOZ_BUNDLED_FONTS', True, when='--enable-bundled-fonts') :)
Attachment #8799613 - Flags: review?(mh+mozilla)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #7)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > Comment on attachment 8799611 [details]
> > Bug 1309015 - Move MOZ_MAINTENANCE_SERVICE to Python configure.
> > 
> > https://reviewboard.mozilla.org/r/84750/#review84364
> > 
> > It was still possible to disable it in the past. Whether this is something
> > that is necessary or not is not something I can tell. But this is clearly a
> > behavior change. As it is apparently updater code, I guess the following
> > would be questions for rstrong:
> > - is disabling maintenance service on windows worth supporting?
> For anyone creating distributions it is. iirc SeaMonkey doesn't use it.

What about Firefox? Is disabling maintenance service on Windows for Firefox worth supporting?
Flags: needinfo?(robert.strong.bugs)
I don't know of any reason to support disabling it on Firefox except for possibly distributions. For example, I don't know if Tor or any other Windows distributions disables building it.
Flags: needinfo?(robert.strong.bugs)
Come to think of it, aren't stub installer and maintenance service things that should be opt-in rather than opt-out, and opted-in from the mozconfigs?
Blocks: 1316724
No longer blocks: 1316724
(In reply to Mike Hommey [:glandium] from comment #8)
> Comment on attachment 8799613 [details]
> Bug 1309015 - Move MOZ_BUNDLED_FONTS to Python configure.
> 
> https://reviewboard.mozilla.org/r/84754/#review84368
> 
> ::: browser/confvars.sh
> (Diff revision 1)
> >  MOZ_UPDATER=1
> >  MOZ_PHOENIX=1
> >  
> > -if test "$OS_ARCH" = "WINNT" -o \
> > -        "$OS_ARCH" = "Linux"; then
> > -  MOZ_BUNDLED_FONTS=1
> 
> This is also in b2g/**/confvars.sh
> 
> ::: browser/moz.configure:33
> (Diff revision 1)
> > +@depends(target)
> > +def windows_or_linux(target):
> > +    if target.os in ('WINNT', 'GNU'):
> > +        return True
> 
> Come to think of it (reading back bug 1231701), I think this is the wrong
> test. Specifically, I think the intent is that all platforms except Android
> and Darwin should have it enabled by default.

I'm not finding the part of that bug that suggests that... and I see some code in the packager that has a related test for `#if defined(XP_WIN) || defined(XP_LINUX)`. Can you elaborate?

> 
> ::: browser/moz.configure:47
> (Diff revision 1)
> > +bundled_fonts = depends_if('--enable-bundled-fonts')(lambda _: True)
> > +
> > +set_define('MOZ_BUNDLED_FONTS', bundled_fonts)
> 
> Soon, it will be set_define('MOZ_BUNDLED_FONTS', True,
> when='--enable-bundled-fonts') :)
There is no reason for *BSD, Solaris, etc. to be treated any different from Linux.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.