Bug 683974 - Review Request: xvst - Download tool for video clips
Review Request: xvst - Download tool for video clips
Status: CLOSED DEFERRED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-10 15:13 EST by Christoph Korn
Modified: 2013-10-19 10:42 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-07-28 14:13:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Christoph Korn 2011-03-10 15:13:36 EST
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 17:40:05 EST
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-12 19:56:42 EST
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 14:59:31 EDT
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 14:05:15 EDT
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 14:41:59 EDT
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 10:57:05 EDT
* Sun Mar 27 2011 Christoph Korn <christoph.korn@getdeb.net> - 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 15:04:21 EDT
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 14:13:50 EDT
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.

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