Review for release 1:
* RPM name:
 Well i would suggest to uses libMini as this a library only package and
upstream (are you upstream ?) seems to call it libMini whereas archive name is
only MINI. We usually should uses archive name indeed (which is MINI in this
example)... Is it subject to change ?

* Source MINI-8.1.zip is the same as upstream (for stable)
Uses %{version} for the Source0 version as this is the same as the package.
Also, using a dist tag is hightly encouraged for the release field
see http://fedoraproject.org/wiki/Packaging/DistTag

* From http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
- MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for
directory ownership and usability). This is bring by other dependencies at this
time, but i wonder if they are required ? (Mini.pc suggest that programs that
links against it links also to them)

* Mini.pc includes /usr/X11/include whereas this directory doesn't exist (
usually it is /usr/include or /usr/include/X11 )

* Builds fine in mock
* rpmlint of mini src.rpm
W: mini mixed-use-of-spaces-and-tabs (spaces: line 27, tab: line 1)
* rpmlint of mini looks OK
* rpmlint of mini-debuginfo looks OK
* File list of mini-devel looks OK
* File list of mini looks OK
* File list of mini-debuginfo looks OK
* OK at install/uninstall (no runtime test for now)
* Missing SMP flags. If it doesn't build with it, please add a comment
  (wiki: Packaging/Guidelines#parallelmake)
Ok - comments are here - good (whereas I didn't experience it)
* rpmlint mini on installed files:
rpmlint mini
W: mini no-documentation
W: mini unused-direct-shlib-dependency /usr/lib64/libMini.so.0.0.0
/usr/lib64/libcurl.so.3
W: mini unused-direct-shlib-dependency /usr/lib64/libMini.so.0.0.0
/usr/lib64/libglut.so.3
You should report theses upstream
* The package should contain the text of the license 
(license seems to be LGPL as your spec file suggest, but uses have to find the
full license text in writting in the %doc directory)
* You can uses --disable-static instead of --enable-static=no which works also
anyway, but this first one is the usual command...

---

Is it possible to have the license bundled ?
What do you think about the package name ? 
I would suggest MINI (archive name ) or libMini (name on the website)
The second choice seems better from cosmectic view.
Have you submitted your patch upstream ?

Thanks. I think this has everything addressed (I also updated libquikgrid and
VTP to address most of these issues, but I'll hold off on changes to minizip
until I find out if I can get it built as part of zlib).

Spec URL: http://miskatonic.cs.nmsu.edu/pub/libMini.spec
SRPM URL: http://miskatonic.cs.nmsu.edu/pub/libMini-8.1-2.fc7.src.rpm

Here's the changelog:
- Changed source and patch to use version tag
- Changed package name to libMini
- Added dist tag
- Added pkgconfig requires devel
- Fixed X11 include header in Mini.pc
- Changed enable-static=no to disable-static in configure

As for the license, README.html contains the license and a link to gnu.org for
the actual LGPL terms.

I have submitted the patch upstream, but it's likely to be awhile as the
upstream author mentioned he is busy relocating.

Ok fine!
Actually there is two remaining things.
- It is usually better to have a full license text (not in html) in the main
package. If you have contacted upstream about your patch, I would suggest that
you submit also a COPYING file from the full LGPL license...so that could be
bundled in the main package.
- Mini.pc could be renamed libMini.pc (so library, pc file and package name are
the same). Mini.pc can be allowed also anyway...
Usually you should split libs that are required by your package at runtime from
libs that package that link to the -lMini needs (Requires: field instead of
Libs: field).
See man pkg-config... Anyway since not all Required libs are pkgconfig
compatible  (mesa's are not it seems), you could add Requires: xmu xt x11 (or
only xmu since it already return -lXmu -lXt -lX11 ). Those will be returned also
when using pkg-config --libs Mini (or libMini), then leave mesa from the Libs field.
About /usr/include/X11, you can leave it empty as /usr/include is used and this
is the default (x11.pc don't writte it for a sample).

Since all autotools are patched, i would suggest that upstream include your
patch first. So if for any reason, upstream decide to uses cmake, the tweaks
will be differents.. (well that would be a shame, since you've done all the work
well...)

I will review others packages to have a better overview of them...

Here's a revised version:
Spec URL: http://miskatonic.cs.nmsu.edu/pub/libMini.spec
SRPM URL: http://miskatonic.cs.nmsu.edu/pub/libMini-8.1-3.fc7.src.rpm

Changelog:
- Added a copy of the LGPL v2.1 to the patch
- Revised the pkgconfig .pc file in the patch to add x11 dependencies
- Renamed to autotools patch
- Remove mini/GL prior to building

As for the license, I added a copy. I'll also ask upstream to add a copy of the
license as well. But, I don't think there is anything wrong with adding the
verbatim license for now since the license is specified in the distributed
README.html and on the author's website.

As for Mini.pc, I'd rather leave it as Mini.pc. I dislike putting the lib in
front, since .pc files are all libraries.

Thanks for pointing out the xmu.pc; I didn't realize it had a pkgconfig file.

From a brief exchange of E-mails with the upstream, he mentioned that he had
always wanted to incorporate autotools and just hadn't. So, it might be awhile
till the next release, but it sounds like he'll probably go with autotools.

OK for Mini.pc - good

Ok i think this package is good...
minor: make %{?_smp_mflags} has no problems for me - can you gives me the
build.log when it fails for info ?

As the Fedora guidelines say, the license file is a should...
But i really would prefer seeing this "full license text" included...

You may ask upstream to provides a 8.1a release (with license included).
with no modification code so we could integrate this package to the collection...

I will review others packages as this one would be integrated, and until others
problems are solved, i will reconsider this case...
(license is not unsure so that will be easy).

Any news ? there is a :
  libMini 8.3 (stable, with sea rendering)
But unfortunately, no autotools added...

Do we have a chance to have autotools in libMini ?

Also, it would be fine to have sources not melted with binaries...
If ever it is not, then you should made a snapshot script that will:
- Remove unneeded files (binaries)
- patch the autotools.
- run autogen (aclocal, autoconf, automake )
- make dist
Then, updload the tarball from a place (but ideally, from upstream website)
This snapshot script will have to be sourced (Souce10: libMini-snapshot.sh )

libMini 8.4 stable (still no autotools support)
Do you have recent email exchange with the author about merging your patches ?
I wonder if your patch can allow cygwin build (maybe the problem to the merge ?!)

I would be better to have them merged, but since the license is now here:
the package can be acceptable... (even if you patch autotools)
So i hope you can update the spec...

I'll have an updated spec next week, but there is a new dependency, libsquish,
that I have packaged and will also need a review.

Well, it looks like libsquish is a no-go. It is a DXT compression/uncompression
library and, as Tibbs pointed out in the squish review, DXT is patented (and
won't expire until 2017 or so).

I'm working with upstream to get squish support properly built with #defines. He
is also preparing a new release with some updates to the mini apps.

So, when the new upstream release (sans squish) is ready I'll get the spec
updated and uploaded.

Any news ?

Not that if something is patented, we will need to remove DXT from our source
package also. (usually this lead to create a script to do that and eventually
provide an alternative location to download the source code).

Yes. I'll try and post another spec in the next week or two. I don't think that
we'll need to modify the source since the DXT code isn't in libmini. The
patented code is in libsquish which libmini links against (which is now optional).

ping ?

Still here. Probably at least two weeks till I'll have a chance to look at it again.

Here's a new spec and srpm that should take everything into account. As far as
the DXT code goes, there isn't anything to remove in the src tarball since the
DXT code resides in the squish library.

http://miskatonic.cs.nmsu.edu/pub/libMini.spec
http://miskatonic.cs.nmsu.edu/pub/libMini-8.7.5-1.fc9.src.rpm

@Nicolas

Are you still interested in review. It has been 3+ months without updated.
Will wait for week before reassigning back to nobody

Yes i'm still here, but i think the answer to #14 was no, the dxt code cannot go. 
So I don't know how to handle this for now (need to be removed from the code, or provided with a third part repository)


I will look into this, as i don't remember well...

I've been working with upstream to get the autotools files into the project. There should be a new release coming up that incorporates them.

As far as the DXT code goes I don't think it will be a problem. The DXT code is in the squish library, not the mini library. When building, mini provides an option to build against the squish library, but we're disabling that option.

At long last... here is an updated srpm and spec.

http://miskatonic.cs.nmsu.edu/pub/mini.spec
http://miskatonic.cs.nmsu.edu/pub/mini-8.8.6-1.20080915svn.fc9.src.rpm

The biggest changes are:
- Source is from svn
  -- This isn't necessarily bad since this is how minor builds are now coming
     from upstream anyway
  -- svn is the only way to get the autotools until the next major release
- Stuttgart and Yukon demo subpackages are dropped
- The name is changed from libMini to mini to reflect Fedora packaging guidelines
  given the name in svn

Also, just for clarification.... mini doesn't have any DXT code of its own. mini relies on libsquish for a DXT implementation.

Since squish is not available (and won't be available in Fedora) mini is built with the "--with-squish=no" configure option which #defines NOSQUISH. If NOSQUISH is defined the squish library headers are not included and squish is not linked against... thus DXT is not used.

The package does include a squishbase.cpp patch, but this is only because the #ifdef NOSQUISH is in the wrong place... it should be around the #include <squish.h> line instead of the #include <squishbase.h> line.

>- The name is changed from libMini to mini to reflect Fedora packaging
guidelines
Which packaging guideline are you refering to ?
At least Fedora guideline say that the package name is the name of the source tarball (case sensitive). Thus the name defined in the configure.ac as the package name in the autotools case.

But others distribution guideline seems to requires package designed to provide a library (mainly) to have lib as a name prefix(ie Debian). Thus using libMini was a good thing (it help to see what is the purpose and the way it needs to be packaged as). Now if the program is designed to run on mingw or other, maybe it can be named libmini (case insensitive).


DXT code... OKay


Build Requested to Rawhide: success
http://koji.fedoraproject.org/koji/taskinfo?taskID=828135

Local test on Fedora 8 x86_64.
rpmlint on installed package (see attached)
undefined-non-weak-symbol should be fixed at build time (missing -llibrary flag at linking time). 
unused-direct-shlib-dependency can be fixed using : (between %configure and make within the spec file)
# clean unused-direct-shlib-dependencies
sed -i -e 's! -shared ! -Wl,--as-needed\0!g' libtool

pkgconfig support in Mini.pc seems fine but gl.pc and glu.pc doesn't exist in F-8 so pkg-config will fails and you will need a workaround.
I just wonder if some of theses shouldn't be moved to Libs.private instead ?
Then package requirement for -devel should be tweaked. At least libGL, libGLU and libcurl seems required since they are exposed via the API. Not sure if others are relevant.

I don't see the aim of doing Cflags: -DNOSQUISH in Mini.pc since this is not used within the API
Please use libGL-devel and libGLU-devel instead of mesa-libGL-devel and mesa-libGLU-devel. The Virtual Provides will pick the appropriate version.

This is minor for this version, but according to the Mini.pc, the version is 8.8.8. This needs to be set within the rpm package using pre-versioning scheme. (where the release tag is <1).

About pre-version, i'm not against, if it is stable enought so we can start working on dependencies within Rawhide...

Keep up the good work...

I think this covers everything except the missing gl.pc and glu.pc workaround.


http://miskatonic.cs.nmsu.edu/pub/libMini-8.8.8-0.1.1091svn.fc9.src.rpm
http://miskatonic.cs.nmsu.edu/pub/libMini.spec

- Changed mesa-libGL-devel and mesa-libGLU-devel to libGL-devel and libGLU-devel
- Changed name back to libMini
- Changed release to 8.8.8 pre-release and used specific svn release number
- Added 0 to release tag to ensure upgrade path to 8.8.8
- Removed previous patches for new release
- Added Mini.pc.in patch to remove -DNOSQUISH flag
- Added Makefile.am patch to add -lMini to compilation of libMiniSFX
- Added line to remove unused-direct-shlib-dependency warning

It seems good:
Could you make a script to repackage the source tarball from the vcs ?
Why to bundle icons and images as %doc in the -devel package  :
 %doc README.html libMini.css libMini.ico libMini.jpg libMini.ppm ?

Why this directory is deleted ?
%{__rm} -rf mini/GL

What about ABI stability and the fact that Rawhide will enter in development freeze in two weeks ? Wouldn't it be better to wait for a release ?

New upstream release:

http://miskatonic.cs.nmsu.edu/pub/libMini.spec
http://miskatonic.cs.nmsu.edu/pub/libMini-8.9-1.fc9.src.rpm

> Could you make a script to repackage the source tarball from the vcs ?

Taken care of by new upstream release. Should also address stability.

> Why to bundle icons and images as %doc in the -devel package  :
>  %doc README.html libMini.css libMini.ico libMini.jpg libMini.ppm ?

They're used by README.html (and only by README.html) which has API documentation.

> Why this directory is deleted ?
> %{__rm} -rf mini/GL

All it contains are copies of the OpenGL headers for platforms that don't have gl.h available. I took it out to ensure that it builds against headers in /usr/include/GL instead of mini/GL.

1* Fix License to LGPLv2+
I don't think there is a LGPLv2 short name as every files under 2.1 should be redistributable under 3 with the LGPL case (in the contrary of the GPL).

2* -devel requires too much:
from what is exposed to the API (grep include /usr/include/mini/* -R )
Only Requires:	libGL-devel libGLU-devel curl-devel should be needed (along with pkgconfig).
So I wonder if -lglut and requires x11 xmu from Mini.pc shoudn't be moved to Libs.private instead.

3* - the package install headers in the default include directory:
(includedir=/usr/include  from Mini.pc)
This is perfectly possible , but IMO produce common error since pkg-config CFLAGS detection can be avoided by the dependant package. If used accurately pkg-config provides a level of abstraction to provides for the headers in case of dual API (LibMini-1.0.pc can be used to access headers in /usr/include/libMini-1.0/mini/mini.h along with the previous API)
This is just a warning

4* There is a problem at link time indeed. (where sed -i -e 's! -shared ! -Wl,--as-needed\0!g' libtool seems a correct workaround for now).


2* will need to be investigated with a dependent package and 1* could be fixed then. 3* 4* are optionals and can be fixed later.

(In reply to comment #24)
> 1* Fix License to LGPLv2+
> I don't think there is a LGPLv2 short name as every files under 2.1 should be
> redistributable under 3 with the LGPL case (in the contrary of the GPL).

There is an LGPLv2 short name: http://fedoraproject.org/wiki/Licensing

I think this one falls under the LGPLv2 only since the accompanying documentation includes this line in README.html:

Terms of Usage
The terrain renderer is licensed under the terms of the LGPL 2.1. No warranty WHATSOEVER is expressed; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE!

If it had the phrase "under the terms of the LGPL 2.1 or any later version" then it would be LGPLv2+, but I think the docs keep it at LGPLv2.

This is a LGPLv2+, there is no dedicated License for LGPLv2+
The + is usually found on source code headers.

There is a 8.9.2 version. I don't know if it is suitable for packaging.
(depend upon the meaning of the "development version").

Any news ?

Btw I think you were right, the license is indeed LGPLv2

Haven't had much time lately to work on it. Maybe in a couple of weeks.

ping ? 8.9.2 is here at least.

We should be pretty close now...

BTW: about Libs.private, there was a change lately with pkg-config behaviour. 
I think most projects were not using Requires.private appropriately...I don't remember if libMini could have been affected...

There is a new release that incorporates everything upstream except the Mini.pc patch.

Spec: http://miskatonic.cs.nmsu.edu/pub/libMini.spec

SRPM: http://miskatonic.cs.nmsu.edu/pub/libMini-9.0.3-1.fc11.src.rpm

Here is a modification of the Yukon demo that uses the mini pkgconfig:
      http://miskatonic.cs.nmsu.edu/pub/yukon-1.0.0.tar.bz2

To build and run the Yukon demo:
  ./configure
  make
  ./Yukon

Quick notes:
- Why OpenThreads-devel can be enabled ?
- Does GREYCStoration could be usefull ?
- switch to rm -rf GL (moved from Mini/GL to GL)
- squish (if enabled) should uses pkg-config support, I was not able to enable squish support as there is a missing -lsquish at link time. (not needed for this review).
- Is it possible to avoid using the whole autogen.sh ? Given the recent discution on fedora-devel, it will always be best to avoid usage of autotools at build step.
- It would be better to use one BuildRequires per line (easier to see BR changes on cvs commit).

Build time and usage test succeed.

(In reply to comment #31)
> Quick notes:
> - Why OpenThreads-devel can be enabled ?

Upstream supports it.

> - Does GREYCStoration could be usefull ?

Not sure. But, it doesn't look like Fedora already has a GREYCstoration library (just the app and GIMP plugin), and I'm not sure it would be worth packaging one. The GREYCstoration home page says the project is about to die: http://cimg.sourceforge.net/greycstoration/

> - switch to rm -rf GL (moved from Mini/GL to GL)

Fixed.

> - squish (if enabled) should uses pkg-config support, I was not able to enable
> squish support as there is a missing -lsquish at link time. (not needed for
> this review).

I didn't suggest that to upstream because, as distributed, squish doesn't have pkgconfig support. That was something I added to the libsquish packages before the issue of patents came up.

> - Is it possible to avoid using the whole autogen.sh ? Given the recent
> discution on fedora-devel, it will always be best to avoid usage of autotools
> at build step.

It will with the next upstream release. The need for autotools at build is due to the configure.ac patch to 9.0.3 which upstream has already accepted into svn. But, I'd rather build against a stable release and patch than build against svn.

> - It would be better to use one BuildRequires per line (easier to see BR
> changes on cvs commit).
> 

Fixed

> Build time and usage test succeed.  

I've also submitted some patches to upstream's Yukon and Stuttgart applications that build against libMini. Those have been accepted and are released.

I'll submit those for review later today. They're small and straightforward but provide a good way to not only test libMini builds, but provide a way for the developer to check out the library via a graphical application.

Seems all good, can I have a package update in order to finish this review ?

SRPM: http://miskatonic.cs.nmsu.edu/pub/libMini-9.0.3-2.fc11.src.rpm

spec: http://miskatonic.cs.nmsu.edu/pub/libMini.spec

Sorry, I've missed this comment about Mini.pc:
(In reply to comment #29)
...
> BTW: about Libs.private, there was a change lately with pkg-config behaviour. 
> I think most projects were not using Requires.private appropriately...I don't
> remember if libMini could have been affected...  

grep include /usr/include/mini/* -R showed that only curl-devel and libGLU-devel (along with libGL-devel) should be required by libMini-devel, and then:
- If only datatype are needed (only cflags but no need to link with -lcurl -lGLU -lGL), then curl gl glu can be moved to Requires.private.
- If libs are also needed for another package to link with -lMini as a shared object, then theses curl glu gl needs to be keept in the Requires field.

But it would be fine to request for advices.

Now for ultimate correctness, libMini remaining direct dependencies (-lpng -ljpeg, etc) need to be mentioned in Libs.private: so they can be searched when using static linking. (totally optional for this review).

-lglut is probably meant to be moved to Lib.private

In any cases, inaccurate linking dependencies are covered by a "warn upstream" policy according to our guidelines. so I would like to have a general answea; no need to upload a new package.

@Rick Do you understand what the problem is with the current Mini.pc ?

(In reply to comment #36)
> @Rick Do you understand what the problem is with the current Mini.pc ?  

Yes. Just haven't had time to look into the library yet. Things are hectic again...

Ping? Any progress here? Or we can close this review?

Stalled Review. Closing per:
https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
If you ever want to continue with this review, please reopen or
submit new review.