Closed Bug 471759 Opened 11 years ago Closed 11 years ago

Plugin tests fail to compile on OS/2

Categories

(Core :: Plug-ins, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mozilla, Assigned: wuno)

References

Details

(Keywords: fixed1.9.1)

Attachments

(2 files, 1 obsolete file)

When compiling with tests enabled I get the following error messages

make.exe[6]: Entering directory `<objdir>/modules/plugin/tools/tester/common'
log.cpp
In file included from <srcdir>/modules/plugin/tools/tester/common/log.cpp:38:
<srcdir>/modules/plugin/tools/tester/include/xp.h:78:19: npupp.h: No such file or directory
In file included from <srcdir>/modules/plugin/tools/tester/common/log.cpp:40:
<srcdir>/modules/plugin/tools/tester/include/log.h: In destructor `
   LogArgumentStruct::~LogArgumentStruct()':
<srcdir>/modules/plugin/tools/tester/include/log.h:58: warning: deleting
   `void*' is undefined
make.exe[6]: *** [log.o] Error 1
make.exe[6]: Leaving directory `<objdir>/modules/plugin/tools/tester/common'


Similarly for tools/tester/os2:

os2entry.cpp
In file included from <srcdir>/modules/plugin/tools/tester/os2/os2entry.cpp:45:
<srcdir>/modules/plugin/tools/tester/include/xp.h:78:19: npupp.h: No such file or directory
In file included from X:/central/mozilla/modules/plugin/tools/tester/os2/os2entry.cpp:48:
<srcdir>/modules/plugin/tools/tester/os2/os2utils.h:44:25: warning: no newline at end of file
<srcdir>/modules/plugin/tools/tester/os2/os2entry.cpp: In function `
   long unsigned int _DLL_InitTerm(long unsigned int, long unsigned int)':
<srcdir>/modules/plugin/tools/tester/os2/os2entry.cpp:63: warning: unused
   variable `APIRET rc'
make.exe[2]: *** [os2entry.o] Error 1
make.exe[1]: *** [tools] Error 2


I wonder if these tests are still useful at all or if they should be removed. I notice that all other platforms only compile
   TOOL_DIRS += sdk
so is tools/tester obsolete by now and we should instead compile sdk on OS/2, too? Or should I fix that?


Dave, Josh, you seem to have done the most recent work on the plugins stuff. Any hints?
Peter, I investigated the changes in the module/plugin directory and found the corresponding bugs. I have now a patch making it build with enable-test. I want to test it again tomorrow before I'll attach it.
I changed modules/plugin/tools/tester/include/xp.h to include npfunctions.h only in the OS/2 relevant part (follow up to bug 455458), the Windows part still includes npupp.h, but except OS/2 the tools/tester dir seems not to be built on any other platform (bug105959 might change this?), thus letting the XP_WIN parts alone.
modules/plugin/tools/tester/common/{npn_gate,npp_gate}.cpp were chanched according to what I found in the respective files you checked in with http://hg.mozilla.org/mozilla-central/rev/f76419292bfd
Assignee: nobody → wuno
Status: NEW → ASSIGNED
Attachment #355210 - Flags: review?(joshmoz)
Comment on attachment 355210 [details] [diff] [review]
make plugins/tools/tester compile again for OS/2

thanks, you can probably make this bug for all platforms, I doubt this is OS/2 only
Attachment #355210 - Flags: review?(joshmoz) → review+
(In reply to comment #3)
> (From update of attachment 355210 [details] [diff] [review])
> thanks, you can probably make this bug for all platforms, I doubt this is OS/2
> only
Ok, I updated the patch a bit that xp.h is corrected for all platforms. Tried to build on linux only tester/common (no tester/unix directory available). This try unraveled a typo in include/logger.h including logFile.h but the name of the header is logfile.h (which is not a problem on case-insensitive OSes). However even after fixing this typo linux didn't successfully build. /../include/plugbase.h:63: error: '_MAX_PATH' was not declared in this scope.
Looking at the history of these files, there was almost no changes since Aviary times, so I doubt that linux ever would have built successfully the tester plugin. Chances might be better for windows, but I don't have the time to try this also. Moreover, this patch doesn't cover {npn,npp}_gate.cpp files in the tools/spy/common and sdk/samples/npthread/windows/ directories. I will set this bug to block bug105959 assigned to you.
Attachment #355210 - Attachment is obsolete: true
Attachment #355277 - Flags: review?(joshmoz)
Blocks: 105959
OS: OS/2 → All
Comment on attachment 355277 [details] [diff] [review]
same patch for {npn,npp}_gate.cpp, include npfunctions.h for all platforms in xp.h, correct a typo in include/logger.h (checkin: comment 11 and 13)

Whether or not this does everything we'd eventually want I don't know, but this is a good start.
Attachment #355277 - Flags: review?(joshmoz) → review+
Is this ready to land or do we need sr for this, too?
Comment on attachment 355277 [details] [diff] [review]
same patch for {npn,npp}_gate.cpp, include npfunctions.h for all platforms in xp.h, correct a typo in include/logger.h (checkin: comment 11 and 13)

(In reply to comment #6)
> Is this ready to land or do we need sr for this, too?
good question, since landings for bug 455458 and follow up bugs had sr from jst I will ask him.
Attachment #355277 - Flags: superreview?(jst)
(In reply to comment #5)
> (From update of attachment 355277 [details] [diff] [review])
> Whether or not this does everything we'd eventually want I don't know, but this
> is a good start.

I found a bit time to build on windows and it builds the tester/common and tester/windows dirs, when you adjust the Makefiles accordingly.
(In reply to comment #0)
Josh, beside fixing the stuff in the tools/tester directories, there is still the question of comment 0 open: 
> I wonder if these tests are still useful at all or if they should be removed. I
> notice that all other platforms only compile
>    TOOL_DIRS += sdk
> so is tools/tester obsolete by now and we should instead compile sdk on OS/2,
> too? Or should I fix that?
> 
> 
> Dave, Josh, you seem to have done the most recent work on the plugins stuff.
> Any hints?
You should get sr.

We should take this patch and worry about whether we're going to keep things or not later. I am hoping to get to fixing up or removing this code in the next few months, this patch only does good for now.
Attachment #355277 - Flags: superreview?(jst) → superreview+
pushed to mozilla-central

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

Thanks Walter!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 474503
Peter, I think we should get this also into 1.9.1-Branch in order to be able to build with enable-tests. Do we need to ask for approval? In principle, OS/2 is the only platform using these files (though the patch touched files of other platforms, but they are not used at the moment).
Yes, it's NPOTB (Not Part Of The Build), no approval needed.  Checked in to mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/780ac66c8d2d

(Good that Josh wants to improve this plugin stuff in the future, because it's very confusing that these files are in the tree several times, with somewhat different code in them...)
Keywords: fixed1.9.1
I see still some problems with a test-enabled thunderbird
g++ -o logger.o -c  -D_IMPL_NS_GFX -D_IMPL_NS_MSG_BASE -D_IMPL_NS_WIDGET  -DMOZ_THUNDERBIRD=1 -DOSTYPE=\"OS22.45\" -DOSARCH=OS2 -IE:/usr/src/hg/comm-central/mozilla/modules/plugin/tools/tester/common/../include  -IE:/usr/src/hg/comm-central/mozilla/modules/plugin/tools/tester/common -I. -I../../../../../dist/include/plugin -I../../../../../dist/include   -I../../../../../dist/include/plugin -IE:/mozbuild3/mozilla/dist/include/nspr     -IE:/mozbuild3/mozilla/dist/sdk/include       -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -fno-strict-aliasing -Zomf -pipe  -DNDEBUG -DTRIMMED -O2   -DMOZILLA_CLIENT -include ../../../../../mozilla-config.h -Uunix -U__unix -U__unix__ -Wp,-MD,.deps/logger.pp E:/usr/src/hg/comm-central/mozilla/modules/plugin/tools/tester/common/logger.cpp
In file included from E:/usr/src/hg/comm-central/mozilla/modules/plugin/tools/tester/common/../include/logger.h:41,
                 from E:/usr/src/hg/comm-central/mozilla/modules/plugin/tools/tester/common/logger.cpp:39:
E:/usr/src/hg/comm-central/mozilla/modules/plugin/tools/tester/common/../include/plugbase.h:63: error: '_MAX_PATH' was not declared in this scope
E:/usr/src/hg/comm-central/mozilla/modules/plugin/tools/tester/common/../include/plugbase.h:102: error: 'uint16' has not been declared
E:/usr/src/hg/comm-central/mozilla/modules/plugin/tools/tester/common/../include/plugbase.h:115: error: 'uint16' has not been declared
make.exe[6]: *** [logger.o] Error 1
Seamonkey does not have this problem. In tb oji is disabled, maybe plugin tests should be disabled for thunderbird at all.
That's really weird, since _MAX_PATH is used all over plugin/tools/tester. If you use the preprocessor on the same file in a Firefox build tree, what do you see where it gets set? Same for uint16.
(In reply to comment #15)
> That's really weird, since _MAX_PATH is used all over plugin/tools/tester. If
> you use the preprocessor on the same file in a Firefox build tree, what do you
> see where it gets set? Same for uint16.
logger.cpp includes ../include/xp.h -> includes modules/plugin/base/public/npapi.h -> ifdef MOZ_OJI includes sun-java/stubs/include/jri.h -> includes jritypes.h -> includes <stdlib.h> -> defines _MAX_PATH=260 thats for this define, it comes from stdlib.h

jri.h includes as well jri_md.h -> includes nsprpub/pr/include/prtypes.h "typedef unsigned short PRUint16" -> includes nsprpub/pr/include/obsolete/protypes.h "typedef PRUint16 uint16".

In other words these headers get included only when MOZ_OJI is set, which is not the case for TB. This also explains, why the tester plugin gets built for firefox or seamonkey.
The patch of the Makefile prevents building  the tester plugin when MOZ_OJI is not defined.
Attachment #362399 - Flags: review?(mozilla)
Comment on attachment 362399 [details] [diff] [review]
enable the testplugin on OS/2 only when MOZ_OJI is defined (checkin comment 18)

OK, I'm convinced. :-) Will push this once my new computer is in shape for that -- and tinderbox is green.
Attachment #362399 - Flags: review?(mozilla) → review+
Attachment #355277 - Attachment description: same patch for {npn,npp}_gate.cpp, include npfunctions.h for all platforms in xp.h, correct a typo in include/logger.h → same patch for {npn,npp}_gate.cpp, include npfunctions.h for all platforms in xp.h, correct a typo in include/logger.h (checkin: comment 11 and 13)
Comment on attachment 362399 [details] [diff] [review]
enable the testplugin on OS/2 only when MOZ_OJI is defined (checkin comment 18)

Pushed:
http://hg.mozilla.org/mozilla-central/rev/b5359ab6a52c
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f38dee2a8ccd
Attachment #362399 - Attachment description: enable the testplugin on OS/2 only when MOZ_OJI is defined → enable the testplugin on OS/2 only when MOZ_OJI is defined (checkin comment 18)
You need to log in before you can comment on or make changes to this bug.