297811 - Disable updater if iconv not found

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Darin Fisher]

Disable updater if iconv not found

This only applies to Windows.

Comment 1

[Darin Fisher]

Attachment: v1 patch

Comment 2

[Benjamin Smedberg]

I don't think this is a good idea. I think we should AC_MSG_ERROR and halt
configure, rather than making the build options depend on the particular machine.

Comment 3

[Darin Fisher]

OK, I was just following the example of --enable-extensions.  We AC_MSG_WARN
(and keep going) when we cannot build xmlterm, gnomevfs, venkman,
tridentprofile, and xforms due to missing prereqs.  Should those be changed too?
 Or, are those different somehow?

Comment 4

[Benjamin Smedberg]

Some are different, and some are wrong: tridentprofile, for example, could never
work on non-windows, so it makes sense to warn+disable, as it makes sense to
warn+disable gnomevfs on windows. But we should error out if gnomevfs doesn't
have the proper prerequistes on unix, we should error.

In any case, I don't want to end up in the situation where a misconfigured
tinderbox machine or build machine could succeed "accidentally" without building
the update system.

Comment 5

[Darin Fisher]

OK, that makes sense.

Comment 6

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

note that some of those build options warn+continue if enabled implicitly, but
halt if enabled explicitly (gnomevfs, for example).

Why should we show an error if gnomevfs dependencies are not found? That's not
how most other configure scripts are working.

Comment 7

[Benjamin Smedberg]

We are different from most configure-based products, in that we do not expect
people to configure themself and build from source, with whatever configuration
they happen to have: our build system is geared to producing end-user binaries,
and the host system that is doing the build should not in general affect build
options.

Comment 8

[cls]

Comment on attachment 186361 [details] [diff] [review]
v1 patch

I don't think win32 is the only platform that will not have iconv installed by
default.

Regarding hard & soft dependencies,  I believe that the only reason we starting
using the warn+disable method for certain items is because we didn't want to
have to keep track of a separate EXTENSIONS_ALL list for each platform. 
Otherwise, we have hard dependencies for practically everything else.  

Unless it's practically impossible to build a feature for a particular platform
(e.g., xmlterm for win32), we shouldn't let the installed packages on the
machine determine the feature set of the application.  

FWIW, if there was a AC_MSG_INFO, I would use that instead of warning about the
missing gnomevfs dependency when building on win32 since it's just an
informational message to inform people that not everything in the default or
all extension list is being built.  We could silently disable those but I think
that would lead to an equal or greater number of complaints.

Comment 9

[Darin Fisher]

> I don't think win32 is the only platform that will not have iconv installed by
> default.

The updater only needs iconv under win32, so I only require iconv under win32. 
Is this somehow not the right thing to do?

Comment 10

[Darin Fisher]

Attachment: v1.1 patch

Changed AC_MSG_WARN to AC_MSG_ERROR.

Comment 11

[cls]

Comment on attachment 186419 [details] [diff] [review]
v1.1 patch

Sorry, I thought iconv was being called universally.

Comment 12

[Darin Fisher]

fixed-on-trunk