Bug 2405

Hi,

a few findings before a full review ...

* note that it'd be nice to package also the data files available to download

* what is the reason to use the svn checkout?
- I know, the release is quite old but were there any changes relevant to linux that are too hard to backport?
doesn't the upstream plan a fresh release soon?

* when using svn snapshot, please do not use the revision number as a Version
see http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages

instead, you should use Release like "1.20120613svn"

* what is the reason to export empty LD_FLAGS?

* debuginfo files should be handled automatically, why to include "/usr/lib/debug/usr/bin" in %files (which is the source of rpmlint error)?
- and in addition, debuginfos should go into separate package

btw, this dir could be written as %{_prefix}/lib%{_bindir}

* it doesn't build for me ... probably a missing dependency?

...
Compiling lib/ffmpeg/avcodec.pas
Compiling lib/ffmpeg/avutil.pas
Fatal: Can't find unit mathematics used by avutil
Fatal: Compilation aborted
Error: /usr/bin/ppcx64 returned an error exitcode (normal if you did not specify a source file to be compiled)
make[1]: *** [../game/ultrastardx] Error 1
make[1]: Leaving directory `/home/kvolny/rpmbuild/BUILD/ultrastardx/src'
make: *** [all] Error 2

* I will look into the data files. I didn't add them because they're a different source.

* The reason for the svn checkout is because the released version requires too much changes to be compatible with recent ffmpeg versions: the API for ffmpeg seems to have changed drastically, which is reflected in the SVN revision.

* The page you linked to also says "and optionally, up to 13 characters (ASCII) alphanumeric characters that could be useful in finding the revision in the revision control system.", so it is an option according to the Fedora naming guidelines, and I like this system because it's clear at once which version I used for the creation of the SRPM.

* debuginfo will only be handled automatically if they are extracted during RPM generation (as far as I know), which is not the case: ultrastardx generates them (always in /usr/lib, as specified in the SPEC) "manually" while building, thus circumventing the RPM debuginfo extraction.

* The building is indeed broken since a version of ffmpeg that was released sometime after my review request was posted. I will try to see if a recent checkout fixes the problem, and else request a new release from upstream.

I do, however, agree that I would need to edit the version to match the guidelines correctly, by putting the svn checkout date and revision id in Release, and putting Version 1 (for example).

(In reply to comment #2)
> * I will look into the data files. I didn't add them because they're a
> different source.

hm, thinking about it a bit, maybe they'd fit rather in a separate package than in a subpackage, as the data are not likely to change that often

> * The reason for the svn checkout is because the released version requires too
> much changes to be compatible with recent ffmpeg versions: the API for ffmpeg
> seems to have changed drastically, which is reflected in the SVN revision.

okay, let's go with a svn snapshot then

> * The page you linked to also says "and optionally, up to 13 characters (ASCII)
> alphanumeric characters that could be useful in finding the revision in the
> revision control system.", so it is an option according to the Fedora naming
> guidelines, and I like this system because it's clear at once which version I
> used for the creation of the SRPM.

(In reply to comment #3)
> I do, however, agree that I would need to edit the version to match the
> guidelines correctly, by putting the svn checkout date and revision id in
> Release, and putting Version 1 (for example).

I said "Release like ...", not that you have to use this exact string :-)

it's perfectly okay to put in the svn revision number as long as the whole NVR can be sorted chronologically (yes, we have the "Epoch" for special cases, but it is better to avoid it as it is not directly visible to the user)

so, the latest stable version is 1.1 and we can expect the next to be possibly 1.1.1 or 1.2 ... but it is not released yet, and as the code we're going to use is newer than 1.1 but it will be older then the future 1.x(.y), the version-release has to fit somewhere in between


(In reply to comment #2)
> * debuginfo will only be handled automatically if they are extracted during RPM
> generation (as far as I know), which is not the case: ultrastardx generates
> them (always in /usr/lib, as specified in the SPEC) "manually" while building,
> thus circumventing the RPM debuginfo extraction.

well, it may be worthy to take a look if this could be disabled, leaving the work upon rpm

if not, then at least the files should be put into -debuginfo subpackage manually, as debug files should not be installed by default

> * The building is indeed broken since a version of ffmpeg that was released
> sometime after my review request was posted. I will try to see if a recent
> checkout fixes the problem, and else request a new release from upstream.

ok; maybe an explicit version dependency should be added then

A previous package was already in rpmfusion.
I've just retired it since it was not maintained here.
If you want to take-over please use pkgdb.