Bug 683974 - Review Request: xvst - Download tool for video clips
Summary: Review Request: xvst - Download tool for video clips
Keywords:
Status: CLOSED DEFERRED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-03-10 20:13 UTC by Christoph Korn
Modified: 2018-11-28 19:39 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-07-28 18:13:50 UTC
Type: ---


Attachments (Terms of Use)

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.


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