Bug 683974

Summary: Review Request: xvst - Download tool for video clips
Product: [Fedora] Fedora Reporter: Christoph Korn <christoph.korn>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED DEFERRED QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: fedora-package-review, mario.blaettermann, martin.gieseking, notting
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-07-28 18:13:50 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

Description Christoph Korn 2011-03-10 20:13:36 UTC
Spec URL: http://188.138.90.189/fedora/xvst.spec
SRPM URL: http://188.138.90.189/fedora/xvst-2.4.1-1.fc14.src.rpm
Description:
xVideoServiceThief (a.k.a xVST) is a tool for downloading
your favorite video clips from a lot of video websites
(currently supports 76 websites and increasing!).

To introduce myself: Hello, I am in process to try Fedora as my new main
or secondary distribution. I want to contribute to the community with this package. It is my first package and I need a sponsor. I am glad about your feedback.

Comment 1 Mario Blättermann 2011-03-12 22:40:05 UTC
rpmlint output:

$ rpmlint xvst*
xvst.src: W: spelling-error Summary(de) Downloadprogramm -> Nachladeprogramm, Downloaden, Downloade
xvst.src: W: spelling-error %description -l en_US xVideoServiceThief -> serviceableness, serviceability, interservice
xvst.src: W: spelling-error %description -l de xVideoServiceThief -> servicefreundlich, Bezahlservice
xvst.src: W: invalid-url Source1: xvst-fedora.tar.gz
xvst.src: W: invalid-url Source0: xvst-2.4.1.tar.gz
1 packages and 0 specfiles checked; 0 errors, 5 warnings.


Some initial issues:

- Please add a downloadable URL for the main source.

- Is there an upstream bugreport for the patches?

- Use either variables or macros. Replace $RPM_BUILD_ROOT with %{buildroot}

- German description:
  Bitte benutze die Höflichkeitsform. Es ist allgemein unüblich,
  den Benutzer zu duzen.


FYI, Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2907311

Comment 2 Christoph Korn 2011-03-13 00:56:42 UTC
Hi,

first thank you for taking the time to review my package.
As expected it was not all perfect for my first try but I
remember your points for future packages.

You can find the updated files here:
http://188.138.90.189/fedora/xvst-2.4.1-2.fc14.src.rpm
http://188.138.90.189/fedora/xvst.spec

I tried to include all necessary information in the spec file
and I informed upstream about open problems.

Comment 3 Christoph Korn 2011-03-16 18:59:31 UTC
I added a patch to not try to update the application on startup (same issue as already mentioned in the comment inside the spec file):
http://188.138.90.189/fedora/xvst-2.4.1-3.fc14.src.rpm
http://188.138.90.189/fedora/xvst.spec

Comment 4 Mario Blättermann 2011-03-17 18:05:15 UTC
Koji scratch build:

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

Again, the main source should be defined as a URL, not as the name of the tarball included in the source rpm only. It's no problem that the tarball unpacks to a folder named different from it. Just define a different folder name with

%setup -n xVST_2_4_1_src

Look here: http://www.rpm.org/max-rpm/s1-rpm-inside-macros.html

Comment 5 Christoph Korn 2011-03-17 18:41:59 UTC
Hi,
thanks again for the review.
I changes the source RPM accordingly (actually the -c option was required because the zip archive does not contain a top-level directory).

http://188.138.90.189/fedora/xvst-2.4.1-4.fc14.src.rpm
http://188.138.90.189/fedora/xvst.spec

Comment 6 Christoph Korn 2011-03-27 14:57:05 UTC
* Sun Mar 27 2011 Christoph Korn <christoph.korn> - 2.4.1-5
- Include the additional files seperately instead of putting them in one tarball.

http://188.138.90.189/fedora/xvst.spec
http://188.138.90.189/fedora/xvst-2.4.1-5.fc14.src.rpm

Comment 7 Martin Gieseking 2011-07-20 19:04:21 UTC
Christoph, your package looks almost fine. However, there are a few issues that need some attention:

- As the updater might pull in new files not yet present in the package from a 
  third-party website, this could be a blocker. I asked on IRC how to handle
  this. It was recommended to ask FESCo [1] whether xvst can be added to Fedora.

- The archive bundles and uses the third-party library qtsingleapplication 
  (in folder src/qtsingleapplication). This is not allowed in Fedora:
  http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

  You must remove the bundled sources in %prep and patch the build system of 
  xvst so that the Fedora packages qtsingleapplication and 
  qtsingleapplication-devel are used instead.

- There are a couple of binary files in the zip file, e.g. 
  tools/MigrationTool/Windows/1_8_2a-to-2_0a/rc/brcc32.exe. Please remove them
  in %prep. You should also ask upstream to drop them from the package.
  http://fedoraproject.org/wiki/PackagingGuidelines#No_inclusion_of_pre-built_binaries_or_libraries


[1] http://fedoraproject.org/wiki/Fedora_Engineering_Steering_Committee

Comment 8 Christoph Korn 2011-07-28 18:13:50 UTC
Currently, I do not use Fedora and cannot work on this bug.

I leave the files on my server so someone else can take them and work with them.