Bug 217275

* I think that in the comment explaining what ocstag and ocsserver
I think that you should put a reference to the README. Something along

# Can, optionaly, be defined at build time (see README)

* instead of dos2unix, you can use
sed -i 's/\r//'

And similarly you can use sed instead perl to do the simple 
substitution, like

sed -i -e 's|PATH_TO_LOG_DIRECTORY|%{_localstatedir}/log/%{name}|'
logrotate.ocsinventory-client

* I think that the README.fedora should be split in a README.fedora
and README.fedora.fr, tagged like
%lang(fr) %doc README.fedora.fr

Patrice, Thanks for the comments

Spec URL: http://remi.collet.free.fr/rpms/extras/ocsinventory-client.spec
SRPM URL:
http://remi.collet.free.fr/rpms/extras/ocsinventory-client-1.0-0.2.RC3.fc7.src.rpm

%changelog
* Sun Nov 26 2006 Remi Collet <Fedora> 1.0-0.2.RC3
- replace perl and dos2unix by sed
- split README.fedora

* %{buildroot} is used everywhere, so I guess $RPM_OPT_FLAGS 
should be changed to %{optflags}. 

* there are some unowned directory issues, %{_sysconfdir}/logrotate.d/
and %{_sysconfdir}/cron.daily/ (owned by logrotate and crontabs). There
was a thread about that issue on fedora-devel-list, I have no particular
opinion on what is the right solution.

* from looking in the script, seems like there is a missing 
Requires: perl(HTTP::Request)

> there are some unowned directory issues,

Yes. I don't have real opinion too.
logrotate and cron are installed on must system. I don't like to add an require
for them as alternatives exists and "virtual-requires" not.

Spec URL: http://remi.collet.free.fr/rpms/extras/ocsinventory-client.spec
SRPM URL:
http://remi.collet.free.fr/rpms/extras/ocsinventory-client-1.0-0.3.RC3.fc7.src.rpm

* Mon Nov 27 2006 Remi Collet <Fedora> 1.0-0.3.RC3
- $RPM_OPT_FLAGS changed to %%{optflags}. 
- add not detected Requires perl(HTTP::Request)

P.S. I will be away for the next 2 days...

I will also change 
  BuildRequires: dmidecode
  Requires: dmidecode
to :
  BuildRequires: %{_sbindir}/dmidecode
  Requires:  %{_sbindir}/dmidecode

To allow build on older version (RHEL and FC) as dmidecode is provided by
kernel-utils.

(In reply to comment #4)
> > there are some unowned directory issues,
> 
> Yes. I don't have real opinion too.
> logrotate and cron are installed on must system. I don't like to add an require
> for them as alternatives exists and "virtual-requires" not.

In that case you can add a Requires on the directories, or have
ocsinventory-client own those directories, until they get owned by
the filesystem package.

(In reply to comment #5)
> I will also change 
>   BuildRequires: dmidecode
>   Requires: dmidecode
> to :
>   BuildRequires: %{_sbindir}/dmidecode
>   Requires:  %{_sbindir}/dmidecode
> 
> To allow build on older version (RHEL and FC) as dmidecode is provided by
> kernel-utils.

Fine for me.

> In that case you can add a Requires on the directories, or have
> ocsinventory-client own those directories, until they get owned by
> the filesystem package.

Is this a MUST for the review ?
I look at some core package and don't see anything...

Some improvement to have ocsinventory-client build and work on RHEL and older FC
(i really need this at work).

Spec URL: http://remi.collet.free.fr/rpms/extras/ocsinventory-client.spec
SRPM URL:
http://remi.collet.free.fr/rpms/extras/ocsinventory-client-1.0-0.4.RC3.fc7.src.rpm

* Tue Nov 28 2006 Remi Collet <Fedora> 1.0-0.4.RC3
- requires %%{_sbindir}/dmidecode (kernel-utils in FC3) rather than dmidecode 
- requires perl(:MODULE_COMPAT) only on Fedora (not provided on RHEL3)
- patch improved (Fedora is RPM based)

(In reply to comment #7)
> > In that case you can add a Requires on the directories, or have
> > ocsinventory-client own those directories, until they get owned by
> > the filesystem package.
> 
> Is this a MUST for the review ?

Yes. I make exception for /usr/share/icons/hicolor. 

In the packaging guidelines it is at
http://fedoraproject.org/wiki/Packaging/Guidelines#head-a5931a7372c4a00065713430984fa5875513e6d4

And in that case I think it is relevant, since it materializes
whether you want to have a real dependency on logrotate/cron
or make logrotate/cron optional - which means that the package
owns the directory.

> I look at some core package and don't see anything...

Most core packages haven't gone through the guidelines and lacks the
fedora extras packages quality.

> Some improvement to have ocsinventory-client build and work on RHEL and older FC
> (i really need this at work).

With EPEL, this is indeed something acceptable or even encouraged. 
Besides in that case it doesn't really hurt legibility.

> Spec URL: http://remi.collet.free.fr/rpms/extras/ocsinventory-client.spec
> SRPM URL:
> http://remi.collet.free.fr/rpms/extras/ocsinventory-client-1.0-0.4.RC3.fc7.src.rpm
> 
> * Tue Nov 28 2006 Remi Collet <Fedora> 1.0-0.4.RC3
> - requires %%{_sbindir}/dmidecode (kernel-utils in FC3) rather than dmidecode 
> - requires perl(:MODULE_COMPAT) only on Fedora (not provided on RHEL3)
> - patch improved (Fedora is RPM based)

Apart from directory owning, seems right. Notice that I didn't 
assign the bug to myself, so a reviewer accepting unowned 
directories for those directories can step up.

Spec URL: http://remi.collet.free.fr/rpms/extras/ocsinventory-client.spec
SRPM URL:
http://remi.collet.free.fr/rpms/extras/ocsinventory-client-1.0-0.5.RC3.fc7.src.rpm

%changelog
* Wed Nov 29 2006 Remi Collet <Fedora> 1.0-0.5.RC3
- Requires %%{_sysconfdir}/logrotate.d and %%{_sysconfdir}/cron.daily
- define perl_vendorlib on non-fedora (for RHEL3)

* rpmlint gives an ignorable warning, % in post is used in date
  invocation:
W: ocsinventory-client percent-in-%post
* the tarball name cannot be used, the name chosen is relevant,
  and used in other distros.
* free software, license included
* follow packaging guidelines
* source match upstream:
34edd057f1937245d06c3515c0ff50ad  OCSNG_LINUX_AGENT_1.0RC3.tar.gz
* sane provides:
Provides: config(ocsinventory-client) = 1.0-0.5.RC3 perl(Ocsinventory::Agent)
perl(Ocsinventory::Agent::Common) perl(Ocsinventory::Agent::Option::Download)
perl(Ocsinventory::Agent::Option::Ipdiscover)
perl(Ocsinventory::Agent::Option::Update)
* buildrequires right
* %files section right


APPROVED