Bug 458338

(Removing NEEDSPONSOR: bug 455067)

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,-)

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

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})

(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

(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.

(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

(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...

(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

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
-------------------------------------------------------------

(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:

Is there any issue here with video codec patents? Or does this only operate on the avi container?

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.

Blocking FE-Legal until I have time to look into this properly.

(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??

Please understand that spot has a lot of work... Please be polite
and don't use the words like "shit" or so carelessly, please.

(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.

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.

cvs done.

DivFix++-0.30-3.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/DivFix++-0.30-3.fc8

DivFix++-0.30-3.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/DivFix++-0.30-3.fc9

Okay, thanks.

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.

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.