Bug 560240

* The svn checkout appears to be pre-release snapshot of 0.3.0. Hence the following applies:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages

* Can you take a look at the many format string warnings in the build.log? They are reason to be concerned.

* The build.log also prints warnings about automake/aclocal, which should be fixed by regenerating the autotools framework prior to packaging the tarball.

* I highly recommend to move the %check section _after_ the %install section and to point LD_LIBRARY_PATH at the files in %buildroot. That has helped with discovering packaging mistakes at least a couple of times before.


> License: LGPLv2+

* src/getopt.c is not LGPLv2+ but original BSD with advertising clause, which is not GPL compatible according to the licensing matrix.

https://fedoraproject.org/wiki/Licensing/BSD#Original_BSD_License_.28BSD_with_advertising.29
http://fedoraproject.org/wiki/Licensing#SoftwareLicenses

* All other files in src/ are LGPLv3+.

(In reply to comment #1)
> * The svn checkout appears to be pre-release snapshot of 0.3.0. Hence the
> following applies:
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages
> 
It seems to be formal release, the upstream released binrary for libxls 0.3.0,
but there's no source for libxls 0.3.0.
See http://sourceforge.net/projects/libxls/

> * Can you take a look at the many format string warnings in the build.log? They
> are reason to be concerned.
> 
> * The build.log also prints warnings about automake/aclocal, which should be
> fixed by regenerating the autotools framework prior to packaging the tarball.
> 
> * I highly recommend to move the %check section _after_ the %install section
> and to point LD_LIBRARY_PATH at the files in %buildroot. That has helped with
> discovering packaging mistakes at least a couple of times before.



I'll try to fix those problem.
> 
> > License: LGPLv2+
> 
> * src/getopt.c is not LGPLv2+ but original BSD with advertising clause, which
> is not GPL compatible according to the licensing matrix.
> 
> https://fedoraproject.org/wiki/Licensing/BSD#Original_BSD_License_.28BSD_with_advertising.29
> http://fedoraproject.org/wiki/Licensing#SoftwareLicenses
> 
> * All other files in src/ are LGPLv3+.    
Should the license for libxls be changed to LGPLv3 and BSD with advertising?

About ormat string warnings:


xls.c:123: warning: format '%ld' expects type 'long int', but argument 2 has type 'DWORD'
xls.c:151: warning: format '%ld' expects type 'long int', but argument 2 has type 'DWORD'
xls.c:176: warning: format '%ld' expects type 'long int', but argument 2 has type 'DWORD'
xls.c:224: warning: format '%li' expects type 'long int', but argument 3 has type 'DWORD'
xls.c:252: warning: format '% 4ld' expects type 'long int', but argument 2 has 
 

I think its we can skip those warnings, it can works fine because 
"typedef unsigned long DWORD"

automake/aclocal are not needed since I didn't modified Makefile.am, acinclude.m4' or configure.in.

New SPEC:http://dl.dropbox.com/u/1338197/1/libxls.spec
SRPM:http://dl.dropbox.com/u/1338197/1/libxls-0.3.0-2.20100130svn.fc12.src.rpm

> automake/aclocal are not needed since I didn't modified Makefile.am,
> acinclude.m4' or configure.in.

I refer to regenering the autotools framework _prior to_ creating the source tarball. For example, configure.in is newer than configure. Hence the source tarball is mispackaged and also triggers warnings.


> Should the license for libxls be changed to LGPLv3 and BSD with advertising? 

Yes, "LGPLv3+ and BSD with advertising".  Upstream should get rid of src/getopt.c and replace it with something less troublesome.


> I think its we can skip those warnings, it can works fine because 
> "typedef unsigned long DWORD"

Where did you find that?

Here it uses uint32_t, which is unsigned and 32-bit. So, %u not %ld.


There are many more compiler warnings:
http://koji.fedoraproject.org/koji/getfile?taskID=1953267&name=build.log

A few excerpts with reason to be worried:
xls2csv.c:182: warning: comparison with string literal results in unspecified behavior
xls.c:390: warning: overflow in implicit constant conversion
xlstool.c:447: warning: zero-length gnu_printf format string
xlstool.c:456: warning: format '%.*s' expects type 'char *', but argument 4 has type 'WORD *'
xlstool.c:489: warning: spurious trailing '%' in format
xlstool.c:578: warning: embedded '\0' in format

s/regenering/regenerating/

Perhaps, I should package libxls 0.2.0, version 0.3.0 have some many bugs also there's some API changes between 0.2.0 and 0.3.0. I don't know if version 0.3.0 can work with qtiplot.

Should all warnings in libxls be fixed before making libxls available in fedora? Since upstream are not active now, It will cost a lot of time. I test the libxls in both i686(rhel) and x84_64(ubuntu), it works OK!

(In reply to comment #4)
> > automake/aclocal are not needed since I didn't modified Makefile.am,
> > acinclude.m4' or configure.in.
> 
> I refer to regenering the autotools framework _prior to_ creating the source
> tarball. For example, configure.in is newer than configure. Hence the source
> tarball is mispackaged and also triggers warnings.
> 
> 
> > Should the license for libxls be changed to LGPLv3 and BSD with advertising? 
> 
> Yes, "LGPLv3+ and BSD with advertising".  Upstream should get rid of
> src/getopt.c and replace it with something less troublesome.
> 
> 
> > I think its we can skip those warnings, it can works fine because 
> > "typedef unsigned long DWORD"
> 
> Where did you find that?
> 
> Here it uses uint32_t, which is unsigned and 32-bit. So, %u not %ld.
> 
> 
> There are many more compiler warnings:
> http://koji.fedoraproject.org/koji/getfile?taskID=1953267&name=build.log
> 
> A few excerpts with reason to be worried:
> xls2csv.c:182: warning: comparison with string literal results in unspecified
> behavior
> xls.c:390: warning: overflow in implicit constant conversion
> xlstool.c:447: warning: zero-length gnu_printf format string
> xlstool.c:456: warning: format '%.*s' expects type 'char *', but argument 4 has
> type 'WORD *'
> xlstool.c:489: warning: spurious trailing '%' in format
> xlstool.c:578: warning: embedded '\0' in format

By the way, most warnings are useful only in debug mode. The global variable xls_debug is 0 by default.
xls.c:123: warning: format '%ld' expects type 'long int', but argument 2 has
type 'DWORD'
xls.c:151: warning: format '%ld' expects type 'long int', but argument 2 has
type 'DWORD'
xls.c:176: warning: format '%ld' expects type 'long int', but argument 2 has
type 'DWORD'
xls.c:224: warning: format '%li' expects type 'long int', but argument 3 has
type 'DWORD'
xls.c:252: warning: format '% 4ld' expects type 'long int', but argument 2 has 

  122     if (xls_debug) {
  123 	    printf("xls_appendSST %ld\n", size);
  124     }

  150 		if (xls_debug) {
  151         	printf("ln=%ld\n", ln);
  152 		}

xlstool.c:447: warning: zero-length gnu_printf format string
  446     case 0x201:		//BLANK
  447         sprintf(ret,"");
  448         break;
  449     case 0x0BE:		//MULBLANK
  450         sprintf(ret,"");
  451         break;
> xlstool.c:456: warning: format '%.*s' expects type 'char *', but argument 4 has
> type 'WORD *'
  452     case 0x0204:	//LABEL (xlslib generates these)
  453 		lPtr = (WORD *)cell->l;
  454 		len = *lPtr++;
  455 		if(pWB->is5ver) {
  456 			sprintf(ret,"%.*s", len, lPtr);
  457 			//printf("Found BIFF5 string of len=%d \"%s\"\n", len, ret);
  458 		} else
  459 		if((*(char *)lPtr & 0x01) == 0) {
  460 			sprintf(ret,"%.*s", len, (char *)lPtr + 1);	// 1 is the format
  461 			//printf("Found BIFF8/ASCII string of len=%d \"%s\"\n", len, ret);

> Should all warnings in libxls be fixed before making libxls
> available in fedora?

Well, you've already shown that you don't like to fix any of these.

It's a tiny package with only few compiler warnings. Fixing real bugs reduces the risk that some of these issues create breakage, which leads to bug reports.


>  447         sprintf(ret,"");

So what? Even that one would be fixed quickly:
ret[0] = '\0';


> most warnings are useful only in debug mode.

But still you quoted some which have nothing to do with debug output. E.g. the bad sprintfs in API function xls_getfcell.  [Btw, they are not protected against buffer overflows eithers. All the developer does is to add limited safety by creating a static 10KiB array to be used by sprintf. That alone is questionable practise.]


> I test the libxls in both i686(rhel) and x84_64(ubuntu), it works OK!

So, you have a test-case that tests every possible code path of libxls?

    

(In reply to comment #9)
> > Should all warnings in libxls be fixed before making libxls
> > available in fedora?
> 
> Well, you've already shown that you don't like to fix any of these.
> 
> It's a tiny package with only few compiler warnings. Fixing real bugs reduces
> the risk that some of these issues create breakage, which leads to bug reports.
Sorry for my impatience， since libxls are only required by qtiplot, I'll test if libxls 0.3.0 can work with qtiplot. I don't know which version of libxls were used by qtiplot developer, libxls 0.3.0 changes API a lot. 

If libxls 0.3.0 can work with qtiplot, I'll fix those compile warnings. Otherwise, I'll switch to packaging libxls 0.2.0 which has few compile waring. It doesn't deserve to fix a library that are not used by any program.

Closed now, libxls is buggy, latest qtiplot 0.9.8 don't require it anymore.

*** This bug has been marked as a duplicate of bug 989859 ***