516758 - Remove more options from configure

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mitchell Field [:Mitch]]

This is a followup to bug 513924.

Comment 1

[Mitchell Field [:Mitch]]

Open to suggestions.

Comment 2

[(not currently active) Ted Mielczarek]

Note bug 517557 is removing --disable-canvas.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

how about:

5288 MOZ_ARG_ENABLE_BOOL(native-uconv,
5305 MOZ_ARG_ENABLE_BOOL(plaintext-editor-only,
6841 MOZ_ARG_ENABLE_BOOL(long-long-warning,
7142 MOZ_ARG_ENABLE_BOOL(static,
4951 MOZ_ARG_DISABLE_BOOL(pango,
5219 MOZ_ARG_DISABLE_BOOL(accessibility,

possibly:
5932 MOZ_ARG_ENABLE_BOOL(leaky,
6656 MOZ_ARG_ENABLE_BOOL(insure,

Comment 4

[(not currently active) Ted Mielczarek]

A good set of suggestions, just some comments:

5288 MOZ_ARG_ENABLE_BOOL(native-uconv,

smontagu would need to comment on this, I think

7142 MOZ_ARG_ENABLE_BOOL(static,

Thunderbird is still using this, we do error if you try to use it with Firefox, at least.

5219 MOZ_ARG_DISABLE_BOOL(accessibility,

I know we're not shipping with a11y enabled on Mac yet, so this might still be useful. In addition, I think removing this would make it difficult to build on mingw.

I don't think removing any of those other options would cause any problems.

Comment 5

[(not currently active) Ted Mielczarek]

From my bug 513924 comment 17, you could ditch MOZ_JSLOADER and MOZ_VIEW_SOURCE throughout the codebase here or in a separate bug.

Things from bz's bug 513924 comment 4 that didn't get fixed there:
  --enable-places        Enable 'places' bookmark/history implementation

Not sure we need this option anymore. Dietrich?

--enable-help-viewer        Enable help viewer

bsmedberg suggested that TB/SM might be using this, but I'm not sure. I guess you could kill the option and leave it for confvars.sh if they need it. dmose?

--disable-freetypetest
                          Do not try to compile and run a test FreeType program

This comes wholesale from http://mxr.mozilla.org/mozilla-central/source/build/autoconf/freetype2.m4. Maybe we can remove that whole file? roc, do we still need freetype2 for anything? (Aside from the one in the tree that WinCE builds are using.) The only place I see it being used is if you --disable-pango, which we can probably remove anyway.

--disable-libIDLtest    Do not try to compile and run a test libIDL program
--disable-glibtest      Do not try to compile and run a test GLIB program

These unfortunately come from http://mxr.mozilla.org/mozilla-central/source/build/autoconf/libIDL.m4 and http://mxr.mozilla.org/mozilla-central/source/build/autoconf/glib.m4 respectively, and I don't think we can ditch those currently. Maybe if someone finishes rewriting xpidl in Python we can ditch the IDL one.

--disable-postscript    Disable PostScript printing support 

This I don't know, would need to ask someone with printing knowledge (good luck).

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> --enable-help-viewer        Enable help viewer
> 
> bsmedberg suggested that TB/SM might be using this, but I'm not sure. I guess
> you could kill the option and leave it for confvars.sh if they need it. dmose?

SeaMonkey still uses it (although I guess at some stage that code should be moved). Thunderbird doesn't.

Comment 7

[[e-mail]]

(In reply to comment #5)
> Things from bz's bug 513924 comment 4 that didn't get fixed there:
>   --enable-places        Enable 'places' bookmark/history implementation
> Not sure we need this option anymore.
Thunderbird doesn't quite want places yet... they may do soon though.

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to comment #6)
> (In reply to comment #5)
> > --enable-help-viewer        Enable help viewer
> SeaMonkey still uses it (although I guess at some stage that code should be
> moved). Thunderbird doesn't.

(In reply to comment #7)
> (In reply to comment #5)
> > Things from bz's bug 513924 comment 4 that didn't get fixed there:
> >   --enable-places        Enable 'places' bookmark/history implementation
> Thunderbird doesn't quite want places yet... they may do soon though.

In light of these, we should be able to remove the configure options, but leave the configure variables, so TB/SM can continue to disable them in confvars.sh. Nobody should need to disable them from the commandline.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> In light of these, we should be able to remove the configure options, but leave
> the configure variables, so TB/SM can continue to disable them in confvars.sh.
> Nobody should need to disable them from the commandline.

Agreed, I'm happy with that.

Comment 10

[Phil Ringnalda (:philor)]

Yeah, I did a test build of Tb with Places the other day, and never for a second thought that there might still be a configure option, so I just hacked my confvars :)

Comment 11

[Simon Montagu :smontagu]

(In reply to comment #4)
> 5288 MOZ_ARG_ENABLE_BOOL(native-uconv,
> 
> smontagu would need to comment on this, I think
 
I would if I knew exactly what is being proposed here ;-) I believe there are Linux distros that enable native uconv in their Firefox builds.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #5)
> --disable-libIDLtest    Do not try to compile and run a test libIDL program
> --disable-glibtest      Do not try to compile and run a test GLIB program
> 
> These unfortunately come from
> http://mxr.mozilla.org/mozilla-central/source/build/autoconf/libIDL.m4 and
> http://mxr.mozilla.org/mozilla-central/source/build/autoconf/glib.m4
> respectively, and I don't think we can ditch those currently. Maybe if someone
> finishes rewriting xpidl in Python we can ditch the IDL one.

Those two flags are used for the old libIDL. For the new libIDL (libIDL 2) we use pkgconfig:
7333         PKG_CHECK_MODULES(LIBIDL, libIDL-2.0 >= 0.8.0 glib-2.0 gobject-2.0, _LIBIDL_FOUND=1,_LIBIDL     _FOUND=)

So we could probably ditch libIDL.m4/glib.m4

Comment 13

[Mitchell Field [:Mitch]]

Attachment: Patch (v1)

This patch removes the following configure options (leaving corresponding configure variables):
--enable-help-viewer
--enable-places.

Insure++ instrumentation support, and configure options, are completely removed:
--enable-insure
--with-insure-dirs
--with-insure-exclude-dirs.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #4)
> 5219 MOZ_ARG_DISABLE_BOOL(accessibility,
> 
> I know we're not shipping with a11y enabled on Mac yet, so this might still be
> useful. In addition, I think removing this would make it difficult to build on
> mingw.

There's also https://developer.mozilla.org/en/atlbase.h

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 434612 [details] [diff] [review]
Patch (v1)

Good stuff! You should also be able to remove the MOZ_PLACES=1 from browser/confvars.sh:
http://mxr.mozilla.org/mozilla/source/browser/confvars.sh#44

Comment 16

[Mitchell Field [:Mitch]]

Attachment: Patch (v2)

(In reply to comment #15)
> Good stuff! You should also be able to remove the MOZ_PLACES=1 from
> browser/confvars.sh:
> http://mxr.mozilla.org/mozilla/source/browser/confvars.sh#44
Done.

Comment 17

[Ben Bucksch (:BenB)]

[Places]
> we should be able to remove the configure options, but leave
> the configure variables, so TB/SM can continue to disable them in confvars.sh.

What about xulrunner? Other apps are likely not to want places either, for the same reasons. You shouldn't force xulrunner devs to know undocumented env vars and set them via some obscure or custom script. That's what configure is for.

If you want to force it "on" for Firefox, then do something like
#if app=browser and not defined PLACES
#error Places is needed for Firefox

Comment 18

[Ben Bucksch (:BenB)]

(I'm speaking as xulrunner dev here.)

Comment 19

[(not currently active) Ted Mielczarek]

The codesize is not significant enough to matter, so allowing developers to disable it for custom xulrunner builds is of negligible gain.

Having it in configure means an extra codepath that we may or may not be supporting, meaning that the configure option might stop working due to disuse anyway. I'd prefer to have it not present and pretend we actually support it.

We're not obligated to provide options every single feature in the platform. The fewer options we have, the smaller our support matrix, and the more likely everything is to work consistently.

Comment 20

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/f95fc26312b8

Comment 21

[(not currently active) Ted Mielczarek]

Bustage fix followup:
http://hg.mozilla.org/mozilla-central/rev/aea2b08ae4a3

Comment 22

[Edmund Wong (:ewong)]

Under VS2010 when building Seamonkey, one needs to add the 
'--disable-activex' option, otherwise bug 559537 pops up.  

In IRC, I was told that the nightlies and the official releases don't have
activeX enabled (i.e., I believe option --disable-activex is added to the
configuration).   If that's the case, why not simply have the configure
script default to disable activeX and have an '--enable-activeX' for those
who require it for whatever reasons?

Comment 23

[(not currently active) Ted Mielczarek]

That's incorrect. Our official builds build ActiveX by default. In any case, we haven't removed that option.

Comment 24

[Jens Hatlak (:InvisibleSmiley)]

AFAICT this broke SeaMonkey's Help window (bug 561663), cf. #ifdef MOZ_HELP_VIEWER in mozilla/toolkit/locales/jar.mn. SeaMonkey defines MOZ_HELP_VIEWER in suite/confvars.sh and comm-central's configure.in still contains the related code that was removed from mozilla-central's version but that doesn't seem to suffice. Unfortunately I know too little about the setup to tell what's needed to fix it but I'm sure one of you does. ;-)

Comment 25

[Robert Kaiser]

Yup, removing the help viewer stuff in this patch was completely the wrong thing to do given that this code exists, is off by default and needs some way to actually be turned on for those that need it (SeaMonkey happens to be the consumer we have inside Mozilla).

Please revert that part. And note that it needs to happen within a week as this problem blocks the SeaMonkey 2.1 Alpha that should be cut early next week (code freeze is Tuesday 23:59 PDT).

Comment 26

[Justin Wood (:Callek)]

Attachment: Fix for SeaMonkey bustage

Comment 27

[(not currently active) Ted Mielczarek]

Comment on attachment 441686 [details] [diff] [review]
Fix for SeaMonkey bustage

I don't really want this in the top-level configure. Can't you fix this in SeaMonkey's build system instead?

Comment 28

[(not currently active) Ted Mielczarek]

Comment on attachment 441686 [details] [diff] [review]
Fix for SeaMonkey bustage

Oh, I see, this code still lives in toolkit. If SeaMonkey is the only consumer, then this really should move to comm-central/suite. r=me on the condition that you file a bug on moving that directory to suite, and removing it from toolkit (along with this configure block).

Comment 29

[Philip Chee]

Ted, Gavin previously agreed to keep the code in toolkit as there are external consumers as well.

Comment 30

[(not currently active) Ted Mielczarek]

Gavin notes this is already filed as bug 425541, so carry on.

Comment 31

[Justin Wood (:Callek)]

http://hg.mozilla.org/mozilla-central/rev/cedd9976e7e0

Comment 32

[Mitchell Field [:Mitch]]

Attachment: Remove long-long-warning

We currently warn about using "long long" types, but I'm told that's inane. This patch adds -Wno-long-long to CFLAGS/CXXFLAGS where -pedantic is used, and removes the long-long-warning option.

Comment 33

[Zack Weinberg (:zwol)]

Comment on attachment 445745 [details] [diff] [review]
Remove long-long-warning

I approve of the concept, but I am not qualified to review patches to configure.in.  Over to ted.

Comment 34

[Zack Weinberg (:zwol)]

From the C++ end of things, the argument for unconditionally disabling this warning goes like so:  The only purpose of the warning is to notify the programmer that this type is nonstandard and might not be supported by every compiler out there.  Indeed, it is not in C++98.  However, it is in C99, and will be in C++1x; it has been supported by G++ since the mid-nineties if not longer; and as best I can tell from MSDN, it has been supported by VC++ since earlier than VS2005.  Empirically, we have tons of unqualified uses of 'long long' in our source tree, so if it were going to be a problem it would have been a problem already.

Comment 35

[Zack Weinberg (:zwol)]

On the gripping hand, we appear to be using 'long long' to mean 'int64_t' and maybe we should start saying what we mean more consistently.

Comment 36

[(not currently active) Ted Mielczarek]

Comment on attachment 445745 [details] [diff] [review]
Remove long-long-warning

># HG changeset patch
># Parent 763e567872bba47eb6e0e78e5c747e34e4b6e88e
># User Mitchell Field <[e-mail]>
>Bug 516758 - Remove --disable-long-long-warning.
>
>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -1489,22 +1489,18 @@ if test "$GNU_CC"; then
>            ;;
>        esac
>     fi
> 
>     dnl Turn pedantic on but disable the warnings for long long
>     _PEDANTIC=1

Fix this comment, please.

If Zack is ok with it, I'm ok with it. r=me with that fix.

Comment 37

[(not currently active) Ted Mielczarek]

Comment on attachment 445745 [details] [diff] [review]
Remove long-long-warning

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/d5d5ed6d3e1c

Comment 38

[Takanori MATSUURA]

Attachment: Sync for comm-central

Merged patch of attachment 435945 [details] [diff] [review], attachment 441686 [details] [diff] [review], and attachment 445745 [details] [diff] [review].

Comment 39

[Justin Wood (:Callek)]

Comment on attachment 454457 [details] [diff] [review]
Sync for comm-central

>+dnl ========================================================
> dnl = Enable help viewer (off by default)
> dnl ========================================================

Please keep this option intact as far as c-c has it. It was only retained in _any_ form in m-c because c-c actually uses it. My reasoning -- We should allow SeaMonkey to build without this (--disable-*) though it has little use for Thunderbird.

We may wish to drop it later, but for now I'd rather keep it in.

Also for future, new bugs are better for c-c ports like this it helps keep clutter down in the original bug(s).

Comment 40

[Takanori MATSUURA]

(In reply to comment #39)

OK.
I file as bug 575509 for this bug in comm-central.

Comment 41

[Takanori MATSUURA]

BTW, why this bug still open?

Comment 42

[Justin Wood (:Callek)]

(In reply to comment #41)
> BTW, why this bug still open?

IMO it shouldn't be... resolving. Mitch lets use a fresh bug for any followup work. If you disagree reopen, but please lets identify _when_ this will be closed if you do.