Plugin tests fail to compile on OS/2

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: mozilla, Assigned: wuno)

Tracking

({fixed1.9.1})

Trunk
x86
All
fixed1.9.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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?
(Assignee)

Comment 1

10 years ago
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.
(Assignee)

Comment 2

10 years ago
Created attachment 355210 [details] [diff] [review]
make plugins/tools/tester compile again for OS/2

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 3

10 years ago
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+
(Assignee)

Comment 4

10 years ago
Created 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 #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)
(Assignee)

Updated

10 years ago
Blocks: 105959
OS: OS/2 → All

Comment 5

10 years ago
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+
(Reporter)

Comment 6

10 years ago
Is this ready to land or do we need sr for this, too?
(Assignee)

Comment 7

10 years ago
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)
(Assignee)

Comment 8

10 years ago
(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.
(Assignee)

Comment 9

10 years ago
(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?

Comment 10

10 years ago
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.

Updated

10 years ago
Attachment #355277 - Flags: superreview?(jst) → superreview+

Comment 11

10 years ago
pushed to mozilla-central

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

Thanks Walter!
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Blocks: 474503
(Assignee)

Comment 12

10 years ago
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).
(Reporter)

Comment 13

10 years ago
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
(Assignee)

Comment 14

10 years ago
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.
(Reporter)

Comment 15

10 years ago
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.
(Assignee)

Comment 16

10 years ago
Created attachment 362399 [details] [diff] [review]
enable the testplugin on OS/2 only when MOZ_OJI is defined (checkin comment 18)

(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)
(Reporter)

Comment 17

10 years ago
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+
(Reporter)

Updated

10 years ago
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)
(Reporter)

Comment 18

10 years ago
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.