Bug 458338 - Review Request: DivFix++ - A program to repair broken AVI file streams by rebuilding index part of file
Review Request: DivFix++ - A program to repair broken AVI file streams by reb...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-07 13:20 EDT by Pavel Alexeev
Modified: 2008-10-20 18:12 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-17 00:37:06 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Pavel Alexeev 2008-08-07 13:20:35 EDT
Spec URL: http://hubbitus.net.ru/rpm/Fedora9/DivFix++/DivFix++.spec
SRPM URL: http://hubbitus.net.ru/rpm/Fedora9/DivFix++/DivFix++-0.30-0.fc9.src.rpm

Description: This program designed to repair broken AVI file streams by
rebuilding index part of file. This is very useful when trying
to preview movies which has no index part, like some files are
currently downloading from ed2k or bittorent networks.

DivFix++ has supports CLI tools, this means you can fix, preview
and delete movies automatically via script (by using argument
parameters...)

DivFix++ program code supports lots of operating system, because
it's written by cross-platform API, wxWidgets.


Rpmlint silent.
Koji build successful - http://koji.fedoraproject.org/koji/taskinfo?taskID=765146


This is my 5th (in Fedora package review, not in packaging history at all)
package and I am looking for sponsor.
Comment 1 Mamoru TASAKA 2008-08-25 03:48:20 EDT
(Removing NEEDSPONSOR: bug 455067)
Comment 2 Mamoru TASAKA 2008-10-03 09:31:07 EDT
Some notes:

* Macros
  - Defining %_prefix is not needed
  - jobs is not defined on Fedora. Also please support
    parallel make if possible:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make
  - %distname is not defined.

* SourceURL
  - must be written with full URL. For sourceforge hosted tarball,
    please refer to
    https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net

* BuildRoot
  - does not honor Fedora packagig guidelines:
    https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

* Compilation flags
  - Fedora specific compilation flags are not correctly honored.
    You can check what compilation flags are used by
    $ rpm --eval %optflags
    https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
  - Also compilation flag "-Os" supersedes "-O2" flag in %optflags so
    please remove this.

* Timestamps
  - When using "cp" or "install" commands, add "-p" option to keep
    timestamps on installed files.

* Desktop file
  - When using only basename of the icon file is used for Icon entry
    removing suffix is preferred:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files

* %clean
   - The part "[ -d %{buildroot} -a "%{buildroot}" != "" ] && "
     is not needed

* Inconsistent usage of %buildroot vs $RPM_BUILD_ROOT
  - Please choose one and not use both

* %defattr
  - We recommend to use %defattr(-,root,root,-)
Comment 3 Pavel Alexeev 2008-10-07 17:33:35 EDT
First, Mamoru Tasaka, thank you for the review.

(In reply to comment #2)
> Some notes:
> 
> * Macros
>   - Defining %_prefix is not needed
Ok.
>   - jobs is not defined on Fedora. Also please support
>     parallel make if possible:
>     https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make
Ok.
>   - %distname is not defined.
%distname replaced by %{distribution}

> * SourceURL
>   - must be written with full URL. For sourceforge hosted tarball,
>     please refer to
>     https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net
Ok.

> * BuildRoot
>   - does not honor Fedora packaging guidelines:
>     https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
Set to %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

> * Compilation flags
>   - Fedora specific compilation flags are not correctly honored.
>     You can check what compilation flags are used by
>     $ rpm --eval %optflags
>     https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
>   - Also compilation flag "-Os" supersedes "-O2" flag in %optflags so
>     please remove this.
Excuse me, but I do ot completely understand you. I'm do ot provide manually any compilation flags, so, default must be used.

> * Timestamps
>   - When using "cp" or "install" commands, add "-p" option to keep
>     timestamps on installed files.
Ok.

> * Desktop file
>   - When using only basename of the icon file is used for Icon entry
>     removing suffix is preferred:
>     https://fedoraproject.org/wiki/Packaging/Guidelines#Desktop_files
Ok, extension .png removed.
> * %clean
>    - The part "[ -d %{buildroot} -a "%{buildroot}" != "" ] && "
>      is not needed
Ok.
> * Inconsistent usage of %buildroot vs $RPM_BUILD_ROOT
>   - Please choose one and not use both
Ok.
> * %defattr
>   - We recommend to use %defattr(-,root,root,-)
Ok.

http://hubbitus.net.ru/rpm/Fedora9/DivFix++/DivFix++-0.30-1.fc9.src.rpm
Comment 4 Mamoru TASAKA 2008-10-08 14:16:49 EDT
Well,

* SourceURL
  - seems 404. The correct one is perhaps:
    http://downloads.sourceforge.net/divfixpp/%{name}_v%{version}-src.tar.bz2

* Compilation flags
(In reply to comment #3)
> > * Compilation flags
> >   - Fedora specific compilation flags are not correctly honored.
> >     You can check what compilation flags are used by
> >     $ rpm --eval %optflags
> >     https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
> >   - Also compilation flag "-Os" supersedes "-O2" flag in %optflags so
> >     please remove this.
> Excuse me, but I do ot completely understand you. I'm do ot provide manually
> any compilation flags, so, default must be used.

  - So what I am saying is that you must modify cflags what is used
    on this tarball by default so that Fedora cflags are used correctly
    (i.e. you must use %optflags manually).
    Also this tarball uses "-Os" option, which should be removed.

    One of the solution is:
--------------------------------------------------------
%prep
%setup -q -n %{name}_v%{version}
sed -i.flags -e 's|-Os||' makefile
......
......
%build
make %{?_smp_mflags} WXCONFIG=wx-config CPP="g++ %optflags"
-------------------------------------------------------

* Desktop
  - Please set Category. Currently no Category is found
    (by the way --remove-category=Application can be removed
     if you don't use Application as Category from the
     beginning)

* Macros
> >   - %distname is not defined.
> %distname replaced by %{distribution}

  - My system does not define %distribution macro. Koji seems to
    define it, however its value (string) is "Unknown" so
    this is still wrong.
    Just use "--vendor=fedora".

  ! By the way if you put macros in braces please them consistently
    for cosmetic issue (i.e. use %{__install})
Comment 5 Pavel Alexeev 2008-10-09 12:45:42 EDT
(In reply to comment #4)
> Well,
> 
> * SourceURL
>   - seems 404. The correct one is perhaps:
>     http://downloads.sourceforge.net/divfixpp/%{name}_v%{version}-src.tar.bz2
Ok, sorry.

>   - So what I am saying is that you must modify cflags what is used
>     on this tarball by default so that Fedora cflags are used correctly
>     (i.e. you must use %optflags manually).
>     Also this tarball uses "-Os" option, which should be removed.
> 
>     One of the solution is:
> --------------------------------------------------------
> %prep
> %setup -q -n %{name}_v%{version}
> sed -i.flags -e 's|-Os||' makefile
> ......
> ......
> %build
> make %{?_smp_mflags} WXCONFIG=wx-config CPP="g++ %optflags"
> -------------------------------------------------------
Oh, thank you. I'm put it into spec.

> 
> * Macros
> > >   - %distname is not defined.
> > %distname replaced by %{distribution}
> 
>   - My system does not define %distribution macro. Koji seems to
>     define it, however its value (string) is "Unknown" so
>     this is still wrong.
>     Just use "--vendor=fedora".
Hmmm. What about this http://fedoraproject.org/wiki/PackageMaintainers/Packaging_Tricks#Avoiding_using_fedora_or_redhat recommendation to opposite do NOT using names like Fedora o Redhat in spec???
And lso in this dociment I get macros %{distribution}...

>   ! By the way if you put macros in braces please them consistently
>     for cosmetic issue (i.e. use %{__install})
Ok, I did that. BTW I like better %__install then %{__install}.

http://hubbitus.net.ru/rpm/Fedora9/DivFix++/DivFix++-0.30-2.fc9.src.rpm
Comment 6 Mamoru TASAKA 2008-10-10 12:02:14 EDT

(In reply to comment #5)
> (In reply to comment #4)
> > * Macros
> > > >   - %distname is not defined.
> > > %distname replaced by %{distribution}
> > 
> >   - My system does not define %distribution macro. Koji seems to
> >     define it, however its value (string) is "Unknown" so
> >     this is still wrong.
> >     Just use "--vendor=fedora".
> Hmmm. What about this
> http://fedoraproject.org/wiki/PackageMaintainers/Packaging_Tricks#Avoiding_using_fedora_or_redhat
> recommendation to opposite do NOT using names like Fedora o Redhat in spec???

  - This item says that naming a document as "README.fedora" or so
    should be avoided, however I oppose to it because there are many
    Fedora specific packaging issue...
    Also this item says that "Of course this doesn't cover internal 
    details like spec file conditionals like %fedora or %rhel."
    So please use --vendor=fedora.

    ! Note
      Currently not a few maintainers simply remove "--vendor=foo"
      when using desktop-file-install. If you remove this completely
      I don't oppose to it

> And lso in this dociment I get macros %{distribution}...
  - But actually on my system %distribution is not defined and
    koji (Fedora build server) sets this as "Unknown"...

* Category of desktop file
  - As you create the base desktop file by yourself, you can simply
    add 
------------------------------------------------
Category=Video;
------------------------------------------------
    line between "%{__cat} > %{name}.desktop << EOF"
    and EOF lines, then remove "--add-category=Video"
    ! Note
      - Semicolon is needed at the last.
Comment 7 Pavel Alexeev 2008-10-10 14:50:55 EDT
(In reply to comment #6)
> 
> (In reply to comment #5)
> > (In reply to comment #4)
> > > * Macros
> > > > >   - %distname is not defined.
> > > > %distname replaced by %{distribution}
> > > 
> > >   - My system does not define %distribution macro. Koji seems to
> > >     define it, however its value (string) is "Unknown" so
> > >     this is still wrong.
> > >     Just use "--vendor=fedora".
> > Hmmm. What about this
> > http://fedoraproject.org/wiki/PackageMaintainers/Packaging_Tricks#Avoiding_using_fedora_or_redhat
> > recommendation to opposite do NOT using names like Fedora o Redhat in spec???
> 
>   - This item says that naming a document as "README.fedora" or so
>     should be avoided,
This document says what this names should be compleatly avoided when possible. File name is just example.

> however I oppose to it because there are many
>     Fedora specific packaging issue...
So, if you oppose, I think you should discuss about it with FESCO...

>     Also this item says that "Of course this doesn't cover internal 
>     details like spec file conditionals like %fedora or %rhel."
>     So please use --vendor=fedora.
Conditionals is conditionals, it is exactly for that, so, failing "%if %fedora" or similar is not fatal in any case even more so if it will be non Fedora build...

>     ! Note
>       Currently not a few maintainers simply remove "--vendor=foo"
>       when using desktop-file-install. If you remove this completely
>       I don't oppose to it
Ok, I do that. I think it is best way now.

> * Category of desktop file
>   - As you create the base desktop file by yourself, you can simply
>     add 
> ------------------------------------------------
> Category=Video;
> ------------------------------------------------
>     line between "%{__cat} > %{name}.desktop << EOF"
>     and EOF lines, then remove "--add-category=Video"
>     ! Note
>       - Semicolon is needed at the last.
Ok, I'm do how you say.
Is there any differences where Category mentioned?

And I tryed this, but got error:
/var/tmp/DivFix++-0.30-3.fc9-root-pasha//usr/share/applications/DivFix++.desktop: error: file contains key "Category" in group "Desktop Entry", but keys extending the format should start with "X-"
Error on file "DivFix++.desktop": Failed to validate the created desktop file
Comment 8 Mamoru TASAKA 2008-10-10 15:22:08 EDT
(In reply to comment #7)
First of all:
> > > Hmmm. What about this
> > > http://fedoraproject.org/wiki/PackageMaintainers/Packaging_Tricks#Avoiding_using_fedora_or_redhat
  - This is packaging tricks (as the title says) and not guidelines.

> So, if you oppose, I think you should discuss about it with FESCO...
  - So no need.

    On the contrary, see this:
https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage
----------------------------------------------------------------
If upstream uses <vendor_id>, leave it intact, otherwise use fedora as <vendor_id>
----------------------------------------------------------------
  But as I said before not a few maintainers simply removes --vendor=foo.

> > * Category of desktop file
> >   - As you create the base desktop file by yourself, you can simply
> >     add 
> > ------------------------------------------------
> > Category=Video;
> > ------------------------------------------------
> >     line between "%{__cat} > %{name}.desktop << EOF"
> >     and EOF lines, then remove "--add-category=Video"
> >     ! Note
> >       - Semicolon is needed at the last.
> Ok, I'm do how you say.
> Is there any differences where Category mentioned?
> 
> And I tryed this, but got error:
> /var/tmp/DivFix++-0.30-3.fc9-root-pasha//usr/share/applications/DivFix++.desktop:
> error: file contains key "Category" in group "Desktop Entry", but keys
> extending the format should start with "X-"
> Error on file "DivFix++.desktop": Failed to validate the created desktop file

- Because the correct one is "Categories=Video;", sorry...
Comment 9 Pavel Alexeev 2008-10-10 16:01:32 EDT
(In reply to comment #8)
> (In reply to comment #7)
> First of all:
> > > > Hmmm. What about this
> > > > http://fedoraproject.org/wiki/PackageMaintainers/Packaging_Tricks#Avoiding_using_fedora_or_redhat
>   - This is packaging tricks (as the title says) and not guidelines.
> 
> > So, if you oppose, I think you should discuss about it with FESCO...
>   - So no need.
> 
>     On the contrary, see this:
> https://fedoraproject.org/wiki/Packaging/Guidelines#desktop-file-install_usage
> ----------------------------------------------------------------
> If upstream uses <vendor_id>, leave it intact, otherwise use fedora as
> <vendor_id>
Very intresting. Is there trics do not follow official guidelines????

> ----------------------------------------------------------------
>   But as I said before not a few maintainers simply removes --vendor=foo.
Ok, ok, I think it is a best way. I already done it now.

> - Because the correct one is "Categories=Video;", sorry...
This works.
But yoa are not answer to:
> > Is there any differences where Category mentioned?

http://hubbitus.net.ru/rpm/Fedora9/DivFix++/DivFix++-0.30-3.fc9.src.rpm
Comment 10 Mamoru TASAKA 2008-10-11 10:51:23 EDT
Okay.

(In reply to comment #9)
 > > - Because the correct one is "Categories=Video;", sorry...
> This works.
> But yoa are not answer to:
> > > Is there any differences where Category mentioned?

- I guess if desktop-file-{install,validate} won't complain
  the place of Categories is of no importance.

One misc issue
#make %{?_smp_mflags} WXCONFIG=wx-config
- Please use %% (instead of %) in comments or %changelog to prevent
  macros from being expanded.

-------------------------------------------------------------
    This package (DivFix++) is APPROVED by mtasaka
-------------------------------------------------------------
Comment 11 Pavel Alexeev 2008-10-11 11:15:19 EDT
(In reply to comment #10)
> - I guess if desktop-file-{install,validate} won't complain
>   the place of Categories is of no importance.
Ok. I thought as much. But I was disappointed by your recommendation to move it from desktop-file-install command option into desktop file itself.

> One misc issue
> #make %{?_smp_mflags} WXCONFIG=wx-config
> - Please use %% (instead of %) in comments or %changelog to prevent
>   macros from being expanded.
Ok, my carelessness.
But in this case it is have absolutely no sense.


New Package CVS Request
=======================
Package Name: DivFix++
Short Description: A program to repair broken AVI file streams by rebuilding index part of file
Owners: hubbitus
Branches: F-8 F-9 F-10
InitialCC:
Comment 12 Kevin Fenzi 2008-10-12 22:21:52 EDT
Is there any issue here with video codec patents? Or does this only operate on the avi container?
Comment 13 Pavel Alexeev 2008-10-13 02:53:36 EDT
As it says about himself ( http://divfixpp.sourceforge.net/about.htm ):
DivFix++ is complete rewrite of "DivFix" program due it's bugs and low performance. This program designed to repair broken AVI file streams by rebuilding index part of file.

So, I think it is working only with avi-container. But I'm not shure.
Comment 14 Tom "spot" Callaway 2008-10-15 17:59:31 EDT
Blocking FE-Legal until I have time to look into this properly.
Comment 15 Pavel Alexeev 2008-10-16 03:28:24 EDT
(In reply to comment #14)
> Blocking FE-Legal until I have time to look into this properly.
Shit. Couldn't you do it early, on start of review to do not make work, which may become useless??
Comment 16 Mamoru TASAKA 2008-10-16 04:41:41 EDT
Please understand that spot has a lot of work... Please be polite
and don't use the words like "shit" or so carelessly, please.
Comment 17 Pavel Alexeev 2008-10-16 06:23:46 EDT
(In reply to comment #16)
> Please understand that spot has a lot of work...
I believe in that.
I'm do not object to legal check at all.
But it is a pity also, if it will be blocked on end of process...

> Please be polite
> and don't use the words like "shit" or so carelessly, please.
Sorry if it is offend you. It wasn't addressed to you. It wasn't addressed anybody concrete.
Comment 18 Tom "spot" Callaway 2008-10-16 13:00:08 EDT
There are no legal concerns with this code, it is not encoding or decoding, it is just shuffling around the bits in the AVI header and index.

Pavel, thanks for your patience. :)

Lifting FE-Legal.
Comment 19 Kevin Fenzi 2008-10-16 13:06:57 EDT
cvs done.
Comment 20 Fedora Update System 2008-10-16 17:51:32 EDT
DivFix++-0.30-3.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/DivFix++-0.30-3.fc8
Comment 21 Fedora Update System 2008-10-16 17:51:36 EDT
DivFix++-0.30-3.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/DivFix++-0.30-3.fc9
Comment 22 Mamoru TASAKA 2008-10-17 00:37:06 EDT
Okay, thanks.
Comment 23 Fedora Update System 2008-10-20 18:05:11 EDT
DivFix++-0.30-3.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 24 Fedora Update System 2008-10-20 18:12:04 EDT
DivFix++-0.30-3.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

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