Closed Bug 471451 Opened 11 years ago Closed 11 years ago

useless warnings about midl from configure when midl is missing

Categories

(Firefox Build System :: General, defect)

x86
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: timeless, Assigned: timeless)

References

Details

Attachments

(1 file)

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)
Attachment #354735 - Flags: review?(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.
Attachment #354735 - Flags: review?(ted.mielczarek) → review+
timeless: did you want to patch the copy-paste of the same thing in js/configure.in, too?
I doubt js/src needs midl, we could probably just remove all traces of it from their configure.
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 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.
http://hg.mozilla.org/mozilla-central/rev/978be7d51a25
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(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 ?
bug 471854 has another patch touching this code. It also does the removal from js/src/configure.in.
Yeah, my patch will resolve all points.
(In reply to comment #8)

Good, let's follow up there.
Blocks: 471854
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.