Bug 1023714

After discussion on previous lpf package it is decided that all lpf packages requires legal review. Blocking on FE-LEGAL.

In this case, I think the legal review is about the LICENSE file in the distribution, also available at http://leamas.fedorapeople.org/lpf-skype/1/LICENSE. The question, thus: is this  non-redistributable license OK when used as a lpf package?

The question whether lpf-* packages are allowed in Fedora is by the fpc [1]. This might mean that this package won't go into fedora. OTOH, the review process is the same here and in rpmfusion, so I guess we could continue that being aware that where the packet eventually goes is up tp the fpc. 

[1] https://fedorahosted.org/fpc/ticket/362

I think there's a leftover in the spec file, the package does not build:

+ /usr/share/lpf/scripts/lpf-setup-pkg /builddir/build/BUILDROOT/lpf-skype-4.2.0.11-1.fc19.x86_64 /builddir/build/SOURCES/skype.spec.in
+ desktop-file-validate /builddir/build/BUILDROOT/lpf-skype-4.2.0.11-1.fc19.x86_64/usr/share/applications/lpf-skype.desktop
/builddir/build/BUILDROOT/lpf-skype-4.2.0.11-1.fc19.x86_64/usr/share/applications/lpf-skype.desktop: file does not exist
error: Bad exit status from /var/tmp/rpm-tmp.FhzCAC (%install)

You can probably just remove line 33 of the spec file.

Updated, new links:
spec: http://leamas.fedorapeople.org/lpf-skype/2/lpf-skype.spec
srpm: http://leamas.fedorapeople.org/lpf-skype/2/lpf-skype-4.2.0.11-2.fc20.src.rpm

Changelog:
* Fri Nov 1 2013 Alec Leamas <leamas> - 4.2.0.11-2
- Adding README
- Fix typo.

Keeping %install section, it looks sane and works for me. Puzzled why it doesn't for you...

(In reply to Alec Leamas from comment #4)
> Keeping %install section, it looks sane and works for me. Puzzled why it
> doesn't for you...

I'm sorry but it's not working with mock. The only source is the Skype spec file; there's nothing installing it in place:

Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.5VStda
+ umask 022
+ cd /builddir/build/BUILD
+ cd /builddir/build/BUILD
+ rm -rf lpf-skype-4.2.0.11
+ /usr/bin/mkdir -p lpf-skype-4.2.0.11
+ cd lpf-skype-4.2.0.11
+ /usr/bin/chmod -Rf a+rX,u+w,g-w,o-w .
+ cp /builddir/build/SOURCES/README README
+ exit 0
Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.2dOP41
+ umask 022
+ cd /builddir/build/BUILD
+ cd lpf-skype-4.2.0.11
+ exit 0
Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.Sl7rWT
+ umask 022
+ cd /builddir/build/BUILD
+ '[' /builddir/build/BUILDROOT/lpf-skype-4.2.0.11-2.fc19.x86_64 '!=' / ']'
+ rm -rf /builddir/build/BUILDROOT/lpf-skype-4.2.0.11-2.fc19.x86_64
++ dirname /builddir/build/BUILDROOT/lpf-skype-4.2.0.11-2.fc19.x86_64
+ mkdir -p /builddir/build/BUILDROOT
+ mkdir /builddir/build/BUILDROOT/lpf-skype-4.2.0.11-2.fc19.x86_64
+ cd lpf-skype-4.2.0.11
+ /usr/share/lpf/scripts/lpf-setup-pkg /builddir/build/BUILDROOT/lpf-skype-4.2.0.11-2.fc19.x86_64 /builddir/build/SOURCES/skype.spec.in
+ desktop-file-validate /builddir/build/BUILDROOT/lpf-skype-4.2.0.11-2.fc19.x86_64/usr/share/applications/lpf-skype.desktop
/builddir/build/BUILDROOT/lpf-skype-4.2.0.11-2.fc19.x86_64/usr/share/applications/lpf-skype.desktop: file does not exist
RPM build errors:
error: Bad exit status from /var/tmp/rpm-tmp.Sl7rWT (%install)

My bad, I did not run it in mock. BBL.

README installed in wrong section. Fixed, now builds in mock/rawhide for me. 

Updated in-place, same links & release, changelog modified.

(In reply to Alec Leamas from comment #7)
> Fixed, now builds in mock/rawhide for me. 

Still not able to build in mock, the desktop part has not changed.

Source0:        skype.spec.in
Source1:        README

%install
# lpf-setup-pkg [eula] <topdir> <specfile> [sources...]
/usr/share/lpf/scripts/lpf-setup-pkg %{buildroot} %{SOURCE0}
desktop-file-validate %{buildroot}%{_datadir}/applications/%{name}.desktop
cp %{SOURCE1} README

Where should the desktop file come from?

I don't really get it. It builds for me in mock on two different machines, and thus it should for you. You are using the rawhide buildroot i. e., mock -r fedora-rawhide-i386 ... ?

lpf is not available in fc19 nor f20 as of now, at least for me.

The desktop file is created by lpf-setup-pkg.

Easiest proof would be to do a rawhide koji scratch build.

While we're at it, why isn't lpf in f20 yet?

I've postponed all lpf pushes: https://fedorahosted.org/fpc/ticket/362
(In reply to Rex Dieter from comment #10)
Welcome onboard!

> Easiest proof would be to do a rawhide koji scratch build.
Indeed: http://koji.fedoraproject.org/koji/watchlogs?taskID=6134863

> While we're at it, why isn't lpf in f20 yet?
I' ve postponed all lpf pushes: https://fedorahosted.org/fpc/ticket/362

OK, figured out. It happens only in f19 mock:

+ /usr/share/lpf/scripts/lpf-setup-pkg /builddir/build/BUILDROOT/lpf-skype-4.2.0.11-2.fc19.x86_64 /builddir/build/SOURCES/skype.spec.in
+ desktop-file-validate /builddir/build/BUILDROOT/lpf-skype-4.2.0.11-2.fc19.x86_64/usr/share/applications/lpf-skype.desktop
/builddir/build/BUILDROOT/lpf-skype-4.2.0.11-2.fc19.x86_64/usr/share/applications/lpf-skype.desktop: file does not exist
RPM build errors:
error: Bad exit status from /var/tmp/rpm-tmp.fKPQrS (%install)

Rebuilding lpf and lfp-skype in rawhide now.

..because as it was suggested in the lpf-spotify-client review I was still using the latest lpf package in the review which is quite old (lpf-0-3.46ae0c3).

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed

Issues:
=======
- Package does not contain duplicates in %files.
  Note: warning: File listed twice: /var/lib/lpf/packages/skype/state
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles

===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[-]: 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]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. No licenses
     found. Please check the source files for licenses manually.
[x]: Package requires other packages for directories it uses.
     Note: No known owner of /var/lib/lpf/packages, /usr/share/lpf,
     /var/lib/lpf, /usr/share/lpf/packages
[-]: Package must own all directories that it creates.
     Note: Directories without known owners: /var/lib/lpf,
     /var/lib/lpf/packages, /usr/share/lpf, /usr/share/lpf/packages
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or desktop-
     file-validate if there is such a file.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[!]: 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]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[!]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[-]: Packages should try to preserve timestamps of original installed files.
[x]: Spec use %global instead of %define unless justified.
     Note: %define requiring justification: %define target_pkg %(t=%{name};
     echo ${t#lpf-})
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.

===== EXTRA items =====

Generic:
[!]: Spec file according to URL is the same as in SRPM.
     Note: Spec file as given by url is not the same as in SRPM (see attached
     diff).
     See: (this test has no URL)
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.

Rpmlint
-------
Checking: lpf-skype-4.2.0.11-2.fc21.noarch.rpm
          lpf-skype-4.2.0.11-2.fc21.src.rpm
lpf-skype.noarch: W: spelling-error %description -l en_US redistributable -> redistribute, redistribution, attributable
lpf-skype.noarch: W: invalid-url URL: http://leamas.fedorapeople.org/skype/4.2.0.11/skype.spec HTTP Error 404: Not Found
lpf-skype.noarch: W: non-standard-uid /var/lib/lpf/packages/skype/state pkg-build
lpf-skype.noarch: W: non-standard-gid /var/lib/lpf/packages/skype/state pkg-build
lpf-skype.noarch: W: non-standard-uid /var/lib/lpf/packages/skype pkg-build
lpf-skype.noarch: W: non-standard-gid /var/lib/lpf/packages/skype pkg-build
lpf-skype.noarch: E: non-standard-dir-perm /var/lib/lpf/packages/skype 0775L
0 packages and 0 specfiles checked; 1 errors, 6 warnings.

Rpmlint (installed packages)
----------------------------
# rpmlint lpf-skype
lpf-skype.noarch: W: spelling-error %description -l en_US redistributable -> redistribute, redistribution, attributable
lpf-skype.noarch: W: invalid-url URL: http://leamas.fedorapeople.org/skype/4.2.0.11/skype.spec HTTP Error 404: Not Found
lpf-skype.noarch: W: non-standard-uid /var/lib/lpf/packages/skype/state pkg-build
lpf-skype.noarch: W: non-standard-gid /var/lib/lpf/packages/skype/state pkg-build
lpf-skype.noarch: W: non-standard-uid /var/lib/lpf/packages/skype pkg-build
lpf-skype.noarch: W: non-standard-gid /var/lib/lpf/packages/skype pkg-build
lpf-skype.noarch: E: non-standard-dir-perm /var/lib/lpf/packages/skype 0775L
1 packages and 0 specfiles checked; 1 errors, 6 warnings.
# echo 'rpmlint-done:'

Diff spec file in url and in SRPM
---------------------------------
--- /home/slaanesh/Documents/fedora/1023714-lpf-skype/srpm/lpf-skype.spec	2013-11-04 10:13:03.342996378 +0100
+++ /home/slaanesh/Documents/fedora/1023714-lpf-skype/srpm-unpacked/lpf-skype.spec	2013-11-01 12:16:04.000000000 +0100
@@ -8,5 +8,5 @@
 
 License:        MIT
-URL:            https://github.com/leamas/lpf
+URL:            http://leamas.fedorapeople.org/skype/4.2.0.11/skype.spec
 Group:          Development/Tools
 BuildArch:      noarch
@@ -26,4 +26,5 @@
 %prep
 %setup -cT
+cp %{SOURCE1} README
 
 
@@ -35,5 +36,4 @@
 /usr/share/lpf/scripts/lpf-setup-pkg %{buildroot} %{SOURCE0}
 desktop-file-validate %{buildroot}%{_datadir}/applications/%{name}.desktop
-cp %{SOURCE1} README
 
 
@@ -55,8 +55,6 @@
 %changelog
 * Fri Nov 1 2013 Alec Leamas <leamas> - 4.2.0.11-2
-- Adding README.
-- Setting upstream to lpf's.
+- Adding README
 - Fix typo.
-- Fix silly README error.
 
 * Sun May 05 2013 Alec Leamas <leamas> - 4.2.0.11-1


Requires
--------
lpf-skype (rpmlib, GLIBC filtered):
    /bin/sh
    lpf

Provides
--------
lpf-skype:
    application()
    application(lpf-skype.desktop)
    lpf-skype

Generated by fedora-review 0.5.0 (920221d) last change: 2013-08-30
Command line :/usr/bin/fedora-review --mock-config fedora-rawhide-x86_64 -b 1023714
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api
Disabled plugins: Java, C/C++, Python, SugarActivity, Perl, R, PHP, Ruby
Disabled flags: EPEL5, EXARCH, DISTTAG

Issues:

> - Package does not contain duplicates in %files.
>  Note: warning: File listed twice: /var/lib/lpf/packages/skype/state
>  See: http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles

Couple of issues in the %files section:

%{_datadir}/lpf/packages/%{target_pkg}
%attr(775,pkg-build,pkg-build) /var/lib/lpf/packages/%{target_pkg}
%attr(664,pkg-build,pkg-build) /var/lib/lpf/packages/%{target_pkg}/state

- First line is redundant with the second one.
- Third line is redundant with the second one; second one already includes the path. If you want to own only the directory but not the files contained in it please use %dir.

> [!]: If the source package does not include license text(s) as a separate file
>     from upstream, the packager SHOULD query upstream to include it.
> [!]: Package does not include license text files separate from upstream.
> [!]: Package does not include license text files separate from upstream.

The license is MIT, but there's no license file installed. Please provide one in %doc.

> lpf-skype.noarch: W: invalid-url URL: http://leamas.fedorapeople.org/skype/4.2.0.11/skype.spec HTTP Error 404: Not Found

Please fix.

Almost the same issues as in comment number 18 of the lfp-spotify-client review:

https://bugzilla.redhat.com/show_bug.cgi?id=973069#c18

> Diff spec file in url and in SRPM

Please re-upload spec file or srpm file to match.

After these fixes I will review the "internal" spec file.

Thanks.
--Simone
---------------------------------

(In reply to Simone Caronni from comment #15)
> Issues:

> Couple of issues in the %files section:
> 
> %{_datadir}/lpf/packages/%{target_pkg}
> %attr(775,pkg-build,pkg-build) /var/lib/lpf/packages/%{target_pkg}
> %attr(664,pkg-build,pkg-build) /var/lib/lpf/packages/%{target_pkg}/state
> 
> - First line is redundant with the second one.
No. Just removing the second line -> different (and wrong) permissions.

> - Third line is redundant with the second one;
No. Just removing the third line -> different (and wrong) permissions.

>  second one already includes
> the path. If you want to own only the directory but not the files contained
> in it please use %dir.
What I do in the third line is to give the state file 664 permissions. IMHO, using %dir would be less clear, it's not obvious in what way lpf-setup-pkg populates this dir.

Added some %exclude to get rid of the warnings. Not really happy with this solution, though.

> The license is MIT, but there's no license file installed. Please provide
> one in %doc.
Done.

> > lpf-skype.noarch: W: invalid-url URL: http://leamas.fedorapeople.org/skype/4.2.0.11/skype.spec HTTP Error 404: Not Found
> 
> Please fix.
Setting upstream to lpf upstream, fixed.
 
> Almost the same issues as in comment number 18 of the lfp-spotify-client
> review:
And almost the same reply :)

> > Diff spec file in url and in SRPM
> 
> Please re-upload spec file or srpm file to match.
Done.

New links:
srpm: http://leamas.fedorapeople.org/lpf-skype/3/lpf-skype-4.2.0.11-3.fc19.src.rpm
spec: http://leamas.fedorapeople.org/lpf-skype/3/lpf-skype.spec

Package APPROVED wrt lpf-skype.spec file.

Now to the bundled skype.spec file. I used to mantain the same for a few years; would you like to take a look at:

http://slaanesh.fedorapeople.org/skype.spec

Apart from stylistical differences in the SPEC file and description/summaries the things I notice most are the following:

- Fedora 20+ needs a wrapper because it needs PULSE_LATENCY_MSEC=30; setting it in a profile under /etc/profile.d does not work.

- Requires webkitgtk%{_isa}; is not pulled automatically in by dependencies and it's needed for the first html based welcome screen, otherwise if it's not already configured, there's no way to configure it for the first time.

- ExclusiveArch should be i686 (RPMFusion style) or %{ix86}; x86_64 is not supported.

- You are missing a prelink configuration file. If you run it for a day, after prelink has run on any EPEL or Fedora system, the program gets corrupted and you need to reinstall the rpm. Prelink it's disabled in Fedora 21+, (iirc).

- I avoid stripping by using "%define __spec_install_post /usr/lib/rpm/brp-compress" at the top of the spec file, don't know what the impact is compared to your solution.

I've also added you in CC to my (obsolete, probably) Skype review in RPMFusion; just for reference.

https://bugzilla.rpmfusion.org/show_bug.cgi?id=2978

Hm... Since you actually have a working and tested spec file: would it be problem if we just used it verbatim in this package? It actually seems to be in a much better shape than current version, which havn't really been tested that well.

Sure, no worries, just add it if you wish. Actually from Apache logs I see it's quite succesful with people on all Fedora releases and running RHEL/CentOS 6:

http://negativo17.org/skype-and-skype-pidgin-plugin/

I would say we could also close for good RPMFusion's review :)

OK, done. New links:
spec: http://leamas.fedorapeople.org/lpf-skype/4/lpf-skype.spec
srpm: http://leamas.fedorapeople.org/lpf-skype/4/lpf-skype-4.2.0.11-4.fc20.src.rpm

It's kind of odd, using your spec somehow reverses our roles when I glance through it. Made a few really small changes:

* Wed Nov 6 2013 Alec Leamas <leamas>  - 4.2.0.11-7
- Importing spec into lpf-skype package
- Fix update-mime-database snippets according to known snippets.
- Add -a cp flags, preserve modification times.

The FPC have discussed the issue. The ticket (#362) is still missing a +1 vote, but I think it a safe bet to say it will be approved. This means that lpf-skype should go to rpmfusion.

In the best of all worlds I think we should co-maintain this package. You  know the skype stuff, I know lpf (which is as yet far from stable...). One way could be that you submitted this package to rpmfusion, I review it (which basically already is done) and we then co-maintain it once it's in the repo. What do you think?

(In reply to Alec Leamas from comment #21)
> In the best of all worlds I think we should co-maintain this package. You 
> know the skype stuff, I know lpf (which is as yet far from stable...).

For me it's ok, I can send you a newer skype.spec wherever there is the need.

> way could be that you submitted this package to rpmfusion, I review it
> (which basically already is done) and we then co-maintain it once it's in
> the repo. What do you think?

I agree, but would like to skip the time-consuming review for both (lpf-skype and lpf-spotify); so I will just post references to the official review in bugzilla.redhat.com so we/you can approve them.

Sounds good. Using that recipe you could formally review rpmfusion 3033, and if you make a similar request for lpf-skype I can formally review that. Those reviews should just be a reference to the already existing reviews in redhat bz, it should no take any time IMHO.

It's very bad review and for sure fpc ticket #362.

While the resolution closed/wontfix certainly is OK, the process is not. This was assigned to Simone (negativo17...), and IMHO the proper action would have been to put a comment in the bug that it should be closed due to the FPC decision. Just grabbing a ticket this way is indeed odd. Changing the review+ to review- without a try to communicate first is even more strange.

More important: this package is now part of a "a more suitable repository" to quote FPC.