Bug 454208

I made a big mistake

please CANCEL review request

I have to work on it

Simon, if you are sure that you want this bug to be ignored, you can "close" it,
by selecting "Resolve bug" (available below, under "Bug Status change").
However if you plan to come back with a revised spec, just leave it as it is and
submit the new version when ready.

Spec URL:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.2-2/florence.spec
SRPM URL:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.2-2/florence-0.2.2-2.fc9.src.rpm

Interesting package.

Some random notes for 0.2.2-2:

* License
  - As far as I checked the whole codes, the license tag should
    be "GPLv2+ and GFDL".

* CFLAGS
  - The following lines:
---------------------------------------------------------------
CFLAGS="${CFLAGS:-$RPM_OPT_FLAGS}"
export CFLAGS
---------------------------------------------------------------
    are redundant and should be removed (please check what
    %configure actually does by
    $ rpm --eval %configure )

* desktop-file-install usage
---------------------------------------------------------------
desktop-file-install --vendor="fedora" \
        --dir=$RPM_BUILD_ROOT%{_datadir}/applications \
        %{buildroot}/%{_datadir}/applications/%{name}.desktop
rm -f $RPM_BUILD_ROOT%{_datadir}/applications/%{name}.desktop
---------------------------------------------------------------
  - You can use --delete-original option (please check
    $ desktop-file-install --help)

* GConf scriptlets
  - For GConf scriptlets, please refer to
    http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#GConf
    %post scriptlet is missing
  - Also please add proper "Requires(pre)" or so.
  - We regards GConf schemas files as _not_ a configuration file
    so please remove %config(noreplace) attribution on GConf schemas file
    (even if rpmlint warns about it)

* sed call on %post
  - Modifying %{_sysconfdir}/gconf/schemas/%{name}.schemas (using sed)
    must not be done on scriptlets but must be done before %install finishes.

* Directory ownership issue
  - Please make it sure that all directories created when installing this
    package are correctly owned by this package.
    For example, the directory %{_datadir}/%{name}/ is not owned by any package.

  ! NOTE
    When you write
-----------------------------------------------------------------------
%files
%defattr(-,root,root,-)
%{_datadir}/%{name}/
-----------------------------------------------------------------------
    This contains the directory %{_datadir}/%{name} itself and all files/
    directories/etc under %{_datadir}/%{name}, while
-----------------------------------------------------------------------
%files
%dir %{_datadir}/%{name}/
------------------------------------------------------------------------
    contains the directory %{_datadir}/%{name} only.

* Documents
  - Please add the following files to %doc:
------------------------------------------------------------------------
COPYING-DOCS
NEWS
------------------------------------------------------------------------

BTW the description "Florence is" is redundant for Summary.

One more thing
* Timestamps
  - Please consider to use
----------------------------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
----------------------------------------------------------------------
    to keep timestamps on installed files as much as possible. This method usually
    works for recent autotool based Makefiles.

okay, i hope i got the trick.

Spec URL:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.2-3/florence.spec
SRPM URL:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.2-3/florence-0.2.2-3.fc9.src.rpm

Well,

* Scriptlets
  - Requires(preun): GConf is missing

* %configure
  - --prefix=/usr is unneeded. Please check again what %configure does
    by $ rpm --eval %configure

* Macros
  - Please use macros correctly
-------------------------------------------------------------------
sed -i "s|@LAYOUTDIR@|/usr/share/florence|"
-------------------------------------------------------------------
    /usr/share must be %{_datadir} (rpm expands this correctly)

* Directory ownership issue
  - This is not yet correctly fixed. For example the directory
    %{_datadir}/%{name}/ is not owned by any packages.

* Some issues from build.log
-------------------------------------------------------------------
   637  GCONF_DISABLE_MAKEFILE_SCHEMA_INSTALL is set, not installing schemas
   638  ./../gconf-refresh
   639  kill: usage: kill [-s sigspec | -n signum | -sigspec] pid | jobspec ...
or kill -l [sigspec]
   640  Unable to send HUP signal to gconf daemon.
   641  Configuration will not be updated until reboot.
   642  Florence may not function correctly before reboot.
-------------------------------------------------------------------
  - Calling "kill -HUP gconfd" command during build is not preferable.
    As this "gconf-refresh" script is not useful for building this
    package, please replace this script with some other command
    e.g. writing the following at %prep:
-------------------------------------------------------------------
rm -f gconf-refresh
ln -sf /bin/true gconf-refresh
-------------------------------------------------------------------

  - The other issue is
-------------------------------------------------------------------
   259  keyboard.c:388: warning: implicit declaration of function
'layoutreader_readkeyboard'
-------------------------------------------------------------------
    Would you try to remove "implicit declaration of function' warning?
    c.f.
    https://www.redhat.com/archives/fedora-devel-list/2008-March/msg02036.html

(In reply to comment #8)
>   - The other issue is
> -------------------------------------------------------------------
>    259  keyboard.c:388: warning: implicit declaration of function
> 'layoutreader_readkeyboard'
> -------------------------------------------------------------------
>     Would you try to remove "implicit declaration of function' warning?
>     c.f.
>     https://www.redhat.com/archives/fedora-devel-list/2008-March/msg02036.html

Sorry, but fixing code has to be done by UPSTREAM and not by the maintainer 
which tries to get downstream hereby. It is a warning and fine. Maybe a bug
report should be opened on upstream side's tracking tool, but he shouldn't be
forced or bugged to fix the bugs/insanities of the upstream code. Otherwise
the kernel package review will never succeed, if we're going to be downstream
and upstream in Fedora of everything.

(In reply to comment #9)
> (In reply to comment #8)
> >   - The other issue is
> > -------------------------------------------------------------------
> >    259  keyboard.c:388: warning: implicit declaration of function
> > 'layoutreader_readkeyboard'
> > -------------------------------------------------------------------
> >     Would you try to remove "implicit declaration of function' warning?
> >     c.f.
> >     https://www.redhat.com/archives/fedora-devel-list/2008-March/msg02036.html
> 
> Sorry, but fixing code has to be done by UPSTREAM and not by the maintainer 
> which tries to get downstream hereby. It is a warning and fine. 

While "I said would you 'try'?", however I must note:

GCC's 'warning'  means 'serious bug' on many cases...
Actually leaving the warnings of 'implicit declaration of function' sometimes causes
segv on the application (and this happened on my package!!) and 'leave it to
upstream,
Fedora's maintainer won't fix it' can _never_ be Fedora's policy.

ref:
https://bugzilla.redhat.com/show_bug.cgi?id=433182

Created attachment 312715 [details]
florence-0.2.2-warnings.patch

The attached patch should get rid about all compiler warnings for implicit 
declarations and unused variables. Please review and forward to upstream.

SPEC:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.2-4/florence.spec
SRPM:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.2-4/florence-0.2.2-4.fc9.src.rpm

Do I have this right now?

Well, for 0.2.2-4:

* Patch name
-------------------------------------------------------------
Patch0:         %{name}-%{version}-warnings.patch
-------------------------------------------------------------
  - For the names of patches, you usually want not to use macros
    (especially %version).
    It often happens that you don't have to modify patches even if
    the tarball is upgraded. However if you use %{version} for patches'
    names, you have to recreate patches again even in such cases.

* Modifying CFLAGS
-------------------------------------------------------------
%configure

CFLAGS="${RPM_OPT_FLAGS} -Werror-implicit-function-declaration"
export CFLAGS

make %{?_smp_mflags}
-------------------------------------------------------------
  - Well, inserting CFLAGS in this method actually does _not_ change
    CFLAGS (see the result:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=741776 )

    The correct way is either
    - to export CFLAGS _before_ %configure (also please check what
      %configure does by $ rpm --eval %configure). This method is
      preferred than the latter. Recent autotool based Makefiles/configures
      usually accepts this method.
    - to use 
      make %{?_smp_mflags} CFLAGS=XXXXXXXXX
      This method should be used when the former method does not work.

* %files entry
  A.
-------------------------------------------------------------
%dir %{_datadir}/%{name}/
%{_datadir}/%{name}/
-------------------------------------------------------------
  - The entry %{_datadir}/%{name}/ contains the directory %{_datadir}/%{name}
    itself, so the first line is not needed (and should be removed)

  B.
-------------------------------------------------------------
%{_datadir}/icons/*
%{_datadir}/icons/hicolor/*/apps/%{name}.svg
-------------------------------------------------------------
  - %{_datadir}/icons/* contains all directories under %{_datadir}/icons,
    for example /usr/share/icons/hicolor (please check the files entry
    by $rpm -ql florence).
    /usr/share/icons/hicolor and so on are already owned by 
    hicolor-icon-theme and should not be owned by this package
    (the first line must be fixed)

One more thing
* desktop files
  - Category 'Application' is deprecated and should be removed
    (you can use --remove-category option)

SPEC:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.2-5/florence.spec
SRPM:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.2-5/florence-0.2.2-5.fc9.src.rpm

Okay, then:

-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few (or no) work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

I'm in a friendly contact with upstream. :-)

SPEC:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.3-1/florence.spec
SRPM:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.3-1/florence-0.2.3-1.fc9.src.rpm

I will try to meet all your requirements.

Okay, good:

* This package itself is now good
* I noticed that you have another review request (bug 454166), which I
  am actually reviewing but I didn't notice that the submitter is you...

-----------------------------------------------------------------------
     This package (florence) is APPROVED by me
-----------------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join.
from "Install the Client Tools (Koji)".

I noticed that you have already requested packager membership so
now I am sponsoring you.

If you want to import this package into Fedora 8/9, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Removing NEEDSPONSOR.

SPEC:
http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.3-2/florence.spec
SRPM:http://packages.cassmodiah.de/fedora/florence/bug-454208/florence-0.2.3-2/florence-0.2.3-2.fc9.src.rpm

Is this still okay? 

Okay. Please follow "Join" wiki.

New Package CVS Request
=======================
Package Name: florence
Short Description: Extensible scalable on-screen virtual keyboard for GNOME
Owners: cassmodiah
Branches: F-8 F-9
InitialCC:
Cvsextras Commits: yes

cvs done.

Please close this bug as NEXTRELEASE when you rebuilt this package on
devel/F-9/F-8 and request on bodhi is done.

Closing as rebuild and request on bodhi is done.

Package Change Request
======================
Package Name: florence
New Branches: EL-6
Owners: cassmodiah

CVS done (by process-cvs-requests.py).

Package Change Request
======================
Package Name: florence
New Branches: epel7
Owners: cicku

Git done (by process-git-requests).