Re: [Gnash-dev] [patch] Fix linker problem with current CVS, and enable more warnings

[Petter Reinholdtsen]

[Rob Savoye]
>   Got this one added now.

Great.

> >While testing, I notice that the source build with very few compiler
> >warnings enabled by default.  I suggest adding more warning flags to
> >the build, for example like this:
> 
> There's been discussion of this in the past. Basically we'd all love
> to have more warnings, but we don't want to use -Wall till more of
> the warnings are cleared up. Unfortunately much of the older code
> Gnash is based on is pretty ugly... Somebody was planning to do
> this, but the cleanup hasn't been done yet.

Well, as a debian maitainer, one of the first things I do when I
consider maintaining a package, is to see how much warnings are
produced by the compiler when lots of warnings are enabled, and if it
is a lot I normally postpone maintaining the package until the code
quality improves.  And besides, a lot of the warnings from gcc are
errors from other compilers, so I really recommend getting all the
warnings reported with the patch I proposed fixed.  Of course that
includes getting it to build with -ansi if you want the code to build
on Irix and HP-UX. :)

Anyway, if you do not want to enable all these warnings by default,
what about making it a configure option?  Something like this should
do the trick.

Index: configure.ac
===================================================================
RCS file: /sources/gnash/gnash/configure.ac,v
retrieving revision 1.53
diff -u -3 -p -r1.53 configure.ac
--- configure.ac        29 Mar 2006 05:42:41 -0000      1.53
+++ configure.ac        29 Mar 2006 06:26:45 -0000
@@ -265,28 +265,32 @@ if test x"$EXEEXT" == "exe"; then
   AC_DEFINE(HAVE_WINSOCK,1,[This is defined is we are on Win32])
 fi

-dnl if test "$GCC" = "yes"; then
-dnl   # Source do not build with -ansi -pedantic yet
-dnl   CXXFLAGS="$CXXFLAGS \
-dnl       -W \
-dnl       -Wall \
-dnl       -Wcast-align \
-dnl       -Wcast-qual \
-dnl       -Wpointer-arith \
-dnl       -Wreturn-type \
-dnl       "
-dnl   CFLAGS="$CFLAGS \
-dnl       -W \
-dnl       -Wall \
-dnl       -Wcast-align \
-dnl       -Wcast-qual \
-dnl       -Wpointer-arith \
-dnl       -Wreturn-type \
-dnl       -Wmissing-declarations \
-dnl       -Wmissing-prototypes \
-dnl       -Wstrict-prototypes \
-dnl       "
-dnl fi
+AC_ARG_ENABLE(lotsa-warnings,
+ [  --enable-lotsa-warnings Turn on tons of compiler warnings (if you're using 
GCC) [default=no]],
+ # We want warnings, lots of warnings :-)
+ [if test "$GCC" = "yes"; then
+  # Source do not build with -ansi -pedantic yet
+  CXXFLAGS="$CXXFLAGS \
+      -W \
+      -Wall \
+      -Wcast-align \
+      -Wcast-qual \
+      -Wpointer-arith \
+      -Wreturn-type \
+      "
+  CFLAGS="$CFLAGS \
+      -W \
+      -Wall \
+      -Wcast-align \
+      -Wcast-qual \
+      -Wpointer-arith \
+      -Wreturn-type \
+      -Wmissing-declarations \
+      -Wmissing-prototypes \
+      -Wstrict-prototypes \
+      "
+  fi
+])

 dnl AC_CONFIG_LINKS(doc/C/images)
 plugmk=""


> I had planned to wait till the plugin was working before making a
> release, but there seems to be more interest in having the
> standalone player released than I thought. I just got the initial
> hack of the plugin working today, so maybe the release will be
> sooner than I thought.

Great. :)

> Well, I get this:
>   Databasen er ikke tilgjengelig. Vennligst prøv igjen senere!
> which doesn't look like a Flash error. :-)

Right.  It reads "The database isn not available.  Please try again
later!".  It is fixed now.  the SWF file is available from
<URL:http://www.boinydalen.no/solsiden.swf>.

> Can I get a copy of the Debian packaging files when it's
> working ? I'd like to add them to the CVS tree so I can also build
> debs.

Sure, you can get a copy.  I started with the attached tarball from
<URL:http://bugs.debian.org/347352>, and removed the code in
debian/rules to apply the patch, as the patch included is no longer
needed.

But do not add the build rules to CVS or at least not in the released
tarball without talking to the prospective maintainer (Miriam Ruiz, I
guess from reading the debian bug report), as some maintainers prefer
to keep the upstream tarball and the debian build rules completly
separate to ease debian package maintainence.