A better npapi-sdk patch

GoogleCodeExporter commented 8 years ago

There are a few issues with the way npapi-sdk support was added in r480-r483:

#1 - 'const char*' is actually a change made -after- NPAPI_VERSION_MINOR=27 
(npapi-sdk just didn't bump the version).  If anyone happens to have 
xulrunner-2.0 on their system, or the mozilla headers, then the code will still 
fail.

#2 - in ./configure the definition of HAVE_NSPR isn't related to the actual 
check for whether or not NSPR is available.  If npapi-sdk is detected, we 
certainly need to have that defined (and it probably should not be defined if 
xulrunner is to be used instead), but configure should also fail if NSPR itself 
is not detected in this case.

I'm attaching a patch to address these issues.

Original issue reported on code.google.com by i.staken...@gmail.com on 2 Dec 2011 at 7:29

Attachments:

gmp-head.patch

GoogleCodeExporter commented 8 years ago

Sorry but your patch does not work, but it looks interesting and I would have 
committed if it would have worked.

Also please do not include system generated files in the patch such as configure

autoreconf -fiv
autoreconf: Entering directory `.'
autoreconf: configure.in: not using Gettext
autoreconf: running: aclocal --force 
configure.in:110: warning: AC_LANG_CONFTEST: no AC_LANG_SOURCE call detected in 
body
../../lib/autoconf/lang.m4:194: AC_LANG_CONFTEST is expanded from...
../../lib/autoconf/general.m4:2591: _AC_COMPILE_IFELSE is expanded from...
../../lib/autoconf/general.m4:2607: AC_COMPILE_IFELSE is expanded from...
configure.in:110: the top level
autoreconf: configure.in: tracing
configure.in:110: warning: AC_LANG_CONFTEST: no AC_LANG_SOURCE call detected in 
body
../../lib/autoconf/lang.m4:194: AC_LANG_CONFTEST is expanded from...
../../lib/autoconf/general.m4:2591: _AC_COMPILE_IFELSE is expanded from...
../../lib/autoconf/general.m4:2607: AC_COMPILE_IFELSE is expanded from...
configure.in:110: the top level
autoreconf: configure.in: not using Libtool
autoreconf: running: /usr/bin/autoconf --force
configure.in:110: warning: AC_LANG_CONFTEST: no AC_LANG_SOURCE call detected in 
body
../../lib/autoconf/lang.m4:194: AC_LANG_CONFTEST is expanded from...
../../lib/autoconf/general.m4:2591: _AC_COMPILE_IFELSE is expanded from...
../../lib/autoconf/general.m4:2607: AC_COMPILE_IFELSE is expanded from...
configure.in:110: the top level
configure.in:107: error: possibly undefined macro: AC_MSG_ERROR
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.
autoreconf: /usr/bin/autoconf failed with exit status: 1

Original comment by kdeko...@gmail.com on 2 Dec 2011 at 8:02

GoogleCodeExporter commented 8 years ago

Please try this one (without the auto-generated files); it works for me:

[gecko-mediaplayer-read-only]# cat ../gmp-head.patch |patch -p0
patching file src/np_entry.cpp
patching file src/npupp.h
patching file src/npp_gate.cpp
patching file configure.in
[gecko-mediaplayer-read-only]# autoreconf -fiv
autoreconf-2.68: Entering directory `.'
autoreconf-2.68: configure.in: not using Gettext
autoreconf-2.68: running: aclocal --force 
configure.in:104: warning: AC_LANG_CONFTEST: no AC_LANG_SOURCE call detected in 
body
../../lib/autoconf/lang.m4:194: AC_LANG_CONFTEST is expanded from...
../../lib/autoconf/general.m4:2591: _AC_COMPILE_IFELSE is expanded from...
../../lib/autoconf/general.m4:2607: AC_COMPILE_IFELSE is expanded from...
/usr/share/aclocal/pkg.m4:106: PKG_CHECK_MODULES is expanded from...
configure.in:104: the top level
autoreconf-2.68: configure.in: tracing
configure.in:104: warning: AC_LANG_CONFTEST: no AC_LANG_SOURCE call detected in 
body
../../lib/autoconf/lang.m4:194: AC_LANG_CONFTEST is expanded from...
../../lib/autoconf/general.m4:2591: _AC_COMPILE_IFELSE is expanded from...
../../lib/autoconf/general.m4:2607: AC_COMPILE_IFELSE is expanded from...
aclocal.m4:605: PKG_CHECK_MODULES is expanded from...
configure.in:104: the top level
autoreconf-2.68: configure.in: not using Libtool
autoreconf-2.68: running: /usr/bin/autoconf-2.68 --force
configure.in:104: warning: AC_LANG_CONFTEST: no AC_LANG_SOURCE call detected in 
body
../../lib/autoconf/lang.m4:194: AC_LANG_CONFTEST is expanded from...
../../lib/autoconf/general.m4:2591: _AC_COMPILE_IFELSE is expanded from...
../../lib/autoconf/general.m4:2607: AC_COMPILE_IFELSE is expanded from...
aclocal.m4:605: PKG_CHECK_MODULES is expanded from...
configure.in:104: the top level
autoreconf-2.68: running: /usr/bin/autoheader-2.68 --force
configure.in:104: warning: AC_LANG_CONFTEST: no AC_LANG_SOURCE call detected in 
body
../../lib/autoconf/lang.m4:194: AC_LANG_CONFTEST is expanded from...
../../lib/autoconf/general.m4:2591: _AC_COMPILE_IFELSE is expanded from...
../../lib/autoconf/general.m4:2607: AC_COMPILE_IFELSE is expanded from...
aclocal.m4:605: PKG_CHECK_MODULES is expanded from...
configure.in:104: the top level
autoreconf-2.68: running: automake --add-missing --copy --force-missing
configure.in:104: warning: AC_LANG_CONFTEST: no AC_LANG_SOURCE call detected in 
body
../../lib/autoconf/lang.m4:194: AC_LANG_CONFTEST is expanded from...
../../lib/autoconf/general.m4:2591: _AC_COMPILE_IFELSE is expanded from...
../../lib/autoconf/general.m4:2607: AC_COMPILE_IFELSE is expanded from...
aclocal.m4:605: PKG_CHECK_MODULES is expanded from...
configure.in:104: the top level
autoreconf-2.68: Leaving directory `.'
[gecko-mediaplayer-read-only]#

Original comment by i.staken...@gmail.com on 2 Dec 2011 at 8:15

Attachments:

gmp-head.patch

GoogleCodeExporter commented 8 years ago

Also, please feel free to swap AC_MSG_FAILURE for AC_MSG_ERROR , if that works 
better for you.  Both macros have the same syntax, it seems.

Original comment by i.staken...@gmail.com on 2 Dec 2011 at 8:17

GoogleCodeExporter commented 8 years ago

Nope, patch does not work on Fedora 15 (xulrunner 2.0) I had to add []'s around 
the AC_LANG_PROGRAM to make the warnings go away

I also found that the check was in the wrong spot and the the patched files 
were not picking up the config.h file. I have attached my patch for review.

Original comment by kdeko...@gmail.com on 2 Dec 2011 at 8:40

GoogleCodeExporter commented 8 years ago

Attaching correct patch

Original comment by kdeko...@gmail.com on 2 Dec 2011 at 8:42

Attachments:

gmp-head-rework.patch

GoogleCodeExporter commented 8 years ago

Adjustments make sense and look good.  Thanks for ironing out the kinks!

Original comment by i.staken...@gmail.com on 2 Dec 2011 at 8:50

GoogleCodeExporter commented 8 years ago

Committing to SVN r484

Original comment by kdeko...@gmail.com on 2 Dec 2011 at 8:59

Changed state: Fixed

GoogleCodeExporter commented 8 years ago

Issue 162 has been merged into this issue.

Original comment by kdeko...@gmail.com on 3 Dec 2011 at 1:45

GoogleCodeExporter commented 8 years ago

This patch does not work with older xulrunner code, please update your test to 
something that works. Issue #162 was opened to address this.

Original comment by kdeko...@gmail.com on 3 Dec 2011 at 1:46

Changed state: Started

GoogleCodeExporter commented 8 years ago

ok, I have put in a simpler check that works on my Fedora 15 (libxul 2.0) and 
my RHEL 6.2 (libxul 1.9.2) machines. Please examine and comment, if any fixes 
are suggested the fix must work on both versions of libxul or nspr.

Original comment by kdeko...@gmail.com on 3 Dec 2011 at 2:09

GoogleCodeExporter commented 8 years ago

Confirming this fix (r485) works great on ubuntu 10.10

xulrunner-1.9.2 1.9.2.24+build2+nobinonly-0ubuntu0.10.10.1
xulrunner-1.9.2-dev 1.9.2.24+build2+nobinonly-0ubuntu0.10.10.1
libnspr4-0d 4.8.6-0ubuntu1
libnspr4-dev 4.8.6-0ubuntu1

Thanks for your attention and speedy fix.

Original comment by glits...@gmail.com on 3 Dec 2011 at 2:57

GoogleCodeExporter commented 8 years ago

this fix unfortunately won't work.   npapi-sdk has NPAPI_VERSION_MINOR = 27 
too, and it DOES use const char*.   there are no #define's that can be used to 
determine this API change.  that's why I tried to write a check that would only 
succeed when const char* has actually been defined in the header.

Original comment by i.staken...@gmail.com on 3 Dec 2011 at 2:22

GoogleCodeExporter commented 8 years ago

xulrunner-2.0 on my machine also uses NPAPI_VERSION_MINOR = 27 and does use 
const, so I think we are still ok.

Original comment by kdeko...@gmail.com on 3 Dec 2011 at 2:53

GoogleCodeExporter commented 8 years ago

Your version of xulrunner-2 must be patched, then -- this is what mozilla 
upstream provides:

# grep NPP_GetMIMEDesc -r *
mozilla-2.0/modules/plugin/base/public/npapi.h:char* 
NPP_GetMIMEDescription(void);
mozilla-2.0/dist/include/npapi.h:char* NPP_GetMIMEDescription(void);

Also please note "#define NP_VERSION_MINOR 27" is also in these files.

And, for comparison, this is what the current HEAD of npapi-sdk has (at 
code.google.com/p/npapi-sdk):

#define NP_VERSION_MINOR 27
const char* NPP_GetMIMEDescription(void);

Original comment by i.staken...@gmail.com on 5 Dec 2011 at 3:07

GoogleCodeExporter commented 8 years ago

Provide me with a test that works and I'll include it. Fedora 15 with libxul 
contained the const values with MINOR=27

Fedora 16 does as well

$ rpm -q xulrunner-devel
xulrunner-devel-8.0-1.fc16.x86_64
$ grep MINOR npapi.h 
#define NP_VERSION_MINOR 27

Original comment by kdeko...@gmail.com on 5 Dec 2011 at 3:18

GoogleCodeExporter commented 8 years ago

I'm working on it..  i'll do a more comprehensive test this time around 
(against multiple xulrunner's, firefox's, etc); hopefully i'll have something 
that works properly by the end of the day.

Original comment by i.staken...@gmail.com on 5 Dec 2011 at 3:47

GoogleCodeExporter commented 8 years ago

OK, this check should work.  It looks like my original idea was good in 
principle, i just forgot to ensure that 'XP_UNIX' was defined during the check 
(if it isn't then the definition for NP_GetMIMEDescription is ignored in the 
header and so the test always succeeds)

I've added some messaging and fixed the aclocal warnings too.  This worked for 
me on  xulrunner-2, xulrunner-1.9.2.15, npapi-sdk-0.27 (pre-const) and 
npapi-sdk-HEAD.

Original comment by i.staken...@gmail.com on 5 Dec 2011 at 5:06

Attachments:

gmp-configure.patch

GoogleCodeExporter commented 8 years ago

Ok, that patch looks good and works on my two machines. Hopefully it will work 
on the Ubuntu box and I expect it will. 

I made one modification to the patch, I kept the major version check in there. 
Also it looks like the XP_UNIX define was the clincher for making it work. 
Thanks for putting up with me on this.

Original comment by kdeko...@gmail.com on 5 Dec 2011 at 5:22

Changed state: Fixed

GoogleCodeExporter commented 8 years ago

Confirming this patch works on Ubuntu 10.10. Thanks for all your efforts, 
appreciated.

Original comment by glits...@gmail.com on 5 Dec 2011 at 8:17

acidburn0zzz / gecko-mediaplayer

A better npapi-sdk patch #161