Bug 754554 - Review Request: presence - Bi-directional audio/video connections
Summary: Review Request: presence - Bi-directional audio/video connections
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Stanislav Ochotnicky
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-11-16 20:07 UTC by Fabian Deutsch
Modified: 2011-12-06 01:03 UTC (History)
4 users (show)

Fixed In Version: presence-0.4.4-2.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-12-06 01:03:38 UTC
fabian.deutsch: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Fabian Deutsch 2011-11-16 20:07:11 UTC
Spec URL: https://gitorious.org/valastuff/presence/blobs/spec/presence.spec
SRPM URL: http://fabiand.fedorapeople.org/presence/presence-0.4.1-1.fc16.src.rpm
Description:
A simple tool to establish a bi-directional audio/video connection in simple
networks (aka LAN).

$ rpmlint presence.spec
presence.spec:9: W: macro-in-comment %{version}
presence.spec:10: W: macro-in-comment %{version}
presence.spec:10: W: macro-in-comment %{version}
presence.spec:11: W: macro-in-comment %{version}
presence.spec:11: W: macro-in-comment %{version}
presence.spec: W: invalid-url Source0: presence-0.4.1.tar.xz
0 packages and 1 specfiles checked; 0 errors, 6 warnings.

$ rpmlint ~/rpmbuild/SRPMS/presence-0.4.1-1.fc16.src.rpm 
presence.src:9: W: macro-in-comment %{version}
presence.src:10: W: macro-in-comment %{version}
presence.src:10: W: macro-in-comment %{version}
presence.src:11: W: macro-in-comment %{version}
presence.src:11: W: macro-in-comment %{version}
presence.src: W: invalid-url Source0: presence-0.4.1.tar.xz
1 packages and 0 specfiles checked; 0 errors, 6 warnings.

$ rpmlint ~/rpmbuild/SRPMS/presence-0.4.1-1.fc16.src.rpm ~/rpmbuild/RPMS/x86_64/presence-0.4.1-1.fc16.x86_64.rpm ~/rpmbuild/RPMS/x86_64/presence-debuginfo-0.4.1-1.fc16.x86_64.rpm 
presence.src:9: W: macro-in-comment %{version}
presence.src:10: W: macro-in-comment %{version}
presence.src:10: W: macro-in-comment %{version}
presence.src:11: W: macro-in-comment %{version}
presence.src:11: W: macro-in-comment %{version}
presence.src: W: invalid-url Source0: presence-0.4.1.tar.xz
presence.x86_64: W: no-manual-page-for-binary presence
3 packages and 0 specfiles checked; 0 errors, 7 warnings.

Comment 1 Fabian Deutsch 2011-11-16 20:23:59 UTC
More about the tool at http://dummdida.blogspot.com/p/presence.html

Comment 2 Stanislav Ochotnicky 2011-11-17 14:35:16 UTC
If you want to create tarball from git it's better to use "git archive" to do it. Your "tar -cJvf presence-%{version}.tar.xz presence-%{version}" packs .git directory as well. 

Another thing: Upstream version is 0.4 not 0.4.1. Version numbers in Fedora should match versions from upstream tarball. I know you are upstream, but it's still better to be consistent.

License is also incorrect, COPYING file is LGPL 2.1, yet spec states GPLv2+.

Comment 3 Fabian Deutsch 2011-11-17 15:06:40 UTC
Thanks for looking at it.

(In reply to comment #2)
> If you want to create tarball from git it's better to use "git archive" to do
> it. Your "tar -cJvf presence-%{version}.tar.xz presence-%{version}" packs .git
> directory as well. 

True. I wonder why I used tar in the first place.

> Another thing: Upstream version is 0.4 not 0.4.1. Version numbers in Fedora
> should match versions from upstream tarball. I know you are upstream, but it's
> still better to be consistent.

I actually pushed the tag for presence-0.4.1, this should match upstream. Or am I missing something?

> License is also incorrect, COPYING file is LGPL 2.1, yet spec states GPLv2+.

Ouch, will be fixed.

Comment 4 Stanislav Ochotnicky 2011-11-17 15:15:07 UTC
(In reply to comment #3)
> > Another thing: Upstream version is 0.4 not 0.4.1. Version numbers in Fedora
> > should match versions from upstream tarball. I know you are upstream, but it's
> > still better to be consistent.
> 
> I actually pushed the tag for presence-0.4.1, this should match upstream. Or am
> I missing something?

Well, configure.ac in tarball states it's version 0.4 so I assumed that. But you're right that it's created from 0.4.1 tag so I guess it's more of a configure.ac problem.

Comment 5 Fabian Deutsch 2011-11-17 19:45:23 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > > Another thing: Upstream version is 0.4 not 0.4.1. Version numbers in Fedora
> > > should match versions from upstream tarball. I know you are upstream, but it's
> > > still better to be consistent.
> > 
> > I actually pushed the tag for presence-0.4.1, this should match upstream. Or am
> > I missing something?
> 
> Well, configure.ac in tarball states it's version 0.4 so I assumed that. But
> you're right that it's created from 0.4.1 tag so I guess it's more of a
> configure.ac problem.

Ah yes, I need to get used to update all those relevant places ...
One question remains for me: How do I "mark" the software as GPLv2+ (and not just GPLv2)?

I've updated the spec at
https://gitorious.org/valastuff/presence/blobs/spec/presence.spec

The new srpm is here
http://fabiand.fedorapeople.org/presence/presence-0.4.2-1.fc15.src.rpm

and the corresponding koji task is
http://koji.fedoraproject.org/koji/taskinfo?taskID=3522354

Comment 6 Stanislav Ochotnicky 2011-11-17 21:25:54 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > > Another thing: Upstream version is 0.4 not 0.4.1. Version numbers in Fedora
> > > > should match versions from upstream tarball. I know you are upstream, but it's
> > > > still better to be consistent.
> > > 
> > > I actually pushed the tag for presence-0.4.1, this should match upstream. Or am
> > > I missing something?
> > 
> > Well, configure.ac in tarball states it's version 0.4 so I assumed that. But
> > you're right that it's created from 0.4.1 tag so I guess it's more of a
> > configure.ac problem.
> 
> Ah yes, I need to get used to update all those relevant places ...
> One question remains for me: How do I "mark" the software as GPLv2+ (and not
> just GPLv2)?

This is usually done by adding "or (at your option) any later version" to licensing headers of source files. Look for "How to Apply These Terms to Your New Programs" in your COPYING file.

Comment 7 Fabian Deutsch 2011-11-18 08:52:10 UTC
I've added the appropriate headers to all source files and a small script to generate to archive.

specfile:
https://gitorious.org/valastuff/presence/blobs/spec/presence.spec

srpm:
http://fabiand.fedorapeople.org/presence/presence-0.4.4-1.fc16.src.rpm

koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3523291

Comment 8 Stanislav Ochotnicky 2011-11-18 09:28:30 UTC
OK, since I've already had a look, I might just as well do the official review :-)

Comment 9 Fabian Deutsch 2011-11-18 10:01:15 UTC
That's "hammer "(de_DE, variant of awesome) :) 
Thanks for the guidance and patience up to now.

Comment 10 Stanislav Ochotnicky 2011-11-18 10:45:10 UTC
Package Review
==============

Key:
- = N/A
x = Check
! = Problem
? = Not evaluated



==== C/C++ ====
[x]: MUST Header files in -devel subpackage, if present.
[x]: MUST Package does not contain any libtool archives (.la)
[x]: MUST Package contains no static executables.
[x]: MUST Rpath absent or only used for internal libs.
[x]: MUST Package is not relocatable.


==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at least one supported architecture.
[x]: MUST All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).(EPEL6 & Fedora < 13)
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package contains a properly installed %{name}.desktop using desktop-file-install file if it is a GUI application.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[!]: MUST Package run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) and the beginning of %install. (EPEL5)
[x]: 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 is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
[x]: MUST Package consistently uses macros (instead of hard-coded directory names).
[x]: MUST Package meets the Packaging Guidelines.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generates any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[!]: MUST Package must own all directories that it creates.
/usr/share/presence is unowned

[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Requires correct, justified where necessary.
[!]: MUST Rpmlint output is silent.

        rpmlint presence-0.4.4-1.fc17.src.rpm
        ================================================================================
        presence.src:9: W: macro-in-comment %{version}
        presence.src:10: W: macro-in-comment %{version}
        presence.src:10: W: macro-in-comment %{version}
        presence.src: W: invalid-url Source0: presence-0.4.4.tar.xz
        1 packages and 0 specfiles checked; 0 errors, 4 warnings.
        ================================================================================

        rpmlint presence-0.4.4-1.fc17.i686.rpm
        ================================================================================
        presence.i686: W: no-manual-page-for-binary presence
        1 packages and 0 specfiles checked; 0 errors, 1 warnings.
        ================================================================================

        rpmlint presence-debuginfo-0.4.4-1.fc17.i686.rpm
        ================================================================================
        1 packages and 0 specfiles checked; 0 errors, 0 warnings.
        ================================================================================
Well now that you generate tarball with a script, it would be best to
get rid of those macros in comments

[!]: MUST Sources used to build the package matches the upstream source, as provided in the spec URL.
        presence-0.4.4.tar.xz :
          MD5SUM this package     : 7d3b9be502690752ee443e4434b657ee
          MD5SUM upstream package : upstream source not found

verified they match when regenerated
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q --requires).
[!]: SHOULD Package functions as described.

While I don't have a webcam in my pc, I'd assume the application shows icons in bottom screen area. Those buttons work, but they have no icons so it's impossible to navigate

[x]: SHOULD Package does not include license text files separate from upstream.
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported architectures.
[-]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: MUST Package run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) and the beginning of %install. (EPEL5)
 - just in case you want EPEL5 branch, then you'd need defattr back as well and buildroot def. I guess not...
[!]: MUST Package must own all directories that it creates.
  /usr/share/presence is unowned
[!]: SHOULD Package functions as described.

While I don't have a webcam in my pc, I'd assume the application shows icons in bottom screen area. Those buttons work, but they have no icons so it's impossible to navigate. Perhaps some missing requires?

Comment 11 Fabian Deutsch 2011-11-18 12:04:04 UTC
(In reply to comment #10)

> Issues:
> [!]: MUST Package run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) and the
> beginning of %install. (EPEL5)

Fixed.

>  - just in case you want EPEL5 branch, then you'd need defattr back as well and
> buildroot def. I guess not...

I am not yet interested in getting this into EPEL :)

> [!]: MUST Package must own all directories that it creates.
>   /usr/share/presence is unowned

I suppose this was because of globbing, I added the dir explicitly.

> [!]: SHOULD Package functions as described.
> 
> While I don't have a webcam in my pc, I'd assume the application shows icons in
> bottom screen area. Those buttons work, but they have no icons so it's
> impossible to navigate. Perhaps some missing requires?

Yes, I suppose you ain't running GNOME? :) I added a dependency on gnome-icon-theme and -symbolic this should solve this.
And you'll need to be creative to test the software on a machine without a webcam :)

specfile:
https://gitorious.org/valastuff/presence/blobs/spec/presence.spec

srpm:
http://fabiand.fedorapeople.org/presence/presence-0.4.4-2.fc16.src.rpm

koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3523562

Comment 12 Stanislav Ochotnicky 2011-11-18 13:04:19 UTC
(In reply to comment #11)
> (In reply to comment #10)
> 
> > Issues:
> > [!]: MUST Package run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) and the
> > beginning of %install. (EPEL5)
> 
> Fixed.

Well this was really needed only for EPEL5, but it doesn't hurt :-)

> I suppose this was because of globbing, I added the dir explicitly.

OK now. Though the "%{_datadir}/presence/*" entry is now redundant :-)


> > [!]: SHOULD Package functions as described.
> > 
> > While I don't have a webcam in my pc, I'd assume the application shows icons in
> > bottom screen area. Those buttons work, but they have no icons so it's
> > impossible to navigate. Perhaps some missing requires?
> 
> Yes, I suppose you ain't running GNOME? :) I added a dependency on
> gnome-icon-theme and -symbolic this should solve this.
> And you'll need to be creative to test the software on a machine without a
> webcam :)

Well I can't seem to make it work in Xnest (segfaults), Xephyr (works, but it has the same problem with icons) even though I already had those icons installed. You are right I am not running Gnome, so my environment is a little non-standard. It shouldn't matter though. I should be able to run any application under any WM/DE. 

There was another weird thing when I tried to resize the window. It jumped to maximum width (as in maxiumu allowed by my graphics gard - wider than my monitor).

Normally I'd probably tell the packager to sort it out with upstream first, but since you are the upstream I think you are in a perfect position to fix this later on...That said:

APPROVED

Comment 13 Fabian Deutsch 2011-11-18 13:34:38 UTC
Unbelievable :) Thanks for this quick review, Stanislav!

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > 
> > > Issues:
> > > [!]: MUST Package run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) and the
> > > beginning of %install. (EPEL5)
> > 
> > Fixed.
> 
> Well this was really needed only for EPEL5, but it doesn't hurt :-)

I'll take this into account and will have a look at other packages what is needed.

> > I suppose this was because of globbing, I added the dir explicitly.
> 
> OK now. Though the "%{_datadir}/presence/*" entry is now redundant :-)

Hum, I thought that this just pulls the path, but not it's children.
Will be removed though.

> > > [!]: SHOULD Package functions as described.
> > > 
> > > While I don't have a webcam in my pc, I'd assume the application shows icons in
> > > bottom screen area. Those buttons work, but they have no icons so it's
> > > impossible to navigate. Perhaps some missing requires?
> > 
> > Yes, I suppose you ain't running GNOME? :) I added a dependency on
> > gnome-icon-theme and -symbolic this should solve this.
> > And you'll need to be creative to test the software on a machine without a
> > webcam :)
> 
> Well I can't seem to make it work in Xnest (segfaults), Xephyr (works, but it
> has the same problem with icons) even though I already had those icons
> installed. You are right I am not running Gnome, so my environment is a little
> non-standard. It shouldn't matter though. I should be able to run any
> application under any WM/DE. 

Crashes in Xnest as expected as it requires GL, which ain't supported by Xnest (afaik). I am also seeing the icon problems on Fedora 15, but they are not present on Fedora 16. Are you running F15?

> There was another weird thing when I tried to resize the window. It jumped to
> maximum width (as in maxiumu allowed by my graphics gard - wider than my
> monitor).

This is weird. I actually don't do much with the window size, just restricting the aspect ratio and switching to fullscreen on demand. Maybe a result of the WM and aspect-ratio-based-resizing.

> Normally I'd probably tell the packager to sort it out with upstream first, but
> since you are the upstream I think you are in a perfect position to fix this
> later on...That said:
>
> APPROVED

What a friday :) Thanks, fabian.

Comment 14 Fabian Deutsch 2011-11-22 16:26:02 UTC
New Package SCM Request
=======================
Package Name: presence
Short Description: Bi-directional audio/video connections
Owners: fabiand
Branches: f16
InitialCC:

Comment 15 Gwyn Ciesla 2011-11-22 16:31:21 UTC
Git done (by process-git-requests).

Comment 16 Fedora Update System 2011-11-25 09:17:19 UTC
presence-0.4.4-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/presence-0.4.4-2.fc16

Comment 17 Fedora Update System 2011-11-25 23:29:32 UTC
presence-0.4.4-2.fc16 has been pushed to the Fedora 16 testing repository.

Comment 18 Fedora Update System 2011-12-06 01:03:38 UTC
presence-0.4.4-2.fc16 has been pushed to the Fedora 16 stable repository.


Note You need to log in before you can comment on or make changes to this bug.