471451 - useless warnings about midl from configure when midl is missing

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[timeless]

Attachment: don't do math on empty strings when midl is missing

midl isn't necessary most of the time, and our build system merrily decides to try to do math on empty strings because it decides the proper name of midl early.

fwiw, i'm not the only person to hit this problem (mook mentions ignoring this warning too)

Comment 1

[(not currently active) Ted Mielczarek]

Comment on attachment 354735 [details] [diff] [review]
don't do math on empty strings when midl is missing

I'm surprised that we don't error at all if we don't find MIDL and you have a11y enabled. That seems like a poor failure mode.

Comment 2

[Phil Ringnalda (:philor)]

timeless: did you want to patch the copy-paste of the same thing in js/configure.in, too?

Comment 3

[(not currently active) Ted Mielczarek]

I doubt js/src needs midl, we could probably just remove all traces of it from their configure.

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 354735 [details] [diff] [review]
don't do math on empty strings when midl is missing

>+                # This switches us back to the old behaviour. When we drop
>+                # support for Windows older than Win2k, we should remove
>+                # this.

Isn't it already the case (since 1.9.0) ?

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 354735 [details] [diff] [review]
don't do math on empty strings when midl is missing

Anyway,

>+        if test -n "$_MIDL_MAJOR_VERSION"; then
>+            if test \( "$_MIDL_MAJOR_VERSION" -gt "6" \) -o \( "$_MIDL_MAJOR_VERSION" = "6" -a "$_MIDL_MINOR_VERSION" -gt "0" \) -o \( "$_MIDL_MAJOR_VERSION" = "6" -a "$_MIDL_MINOR_VERSION" = "00" -a "$_MIDL_REV_VERSION" -gt "359" \); then

Why not merge these 2 |if| ?

>+        else
>+                MIDL_FLAGS="${MIDL_FLAGS}"
>+                AC_MSG_RESULT([none needed])            

The identation should be reduced.

Comment 6

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/978be7d51a25

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> http://hg.mozilla.org/mozilla-central/rev/978be7d51a25

The indentation was fixed :-)
But what about comment 2+3 and my two other questions ?

Comment 8

[(not currently active) Ted Mielczarek]

bug 471854 has another patch touching this code. It also does the removal from js/src/configure.in.

Comment 9

[Masatoshi Kimura [:emk]]

Yeah, my patch will resolve all points.

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)

Good, let's follow up there.