Builds fine in a rawhide/x86_64 mock chroot, and an updated beryl-vidcap builds against it as well.

Well,

* License
  - No license text exists, and no source code contains license term.
    tomc, do you agree that this is licensed under GPL? 
    If so, please include GPL license document to your tarball from
    next release.

* Requires for -devel package
  - Is "freeglut-devel" for Requires on -devel package
    is needed? This seems to be of no relation for this
    package.

    And according to the header files included, libX11-devel
    is needed for Requires on -devel package. Note that
    libX11-devel automatically pulls mesa-libGL-devel.

* Timestamps
  - Generally keeping timestamps on text files, such as header
    files is preferrred. For this package, this can be done
    by replacing all "install" in Makefile by "install -p".

    And .. libX11-devel requires mesa-libGL-devel finally.

(In reply to comment #2)
> * License
IMO, this package is a clear case of an improperly licensed piece SW that can't
be shipped.

(In reply to comment #3)
> (In reply to comment #2)
> > * License
> IMO, this package is a clear case of an improperly licensed piece SW that can't
> be shipped.
> 
Well, upstream is checking this report (perhaps) and so
I will expect he/she posts some comments on this.

*License-
In bug 215569, Tom said:

----8<----
[...]capture.c has a standard
header just like any other file in beryl-plugins, and the seom files have no
header, just a LICENSE file in the root directory. If you require that every
file needs to have a GPL header, I can add it, no problem.
----8<----

However, said GPL LICENSE file seems to be missing from the seom tarballs...
Tom, a license file alone added to the seom tarball should be sufficient.


* Requires-
Including freeglut-devel is an error on my part, the spec started life as a copy
of beryl-vidcap's. I'll drop that and add libX11-devel.


* Timestamps-
I'll fix that too.

Updated srpm:
http://people.redhat.com/jwilson/packages/seom/seom-1.0-0.2.159.fc6.src.rpm

> In bug 215569, Tom said:
>
>[...]capture.c has a standard
>header just like any other file in beryl-plugins, and the seom files have no
>header, just a LICENSE file in the root directory.

This does not apply to the seom tarball.

International copyright laws require him to claim ownership (Copyright/Author
disclaimer - Otherwise he can't grant a license) and to provide a License. 
How to do so is arguable.


Also, there is a file inside of the tarball (src/codec.c) containing another
person's copyright without license:
 * Copyright 2006, Lasse Reinhold (lar)
=> Tom is not allowed to claim copyright rsp. re-license this file, unless he
received a license explicitly permitting him to do so from this person.

Pretty sure Tom was talking about the seom tarball, and seom is his work, so
claiming ownership to grant a license shouldn't be a problem, assuming the
rights to codec.c can be ironed out. (Hadn't seen that one, yeah, could indeed
be an issue.) Tom?

Well, two issue.

* License:
  - This must be resolved in a proper way.

* /usr/bin/seom-backup
  - is a shell script and this contains
----------------------------------------------------
if ! which seom-x264 &> /dev/null; then
        echo "You need to install seom-x264"
        exit -1
fi
----------------------------------------------------
   .. however, what is seom-x264?

seom-x264 is a utility to convert seom captures to h.264-formatted video:

http://forum.beryl-project.org/viewtopic.php?f=37&t=1020&start=0

However, I'm fairly certain the build-dep, x264, isn't shippable in Fedora. On
that assumption, should we drop the script from the build, or maybe just drop it
in %docdir non-executable so users can figure it out themselves?

(In reply to comment #10)
> seom-x264 is a utility to convert seom captures to h.264-formatted video:
> 
> http://forum.beryl-project.org/viewtopic.php?f=37&t=1020&start=0
> 
> However, I'm fairly certain the build-dep, x264, isn't shippable in Fedora. On
> that assumption, should we drop the script from the build, or maybe just drop it
> in %docdir non-executable so users can figure it out themselves?
> 

I think this should be dropped.

(In reply to comment #2)
> * License

added LICENSE (GPLv2)
btw, quicklz is licensed under the GPL, so that shouldn't be a problem.

> 
> * Timestamps

No reason to add '-p' when installing seom.pc, because that file is
auto-generated. But I added '-p' when installing the headers..


-$(CC) $(CFLAGS) $(LDFLAGS) -L.libs -o $@ src/$@/main.c -lseom $($@LIBS)
+$(CC) $(CFLAGS) $(EXTRA_CFLAGS) $(INCLUDES) $(LDFLAGS) -L.libs -o seom-$@
src/$@/main.c -lseom $($@LIBS)

'seom-$@' is ok in the srpm, but when compiling seom from sources more than once
it's bad because then make will recompile the apps every time make is executed..  

and, what's wrong with 'libtool --mode=install'?

(In reply to comment #12)
> (In reply to comment #2)
> > * License
> 
> added LICENSE (GPLv2)
> btw, quicklz is licensed under the GPL, so that shouldn't be a problem.

I think we could probably use something along the lines of a README that
includes a bit stating that you're the author of the bulk of the code, with a
reference stating where the codec.c bit came from and that it is also GPL, just
to cover all the bases.

> > 
> > * Timestamps
> 
> No reason to add '-p' when installing seom.pc, because that file is
> auto-generated. But I added '-p' when installing the headers..

Ah, true.

> -$(CC) $(CFLAGS) $(LDFLAGS) -L.libs -o $@ src/$@/main.c -lseom $($@LIBS)
> +$(CC) $(CFLAGS) $(EXTRA_CFLAGS) $(INCLUDES) $(LDFLAGS) -L.libs -o seom-$@
> src/$@/main.c -lseom $($@LIBS)
> 
> 'seom-$@' is ok in the srpm, but when compiling seom from sources more than once
> it's bad because then make will recompile the apps every time make is executed..  

I'll drop that part of my patch.

> and, what's wrong with 'libtool --mode=install'?

For Fedora packages, libtool archives and static libs are forbidden, so it was
solely for the benefit of the package. I'll use a patch that doesn't nuke that,
and will instead remove the file from within the spec.

Other notes on the patch I'm using:
- splitting CFLAGS and EXTRA_CFLAGS makes packaging much easier. Fedora has a
standard set of CFLAGS that are supposed to be used on all packages, so
splitting off the -std=c99 bit into EXTRA_CFLAGS makes life easier, since its
required, but not part of the standard Fedora CFLAGS.
- some of the added $(*DIR) bits are for convenience more than anything, but are
fairly standard.
- you've got lots of extraneous slashes -- $(DESTDIR)/$(PREFIX) works out to
/somewhere//usr -- so I nuke all those.
- I need/want the shared libs, they're greatly preferred over static libs and/or
libtool archives in the Fedora world.


Updated build:

http://people.redhat.com/jwilson/packages/seom/seom-1.0-0.3.161.fc6.src.rpm

(In reply to comment #13)
> I think we could probably use something along the lines of a README that
> includes a bit stating that you're the author of the bulk of the code, with a
> reference stating where the codec.c bit came from and that it is also GPL, just
> to cover all the bases.
> 

will do ... I'll drop you an email as soon as a new tarball is available.

> For Fedora packages, libtool archives and static libs are forbidden, so it was
> solely for the benefit of the package. I'll use a patch that doesn't nuke that,
> and will instead remove the file from within the spec.

I would actually love to drop libtool, but it's a nice tool that adds '-fPIC'
when needed (and other arch-dependent flags). But other than that, I find it
horrible (what are these libtool archives for anyway? The compiler doesn't need
them when linking against the library, right?).

> 
> Other notes on the patch I'm using:
> - splitting CFLAGS and EXTRA_CFLAGS makes packaging much easier. Fedora has a
> standard set of CFLAGS that are supposed to be used on all packages, so
> splitting off the -std=c99 bit into EXTRA_CFLAGS makes life easier, since its
> required, but not part of the standard Fedora CFLAGS.

What about 'CFLAGS += ...' in the Makefile and a configure option --cflags,
would that work for you?

> - some of the added $(*DIR) bits are for convenience more than anything, but are
> fairly standard.

One thing that still bothers be is my use of LIBDIR. I know it's non-standard,
but I find it much more convenient when cross-compiling, because I only have to
set LIBDIR='lib' or 'lib32' and not LIBDIR='$PREFIX/lib'. I also don't really
see a use in installing the libs, apps and headers each into a different prefix.
I'm hard to convince, I admit, but my decisions are rarely set in stone, so I'm
sure if you give me strong, good reasons I may change my use of LIBDIR ;)

> - you've got lots of extraneous slashes -- $(DESTDIR)/$(PREFIX) works out to
> /somewhere//usr -- so I nuke all those.

Yeah, I noticed that, too, but i don't care because it's just a cosmetic change,
or isn't it?

Just a question:

Should I wait this review until a new tarball is released?

(In reply to comment #14)
> will do ... I'll drop you an email as soon as a new tarball is available.

Excellent, thank you.

> > For Fedora packages, libtool archives and static libs are forbidden, so it was
> > solely for the benefit of the package. I'll use a patch that doesn't nuke that,
> > and will instead remove the file from within the spec.
> 
> I would actually love to drop libtool, but it's a nice tool that adds '-fPIC'
> when needed (and other arch-dependent flags). But other than that, I find it
> horrible (what are these libtool archives for anyway? The compiler doesn't need
> them when linking against the library, right?).

I must admit to knowing very little about libtool or ways to replace it...

> > Other notes on the patch I'm using:
> > - splitting CFLAGS and EXTRA_CFLAGS makes packaging much easier. Fedora has a
> > standard set of CFLAGS that are supposed to be used on all packages, so
> > splitting off the -std=c99 bit into EXTRA_CFLAGS makes life easier, since its
> > required, but not part of the standard Fedora CFLAGS.
> 
> What about 'CFLAGS += ...' in the Makefile and a configure option --cflags,
> would that work for you?

I think a CFLAGS += might work, but the preferred way to get our CFLAGS into a
build is via an export, preferrably the one done by the %configure macro (which
also passes in libdir, bindir, sbindir, etc. info, which is part of why I was
dropping the additional *DIR bits into the Makefile).

> > - some of the added $(*DIR) bits are for convenience more than anything, but are
> > fairly standard.
> 
> One thing that still bothers be is my use of LIBDIR. I know it's non-standard,
> but I find it much more convenient when cross-compiling, because I only have to
> set LIBDIR='lib' or 'lib32' and not LIBDIR='$PREFIX/lib'. I also don't really
> see a use in installing the libs, apps and headers each into a different prefix.

Nah, we'd never install those bits in different prefixes either. This again goes
back to the %configure macro, which passes in a full path for LIBDIR. We never
touch LIB though, so I added that to add support for doing essentially what
you're doing w/LIBDIR at the moment. I could be missing something, but I though
that change would still allow you to build and install as you have been by
passing LIB='lib' or 'lib32'.

> I'm hard to convince, I admit, but my decisions are rarely set in stone, so I'm
> sure if you give me strong, good reasons I may change my use of LIBDIR ;)

Hope the above helped... :)

> > - you've got lots of extraneous slashes -- $(DESTDIR)/$(PREFIX) works out to
> > /somewhere//usr -- so I nuke all those.
> 
> Yeah, I noticed that, too, but i don't care because it's just a cosmetic change,
> or isn't it?

Mostly cosmetic, but also the right thing to do. :)


(In reply to comment #15)
> Just a question:
> 
> Should I wait this review until a new tarball is released?

Might as well.

Any progress?
It seems that version 168 is released.

(In reply to comment #17)
> Any progress?
> It seems that version 168 is released.

No noteworthy changes.. I just implemented the RGB->YCbCr conversion as defined
by ITU-R BT.709 (before it was BT.601). Only the conversion matrix changed, a
normal user shouldn't notice any changes in the picture quality.

I see quite a few new seom tarballs (1.0-180 being the latest). Is there reason
to believe the latest is ready for me to revisit packaging?

(In reply to comment #19)
> I see quite a few new seom tarballs (1.0-180 being the latest). Is there reason
> to believe the latest is ready for me to revisit packaging?

There were a few optimizations, though changeset 172 was quite important 
(http://neopsis.com/projects/seom/changeset/172). I'm currently rewriting the
file format and looking how to do sound capturing. All that work is done in
branches and once it is stable, I'll merge it with trunk.

If your last version is <172, I'd suggest to upgrade to at least 172. And you
may want to know that I've made quite big changes to the Makefile in 176, you'll
probably have to update your patch.

( I certainly remember I am reviewing this bug. When
  new package is ready, please upload and I will check it )

Any news on this package?

179 is the latest version of trunk (main development branch). It works
reasonably well and I need no need for an update. It will take some more time to
merge the branch with the trunk, don't count on that it will happen anytime soon.

Jarod, would you try to repackage, again?

Please let me know how we should treat this review request.

Blah, haven't had time to touch beryl much of late. With the pending release of
something from the compiz-fusion (compiz/beryl merger), I'm inclined to just
deep-six this for now. Unless of course part of compiz-fusion needs seom as well...

Yukon (http://neopsis.com/projects/yukon/) needs seom, it would be nice to
create a package for that as well. Do you want me to create a new bug for that?
AFAIK nothing from compiz-fusion needs seom, unless someone writes a vidcap
plugin based on seom.

(In reply to comment #27)
> Yukon (http://neopsis.com/projects/yukon/) needs seom, it would be nice to
> create a package for that as well. Do you want me to create a new bug for that?

"For that" means:
* you want to take over seom maintainership?
* or you want to create a review request for yukon?

3rd option:
* or both?

:)

I'd be happy to give up this package though. My time available for anything
outside kernel-space (aka "my day job") is rather limited these days...

(In reply to comment #28)
> "For that" means:
> * you want to take over seom maintainership?
> * or you want to create a review request for yukon?
> 

I'm not running fedora. But it would be nice if fedora users could install yukon
from an official package. Do you want me to create a new bug to track the review
request for a yukon package?

Generally, the person who is going to maintain the package for fedora opens the
review request. I think there's a package wish list in the fedora wiki, but
yeah, someone has to actually step up to maintain the package before it can be
reviewed for inclusion.

Tomc, if you want to be a yukon maintainer on Fedora, you have
to open a new review request (with creating the spec/srpm of yukon).

And IMO you may also want to be a maintainer for seom on Fedora.

I think we should once close this bug. Any comments?

Agreed. If someone else wants to pick up the ball and run with it, they're more
than welcome, but for now, closing bug WONTFIX.

Thank you.