394272 - Mingw build error in nsDownloadManager.cpp

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Martijn Wargers (dead)]

After updating my mingw build, I get this build error:
/cygdrive/c/mozilla/mozilla/build/cygwin-wrapper g++ -mno-cygwin -o nsDownloadMa
nager.o -c  -DMOZILLA_INTERNAL_API -DOSTYPE=\"WINNT5.1\" -DOSARCH=WINNT  -I../..
/../../dist/include/xpcom -I../../../../dist/include/string -I../../../../dist/i
nclude/rdf -I../../../../dist/include/uriloader -I../../../../dist/include/mimet
ype -I../../../../dist/include/necko -I../../../../dist/include/pref -I../../../
../dist/include/progressDlg -I../../../../dist/include/intl -I../../../../dist/i
nclude/windowwatcher -I../../../../dist/include/webbrowserpersist -I../../../../
dist/include/appshell -I../../../../dist/include/dom -I../../../../dist/include/
embed_base -I../../../../dist/include/alerts -I../../../../dist/include/storage
-I../../../../dist/include/xulapp -I../../../../dist/include   -I../../../../dis
t/include/downloads -I../../../../dist/include/nspr  -DMOZ_PNG_READ -DPNG_NO_MMX
_CODE -DMOZ_PNG_WRITE   -I../../../../dist/sdk/include       -fno-rtti -fno-exce
ptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsy
nth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -mms-b
itfields -pipe  -DDEBUG -D_DEBUG -DDEBUG_mw -DTRACING -g   -DMOZILLA_CLIENT -inc
lude ../../../../mozilla-config.h /cygdrive/c/mozilla/mozilla/toolkit/components
/downloads/src/nsDownloadManager.cpp
In file included from c:/mozilla/mozilla/toolkit/components/downloads/src/nsDown
loadManager.cpp:76:
c:/mozilla/mozilla/toolkit/components/downloads/src/nsDownloadScanner.h:12:19: m
soav.h: No such file or directory
In file included from c:/mozilla/mozilla/toolkit/components/downloads/src/nsDown
loadManager.cpp:76:
c:/mozilla/mozilla/toolkit/components/downloads/src/nsDownloadScanner.h:49: erro
r: `IOfficeAntiVirus' was not declared in this scope
c:/mozilla/mozilla/toolkit/components/downloads/src/nsDownloadScanner.h:49: erro
r: template argument 1 is invalid
c:/mozilla/mozilla/toolkit/components/downloads/src/nsDownloadScanner.h:49: erro
r: ISO C++ forbids declaration of `mAVScanner' with no type
make[6]: *** [nsDownloadManager.o] Error 1
make[6]: Leaving directory `/cygdrive/c/mozilla/mozilla/_firefox/toolkit/compone
nts/downloads/src'
make[5]: *** [libs] Error 2
make[5]: Leaving directory `/cygdrive/c/mozilla/mozilla/_firefox/toolkit/compone
nts/downloads'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/cygdrive/c/mozilla/mozilla/_firefox/toolkit/compone
nts'
make[3]: *** [libs_tier_toolkit] Error 2
make[3]: Leaving directory `/cygdrive/c/mozilla/mozilla/_firefox'
make[2]: *** [tier_toolkit] Error 2
make[2]: Leaving directory `/cygdrive/c/mozilla/mozilla/_firefox'
make[1]: *** [alldep] Error 2
make[1]: Leaving directory `/cygdrive/c/mozilla/mozilla/_firefox'
make: *** [alldep] Error 2

C:\mozilla\mozilla>

Comment 1

[Rob Arnold [:robarnold]]

It looks like it cannot find the header msoav.h which defines the IOfficeAntiVirus interface. This header should be included on any reasonably recent Windows SDK. Can you check which sdk you're using and if the header is in its include directory?

Comment 2

[Martijn Wargers (dead)]

Well, moving the msoav.h header file from the Microsoft Platform SDK into the mingw include dir fixes that build problem, but then I get this build error:

cmps.dll  nsToolkitCompsModule.o   ./module.res          -Wl,--whole-archive ../
startup/src/libappstartup_s.a  ../downloads/src/libdownload_s.a ../alerts/src/li
balerts_s.a ../url-classifier/src/liburlclassifier_s.a ../feeds/src/libfeed_s.a
../typeaheadfind/src/libfastfind_s.a -Wl,--no-whole-archive -L../../../modules/z
lib/src -lmozz -L../../../dist/bin -L../../../dist/lib -lgkgfx ../../../dist/lib
/libunicharutil_s.a -L../../../dist/lib -lxpcom -lxpcom_core -L../../../dist/bin
 -L../../../dist/lib -lnspr4 -lplc4 -lplds4 -L../../../dist/lib -ljs3250   -lm
-lgdi32 -lwinmm -lwsock32 -lshell32 -lole32
../downloads/src/libdownload_s.a(nsDownloadScanner.o): In function `ZN17nsDownlo
adScanner9FindCLSIDEv':c:/mozilla/mozilla/toolkit/components/downloads/src/nsDow
nloadScanner.cpp:113: undefined reference to `IID_ICatInformation'
:c:/mozilla/mozilla/toolkit/components/downloads/src/nsDownloadScanner.cpp:113:
undefined reference to `CLSID_StdComponentCategoriesMgr'
collect2: ld returned 1 exit status
make[5]: *** [tkitcmps.dll] Error 1
make[5]: Leaving directory `/cygdrive/c/mozilla/mozilla/_firefox/toolkit/compone
nts/build'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/cygdrive/c/mozilla/mozilla/_firefox/toolkit/compone
nts'
make[3]: *** [libs_tier_toolkit] Error 2
make[3]: Leaving directory `/cygdrive/c/mozilla/mozilla/_firefox'
make[2]: *** [tier_toolkit] Error 2
make[2]: Leaving directory `/cygdrive/c/mozilla/mozilla/_firefox'
make[1]: *** [alldep] Error 2
make[1]: Leaving directory `/cygdrive/c/mozilla/mozilla/_firefox'
make: *** [alldep] Error 2

But Mozilla needs to be built with Mingw without depending on Microsoft Platform SDK header files anyway, so I guess this code has to be ifdef-ed out for mingw users.

It's not like one can copy the msoav.h header file into the mingw package by default, right?

Comment 3

[Shawn Wilsher :sdwilsh]

I'm very much inclined to not add more ifdefs to the download manager code...

Comment 4

[Martijn Wargers (dead)]

Attachment: patch

So this should probably be disabled for mingw users.
This makes that possible again.

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> So this should probably be disabled for mingw users.
why should it probably be disabled for mingw users?  It seems like it's a mingw bug for not supplying a header file that is part of the normal MSSDK.

Comment 6

[Martijn Wargers (dead)]

(In reply to comment #5)
> why should it probably be disabled for mingw users?  It seems like it's a mingw
> bug for not supplying a header file that is part of the normal MSSDK.

You mean that it is allowed for mingw to include the header which is normally part of the MSSDK?

Comment 7

[Hans-Andreas Engel]

> You mean that it is allowed for mingw to include the header which is normally
> part of the MSSDK?

You cannot do this directly -- however, you can use the MSDN documentation to write a header file containing the required declarations.  Ideally, this header file is then submitted to the w32api part of mingw.

The header file is not enough (see Comment 2); for linking you also need the corresponding library files that are also part of w32api.  In your case it seems that you need to extend these a bit to include CLSID_StdComponentCategoriesMgr.

Comment 8

[Shawn Wilsher :sdwilsh]

Alright, I'm OK with taking this fix temporarily, but I want a bug filed upstream with mingw to fix this, and once that lands in mingw (as per comment 7), I'd like to back this out.

Rob still needs to verify that this patch will work out OK.

Comment 9

[Martijn Wargers (dead)]

Ok, thanks.

Well, there is some documentation here:
http://msdn2.microsoft.com/en-us/library/ms537369.aspx
Looking at msoav.h, it may not seem that difficult to write an IOfficeAntiVirus definition based on the info on msdn, but I'm afraid I'm not capable of that.

Comment 10

[Rob Arnold [:robarnold]]

Comment on attachment 278966 [details] [diff] [review]
patch

>+ifndef GNU_CXX
> ifeq ($(OS_ARCH),WINNT)
> CPPSRCS += nsDownloadScanner.cpp
> endif
>+endif

This looks good.

>+#ifndef __MINGW32__
> #ifdef XP_WIN
>   mScanner = new nsDownloadScanner();
>   if (!mScanner)
>     return NS_ERROR_OUT_OF_MEMORY;
>   rv = mScanner->Init();
>   if (NS_FAILED(rv)) {
>     delete mScanner;
>     mScanner = nsnull;
>   }
> #endif
>+#endif

You still need to initialize mScanner here, otherwise it might be non-null.

>+#ifndef __MINGW32__
> #ifdef XP_WIN
>     case nsIDownloadManager::DOWNLOAD_SCANNING:
>     {
>       nsresult rv = mDownloadManager->mScanner ? mDownloadManager->mScanner->ScanDownload(this) : NS_ERROR_NOT_INITIALIZED;
>       // If we failed, then fall through to 'download finished'
>       if (NS_SUCCEEDED(rv))
>         break;
>       mDownloadState = aState = nsIDownloadManager::DOWNLOAD_FINISHED;
>     }
> #endif
>+#endif

What happens if someone sets the state to scanning (as is done in OnStateChange IIRC).

Comment 11

[Martijn Wargers (dead)]

Attachment: patch

Sorry, I was happy that I got my build to work again, I didn't really think any further than that.

I've now filed a mingw bug, at: https://sourceforge.net/tracker/index.php?func=detail&aid=1785282&group_id=2435&atid=352435

Comment 12

[Martijn Wargers (dead)]

Ping?

Comment 13

[Rob Arnold [:robarnold]]

Comment on attachment 279032 [details] [diff] [review]
patch

Looks good technically. Just two style nits.

1. You have this pattern very often:
>+#ifndef __MINGW32__
> #ifdef XP_WIN
...
> #endif
>+#endif

which I think would be better replaced by

>-#ifdef XP_WIN
>+#if defined(XP_WIN) and !defined(__MINGW32__)
...
> #endif

since fewer preprocessor statements are (in general) better for readability/maintainability.

>-#ifdef XP_WIN
>+#if defined(WIN32) && !defined(__MINGW32__)
>       (void)SetState(nsIDownloadManager::DOWNLOAD_SCANNING);
> #else
>       (void)SetState(nsIDownloadManager::DOWNLOAD_FINISHED);
> #endif
>     }

I'm not sure why you switched to checking for WIN32, but it's prevailing style to check for XP_WIN throughout the Mozilla codebase. I don't think there's any situation where the two would give different behavior.

Comment 14

[Martijn Wargers (dead)]

Attachment: patchv2

Sorry for the delay, thanks for the review, I think this addresses your comments.

Comment 15

[Rob Arnold [:robarnold]]

Comment on attachment 294450 [details] [diff] [review]
patchv2

>+ifndef GNU_CXX
> ...
>+endif

I haven't actually tried this out, but I assume that it works.

>+#ifndef __MINGW32__
> #ifdef XP_WIN
>   nsDownloadManager() : mScanner(nsnull) { };
> private:
>   nsDownloadScanner *mScanner;
> #endif
>+#endif

This one still needs to be consolidated into a single if/endif pair.

Comment 16

[Martijn Wargers (dead)]

Attachment: patchv2

Sorry, I had that one actually changed, but I forgot to rediff it.
I'll check the >+ifndef GNU_CXX thing on mozilla-build.

Comment 17

[Martijn Wargers (dead)]

Attachment: patchv3

Comment 18

[Rob Arnold [:robarnold]]

Comment on attachment 295935 [details] [diff] [review]
patchv3

looks good

Comment 19

[Martijn Wargers (dead)]

Comment on attachment 295935 [details] [diff] [review]
patchv3

Thanks, asking approval.

Comment 20

[Mike Connor [:mconnor]]

Comment on attachment 295935 [details] [diff] [review]
patchv3

a=mconnor on behalf of drivers for 1.9 checkin

mingw builds don't get anything cool ;)

Comment 21

[Martijn Wargers (dead)]

Checking in Makefile.in;
/cvsroot/mozilla/toolkit/components/downloads/src/Makefile.in,v  <--  Makefile.i
n
new revision: 1.18; previous revision: 1.17
done
Checking in nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--
nsDownloadManager.cpp
new revision: 1.153; previous revision: 1.152
done
Checking in nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  ns
DownloadManager.h
new revision: 1.58; previous revision: 1.57
done

Checked into trunk.

Comment 22

[Evan Yan]

This change makes Sun CC unhappy.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Ports/1199883000.1199883184.10686.gz#err6

Building deps for nsDownloadManager.cpp using Sun Studio CC
NEXT ERROR "nsDownloadManager.h", line 74: Error: Badly formed constant expression.
"nsDownloadManager.h", line 91: Error: Badly formed constant expression.
"nsDownloadManager.cpp", line 132: Error: Badly formed constant expression.
"nsDownloadManager.cpp", line 841: Error: Badly formed constant expression.
"nsDownloadManager.cpp", line 1784: Error: Badly formed constant expression.
"nsDownloadManager.cpp", line 2088: Error: Badly formed constant expression.

Comment 23

[Ginn Chen]

+#if defined(XP_WIN) && !defined(__MINGW32__)
is better than
+#if defined(XP_WIN) and !defined(__MINGW32__)

right?

Comment 24

[Justin Dolske [:Dolske]]

Attachment: Solaris bustage fix

I just hit bustage on Solaris too. This fixes it.

Comment 25

[Justin Dolske [:Dolske]]

Solaris fix committed...

Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.154; previous revision: 1.153
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.59; previous revision: 1.58
done

Comment 26

[Martijn Wargers (dead)]

Sorry for that (I don't have Solaris, as you can tell).