Bug 177782

Here's a review:

ok MUST items:

ok - MUST: The spec file name must match the base package %{name}, in the format
%{name}.spec
ok - MUST: The package must be named according to the PackageNamingGuidelines.
ok - MUST: The package must meet the PackagingGuidelines.
ok - MUST: The package must be licensed with an open-source compatible license...
GPL, good.
ok - MUST: If (and only if) the source package includes the text of the
license(s) in its own file, then that file, containing the text of the
license(s) for the package must be included in %doc.
ok - MUST: The License field in the package spec file must match the actual license.
ok - MUST: The spec file must be written in American English.
ok - MUST: The spec file for the package MUST be legible.
ok - MUST: The sources used to build the package must match the upstream source
3c57cd11aba49fe05f77709cd9a7d609  byzanz-0.0.3.tar.gz
ok - MUST: A package must not contain any BuildRequires that are listed in the
exceptions section of PackagingGuidelines.
ok - MUST: All other Build dependencies must be listed in BuildRequires.
ok - MUST: The spec file MUST handle locales properly.
ok - MUST: A package must not contain any duplicate files in the %files listing.
ok - MUST: Permissions on files must be set properly.
ok - MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot}...
ok - MUST: Each package must consistently use macros...
ok - MUST: The package must contain code, or permissable content.
ok - MUST: If a package includes something as %doc, it must not affect the
runtime of the application....
ok - MUST: The package must successfully compile and build into binary rpms on
at least one supported architecture.

Builds ok under devel.
Doesn't build on fc4 due to modular X and unavailable gnome packages,
I assume it's just targeted at devel and beyond.

NEEDINFO MUST items:

1 - MUST: A package must own all directories that it creates.

filesystem package owns /usr/libexec. I guess it's safe to assume filesystem is
installed, but
perhaps it should have a explicit requires for it?

2 - MUST: Packages containing GUI applications must include a %{name}.desktop
file, and that file must be properly installed with desktop-file-install in the
%install section. This is described in detail in the desktop files section of
PackagingGuidelines. If you feel that your packaged GUI application does not
need a .desktop file, you must put a comment in the spec file with your explanation.

Should this package have a .desktop file since it's got a applet?

3 - MUST: rpmlint must be run on every package. The output should be posted in
the review.

Some rpmlint output:

W: byzanz summary-ended-with-dot A desktop recorder.
Should remove . on the end of summary.

W: byzanz non-standard-dir-in-usr libexec
I guess this can be ignored, but see note 1 above.

SHOULD Items:

ok - SHOULD: The reviewer should test that the package builds in mock.
4 - SHOULD: The package should compile and build into binary rpms on all
supported architectures.

I only have a x86 test box. I assume it will build ok on other arches.

5 - SHOULD: The reviewer should test that the package functions as described. ...

I am getting a segfault when I run the application on my devel test machine.
I was running inside a vnc session, but tried both Xfce and gnome.
(I don't have a monitor on my test box right at the moment).

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1211094112 (LWP 23477)]
0x0804d966 in gifenc_quantize_image (
    data=0xb7301800
"I\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\t"...,
width=1024, height=768, bpp=2, rowstride=2048, alpha=1,
    byte_order=1234, max_colors=255) at quantize.c:383
383           GIFENC_READ_TRIPLET (color, row);
(gdb) where
#0  0x0804d966 in gifenc_quantize_image (
    data=0xb7301800
"I\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\tI\t"...,
width=1024, height=768, bpp=2, rowstride=2048, alpha=1,
    byte_order=1234, max_colors=255) at quantize.c:383
#1  0x0804bb62 in byzanz_recorder_run_encoder (data=0x8253ed0)
    at byzanzrecorder.c:442
#2  0x0070f2a4 in g_thread_create_full () from /usr/lib/libglib-2.0.so.0
#3  0x009bf262 in start_thread () from /lib/libpthread.so.0
#4  0x0062c14e in clone () from /lib/libc.so.6

Let me know if I can provide any more information on tracking the segfault down. 

(In reply to comment #1)
> ok - MUST: The package must successfully compile and build into binary rpms on
> at least one supported architecture.
> 
> Builds ok under devel.
> Doesn't build on fc4 due to modular X and unavailable gnome packages,
> I assume it's just targeted at devel and beyond.

Yes, the base package requires gnome-vfs2-devel >= 2.12 and libgnomeui-devel >=
2.12.  FC4 only has 2.10.  The modular X dependency could be worked around but I
doubt that FC4 will ever get a newer version of Gnome.

> NEEDINFO MUST items:
> 
> 1 - MUST: A package must own all directories that it creates.
> 
> filesystem package owns /usr/libexec. I guess it's safe to assume filesystem is
> installed, but
> perhaps it should have a explicit requires for it?



> 2 - MUST: Packages containing GUI applications must include a %{name}.desktop
> file, and that file must be properly installed with desktop-file-install in the
> %install section. This is described in detail in the desktop files section of
> PackagingGuidelines. If you feel that your packaged GUI application does not
> need a .desktop file, you must put a comment in the spec file with your
explanation.
> 
> Should this package have a .desktop file since it's got a applet?

I don't think that a .desktop file would be useful since it's just an applet. 
The applet shows up in the "Add to Panel" dialog (right click on a panel and
then select "Add to Panel...").

> 3 - MUST: rpmlint must be run on every package. The output should be posted in
> the review.
> 
> Some rpmlint output:
> 
> W: byzanz summary-ended-with-dot A desktop recorder.
> Should remove . on the end of summary.

Will fix in the next version.

> W: byzanz non-standard-dir-in-usr libexec
> I guess this can be ignored, but see note 1 above.


> SHOULD Items:
> 
> ok - SHOULD: The reviewer should test that the package builds in mock.
> 4 - SHOULD: The package should compile and build into binary rpms on all
> supported architectures.
> 
> I only have a x86 test box. I assume it will build ok on other arches.

I builds in mock on i386/devel and x86_64/devel.

> 5 - SHOULD: The reviewer should test that the package functions as described. ...
> 
> I am getting a segfault when I run the application on my devel test machine.
> I was running inside a vnc session, but tried both Xfce and gnome.
> (I don't have a monitor on my test box right at the moment).
> 
> [...]
>
> Let me know if I can provide any more information on tracking the segfault down. 

This could be related to the use of VNC.  We probably need to bring this
segfault to the attention of the author (Benjamin Otte).  It works in the few
tests that I did on i386/devel system.

I'll post updated packages in a bit.

Updated Spec/SRPM:

Spec Name or Url: http://www.ocjtech.us/byzanz-0.0.3-2.spec
SRPM Name or Url: http://www.ocjtech.us/byzanz-0.0.3-2.src.rpm

(In reply to comment #2)
>>
>> Builds ok under devel.
>> Doesn't build on fc4 due to modular X and unavailable gnome packages,
>> I assume it's just targeted at devel and beyond.
>
>Yes, the base package requires gnome-vfs2-devel >= 2.12 and libgnomeui-devel >=
>2.12.  FC4 only has 2.10.  The modular X dependency could be worked around but I
>doubt that FC4 will ever get a newer version of Gnome.

No problem there. Just targeting devel is fine.

>> NEEDINFO MUST items:
>>
>> 1 - MUST: A package must own all directories that it creates.
>>
>> filesystem package owns /usr/libexec. I guess it's safe to assume filesystem is
>> installed, but
>> perhaps it should have a explicit requires for it?

Any comments on that? If you are installing a file into /usr/libexec
and the filesystem rpm isn't installed, no one will own that directory.

>> Should this package have a .desktop file since it's got a applet?
>
>I don't think that a .desktop file would be useful since it's just an applet.
>The applet shows up in the "Add to Panel" dialog (right click on a panel and
>then select "Add to Panel...").

ok. Sounds fine.

>> W: byzanz summary-ended-with-dot A desktop recorder.
>> Should remove . on the end of summary.
>
>Will fix in the next version.

Looks good. Thanks.

>> I only have a x86 test box. I assume it will build ok on other arches.
>
>I builds in mock on i386/devel and x86_64/devel.

Good.

>> I am getting a segfault when I run the application on my devel test machine.
>> I was running inside a vnc session, but tried both Xfce and gnome.
>> (I don't have a monitor on my test box right at the moment).
>>
>> [...]
>>
>> Let me know if I can provide any more information on tracking the segfault down.
>
>This could be related to the use of VNC.  We probably need to bring this
>segfault to the attention of the author (Benjamin Otte).  It works in the few
>tests that I did on i386/devel system.
>
>I'll post updated packages in a bit.

Let me know if I can assit in tracking down the vnc related segfault.

If a few other folks could confirm that it's working under regular X
that would be good. I can try and lug a monitor up to my test box at some
point and try it out. 

The only other outstanding issue I see is the (possible) filesystem dependency.

(In reply to comment #4)
> (In reply to comment #2)
> >>
> >> NEEDINFO MUST items:
> >>
> >> 1 - MUST: A package must own all directories that it creates.
> >>
> >> filesystem package owns /usr/libexec. I guess it's safe to assume filesystem is
> >> installed, but
> >> perhaps it should have a explicit requires for it?
> 
> Any comments on that? If you are installing a file into /usr/libexec
> and the filesystem rpm isn't installed, no one will own that directory.

The filesystem package is a required part of a Fedora install - you can't have
one without it.  If you take a look at the requirements chain, filesystem is
required by basesystem, and basesystem is required by glibc.  So to uninstall
filesystem you'd have to uninstall glibc.  Since explicit requirements on glibc
are not needed, I don't think that we need an explicit requirement on filesystem.

> >> I am getting a segfault when I run the application on my devel test machine.
> >> I was running inside a vnc session, but tried both Xfce and gnome.
> >> (I don't have a monitor on my test box right at the moment).
> >>
> >> [...]
> >>
> >> Let me know if I can provide any more information on tracking the segfault
down.
> >
> >This could be related to the use of VNC.  We probably need to bring this
> >segfault to the attention of the author (Benjamin Otte).  It works in the few
> >tests that I did on i386/devel system.
> 
> Let me know if I can assit in tracking down the vnc related segfault.
> 
> If a few other folks could confirm that it's working under regular X
> that would be good. I can try and lug a monitor up to my test box at some
> point and try it out. 

I talked with the author on IRC ("Company" - usually can be found in #gstreamer
on chat.freenode.net).  He said that he'd have to get access to the box in
question or at least get some more information to diagnose the segfault.  Is
there anyone else out there that could give this package a try?

> I don't think that we need an explicit requirement on filesystem.

ok, makes sense. Just wanted to make sure we checked that.

>I talked with the author on IRC ("Company" - usually can be found in #gstreamer
>on chat.freenode.net).  He said that he'd have to get access to the box in
>question or at least get some more information to diagnose the segfault.  Is
>there anyone else out there that could give this package a try?

I am "nirik99" on freenode.net. I will try and look him up later today when I
have time and get him access to my test box (if he wants it). 

Hopefully some other folks can test the package for functionality as well... :) 

I managed to get a monitor/mouse attached to my test box and do some testing. 

It works just fine on a local X session. The segfault must be due to vnc doing
something odd. 

I don't see any further blockers, so this package is APPROVED. 

As soon as the bugzilla component is created we should add a bug on the vnc
issue so it's tracked and solved. 
Thanks for the nifty package. 

Don't forget to close this bug once the package is imported and built. 

Package imported and built on -devel.