Bug 230324

rpmlint output:
[trondd@localhost result]$ rpmlint avrdude-5.3.1-1.fc6.i386.rpm 
W: avrdude conffile-without-noreplace-flag /etc/avrdude.conf

This warning is ignored since the config file is not intended to be modified by
the users.

Package is built using mock.

MUST:
=====
0 rpmlint output is:
W: avrdude conffile-without-noreplace-flag /etc/avrdude.conf
W: avrdude-debuginfo spurious-executable-perm
/usr/src/debug/avrdude-5.3.1/safemode.c
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License ok
* spec file is legible and in Am. English.
* Source matches upstream
0 Compiles and builds on devel x86_64
0 BR: ok
* No locales
* No shared libraries
* Not relocatable
* Package owns / or requires all dirs
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* no -devel package needed, no libs / .la files.
* no .desktop file required

MUST fix:
=========
* Doesn't compile, this can be fixed by removing "%{?_smp_mflags}" from the
  make command. Note that you can usually reproduce this problem yourself by
  adding "%_smp_mflags -j3" to ~/.rpmmacros . I have this even though I'm on a
  uni-processor machine.

* Missing BuildRequires: ncurses-devel readline-devel. Note that ncurses-devel
  is not really needed as readline-devel already Requires it.

* This rpmlint message:
  W: avrdude-debuginfo spurious-executable-perm
/usr/src/debug/avrdude-5.3.1/safemode.c
  Just chmod -x the file in %prep

Questions:
==========

* Can you explain a bit about how the config file is not supposed to be modified
  by end-users? Also can you add a comment to this extend above the %config line
  in the specfile?

(In reply to comment #1)
* The package builds against libusb if it finds it => Non deterministic builds

If you want to force building against libusb:
=> BuildRequires: libusb-devel

If you do NOT want to build against libusb
=> BuildConflicts: libusb-devel


* What also makes me wonder is the sources shipping a *.info but the package not
installing it ?!?


(In reply to comment #2)
> * Can you explain a bit about how the config file is not supposed to 
> be modified by end-users? 
I doubt Trond's statement. It's a system-wide configuration file, being
generated by the configure script, not a sample.
IMO, if it's a sample then it must not be located under /etc but should be
placed elsewhere (e.g. %doc)

For the moment I'd recommend to use
%configure ... --sysconfdir=%{_sysconfdir}/avrdude
And to mark it %config(noreplace)

i.e. to treat it as a system-wide config file.

Also not the source code in lexer.l: It refers to an avrdude.conf.sample which
doesn't exist (Minor upstream bug, I'd say).

(In reply to comment #2) 
> MUST fix:
> =========
> * Doesn't compile, this can be fixed by removing "%{?_smp_mflags}" from the
>   make command. Note that you can usually reproduce this problem yourself by
>   adding "%_smp_mflags -j3" to ~/.rpmmacros . I have this even though I'm on a
>   uni-processor machine.

I did get this message if I did not remove the build tree before rebuilding, but
otherwise I could not reproduce the error, and I already had "%_smp_mflags -j3"
in ~/.rpmmacros. However, I removed _smp_mflags from make, and it works just
fine now.

> * Missing BuildRequires: ncurses-devel readline-devel. Note that ncurses-devel
>   is not really needed as readline-devel already Requires it.

I did not have any problems when building the package in mock, but I added the
requirements anyway.
 
> * This rpmlint message:
>   W: avrdude-debuginfo spurious-executable-perm
> /usr/src/debug/avrdude-5.3.1/safemode.c
>   Just chmod -x the file in %prep

FIXED.

(in Reply to comment #3)
> (In reply to comment #2)
> > * Can you explain a bit about how the config file is not supposed to 
> > be modified by end-users? 
> I doubt Trond's statement. It's a system-wide configuration file, being
> generated by the configure script, not a sample.
> IMO, if it's a sample then it must not be located under /etc but should be
> placed elsewhere (e.g. %doc)
> 
> For the moment I'd recommend to use
> %configure ... --sysconfdir=%{_sysconfdir}/avrdude
> And to mark it %config(noreplace)
> 
> i.e. to treat it as a system-wide config file.

avrdude.conf is a system-wide config file - and not just a sample file - and is
usually not edited by the users. But I added the (noreplace) parameter to
%config just in case, and moved avrdude.conf to a separate folder under /etc, as
suggested.

The reason for not adding noreplace initially, was just my misunderstanding of 
what noreplace did, but
http://fedora.redhat.com/docs/drafts/rpm-guide-en/ch09s05.html#id2972655
enlightened me :)


I uploaded the new versions to the same location. No errors from rpmlint no any
of the packages.

(In reply to comment #3)
> (In reply to comment #1)
> * The package builds against libusb if it finds it => Non deterministic builds
> 
> If you want to force building against libusb:
> => BuildRequires: libusb-devel
> 

You're right I missed that in the ./configure output, I say we _want_ usb
support, so that BuildRequires: libusb-devel should be added.

Trond, you didn't do that in your new version, so can you fix that please.

> * What also makes me wonder is the sources shipping a *.info but the package not
> installing it ?!?
> 

It is, good catch. I see that there also is no %doc, see below.

> 
> (In reply to comment #2)
> > * Can you explain a bit about how the config file is not supposed to 
> > be modified by end-users? 
> I doubt Trond's statement. It's a system-wide configuration file, being
> generated by the configure script, not a sample.
> IMO, if it's a sample then it must not be located under /etc but should be
> placed elsewhere (e.g. %doc)
> 
> For the moment I'd recommend to use
> %configure ... --sysconfdir=%{_sysconfdir}/avrdude
> And to mark it %config(noreplace)
> 
> i.e. to treat it as a system-wide config file.
> 

Agreed, but why put it in a seperate subdir of /etc, do we expect avrdude to
have multiple config files, is there a guideline for this I'm not aware of. I
think this is deviating from upstream without a good reason. Making things like
the man and info page be out of sync with whats installed. Ralf can you explain
this?

New MUST FIX list:
==================
* add BuildRequires: libusb-devel
* install info page, to %install add
  install -p -m 644 doc/avrdude.info $RPM_BUILD_ROOT%{_infodir}
  and add it to %files and add the nescesarry scriptlets, see:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-47896da5fb2662d75deefeb9ba75145a398515db
* add a %doc to %files, may I suggest:
  %doc AUTHORS COPYING ChangeLog* NEWS README doc/TODO


Trond maybe its a good idea to wait with a new version until Ralf answers my
question about the avrdude.conf location.



(In reply to comment #4)
> (In reply to comment #2) 
> > * Missing BuildRequires: ncurses-devel readline-devel. Note that ncurses-devel
> >   is not really needed as readline-devel already Requires it.
> 
> I did not have any problems when building the package in mock, but I added the
> requirements anyway.
>  

Sometimes a package can build just fine without some BuildRequires, but then is
missing some functionality, thats the case here. Thats why you should always
take a good look at ./configure's output. I thought I did, but I missed the
libusb usage.

[I love mid-air collisions ... might intersect with what Hans might have responded.]

1. FWIW: I can't reproduce the -j3 issue.


2. libusb-devel handling is still missing.
When building with libusb-devel installed you see this
...
checking for usb_get_string_simple in -lusb... yes

Without you see:
...
checking for usb_get_string_simple in -lusb... no

The resulting binaries are different.


3. Unowned directory:
/etc/avrdude

(In reply to comment #6)
> [I love mid-air collisions ... might intersect with what Hans might have
responded.]
> 
> 1. FWIW: I can't reproduce the -j3 issue.
> 

I haven't tried it more then once, probably a race condition, but the compiling
of some c-file failed because an autogenerated header file wans't present at the
moment of compiling, after make exited it was present -> smp issue.

> 
> 2. libusb-devel handling is still missing.
> When building with libusb-devel installed you see this
> ...

Agreed.

> 3. Unowned directory:
> /etc/avrdude
> 

See my comment, why a seperate dir at all, why deviate from upstream on this?

(In reply to comment #5)
> New MUST FIX list:
> ==================
> * add BuildRequires: libusb-devel

These are fixed, but I haven't uploaded the new version yet.

> * install info page, to %install add
>   install -p -m 644 doc/avrdude.info $RPM_BUILD_ROOT%{_infodir}
>   and add it to %files and add the nescesarry scriptlets, see:
>
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-47896da5fb2662d75deefeb9ba75145a398515db
> * add a %doc to %files, may I suggest:
>   %doc AUTHORS COPYING ChangeLog* NEWS README doc/TODO

I have added --enable-doc to configure, so now the complete documentation is
generated (info page and html), and installed by make install.

Ownership of /etc/avrdude has also been fixed.

[Another mid-air collision ;)]

(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #1)

> > * What also makes me wonder is the sources shipping a *.info but the package not
> > installing it ?!?
> > 
> 
> It is, good catch.
The appropriate automake-magic is in doc/Makefile.am, but it's commented out,
for reasons I can only guess on - Is upstream still active? Then they might want
to clean up their sources ;)

> > (In reply to comment #2)
> > > * Can you explain a bit about how the config file is not supposed to 
> > > be modified by end-users? 
> > I doubt Trond's statement. It's a system-wide configuration file, being
> > generated by the configure script, not a sample.
> > IMO, if it's a sample then it must not be located under /etc but should be
> > placed elsewhere (e.g. %doc)
> > 
> > For the moment I'd recommend to use
> > %configure ... --sysconfdir=%{_sysconfdir}/avrdude
> > And to mark it %config(noreplace)
> > 
> > i.e. to treat it as a system-wide config file.
> > 
> 
> Agreed, but why put it in a seperate subdir of /etc,
why not?

> do we expect avrdude to
> have multiple config files, is there a guideline for this I'm not aware of. 

Nope, it' just my personal preference. For two reasons:

1. I hate package polluting /etc and I hate packages which momentary
(temporarily?) apply one single configuration file, because packages in longer
terms tend to have more than one config file.

2. In case of embedded tools, I tend to have several config files reflecting
different setups, which I tend to keep in parallel. 
I don't use avrdude, so I am not sure if this applies in this particular case.
Probably most users will apply setups in ~/ should they need customized setups.

(In reply to comment #9)
> > Agreed, but why put it in a seperate subdir of /etc,
> why not?
> 

Because its unnecessary deviating from upstream, upstreams docs / FAQ's
mailinglist advice will say look at / edit /etc/avrdude.conf and it won't be there.

Unless there are strong reasons lets not deviate from what upstream does, in
order to invoke the least element of surprise factor.

So Trond could you please drop the extra sysconfdir parameter to %configure?
and then upload the new srpm / spec, I think that we can finish the review then.

(In reply to comment #10)
> (In reply to comment #9)
> > > Agreed, but why put it in a seperate subdir of /etc,
> > why not?
> > 
> 
> Because its unnecessary deviating from upstream, upstreams docs / FAQ's
> mailinglist advice will say look at / edit /etc/avrdude.conf and it won't be
there.
Now you've lost me. Upstream lack of insight/experience as argument ?!?

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > > Agreed, but why put it in a seperate subdir of /etc,
> > > why not?
> > > 
> > 
> > Because its unnecessary deviating from upstream, upstreams docs / FAQ's
> > mailinglist advice will say look at / edit /etc/avrdude.conf and it won't be
> there.
> Now you've lost me. Upstream lack of insight/experience as argument ?!?
> 

No staying close to upstream as argument al by itself, just like we want to use
the upstream <prefix>/<target> dir for gcc cross compilers amongst other things
to not deviate from upstream. Now if upstream but the config file under /usr/etc
I would the first to say @#$% upstream and move that file to a better place, but
there is _nothing_ wrong with upstream's placing in this case, so why move it
and confuse users?

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)

> > > Because its unnecessary deviating from upstream, upstreams docs / FAQ's
> > > mailinglist advice will say look at / edit /etc/avrdude.conf and it won't be
> > there.
> > Now you've lost me. Upstream lack of insight/experience as argument ?!?
> > 
> 
> No staying close to upstream as argument al by itself,
We are supposed to be system integrators. It's our job to integrate/customize a
package in such a way it fits best (what ever this means) into a system. This
exactly is the reason why configure scripts exist and why configure scripts
supply a --sysconfdir option. It's our job to choose what we think is best.

My experience tells me /etc/<file> is a mistake, because many other packages
commited the same mistake before and regretted it.

> just like we want to use
> the upstream <prefix>/<target> dir for gcc cross compilers amongst other
> things to not deviate from upstream.

Nope, not deviating from upstream is not the point wrt. <prefix>/target

The real reasons are 
* prefix/target is an established defacto standard for > decade, predating the
FHS/LSB.
* like many other "exotic" items and details it is missing from the FHS/LSB.
* prefix/target is not a configuration item. It's hard-coded and almost
impossible to change.

I'd be more than pleased to see this changed, but ... it's simply non-trivial
and non-realistic.

> Now if upstream but the config file under /usr/etc
> I would the first to say @#$% upstream and move that file to a better place, but
> there is _nothing_ wrong with upstream's placing in this case, so why move it
> and confuse users?
Cf. above. It's our job.

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > Now if upstream but the config file under /usr/etc
> > I would the first to say @#$% upstream and move that file to a better place, but
> > there is _nothing_ wrong with upstream's placing in this case, so why move it
> > and confuse users?
> Cf. above. It's our job.

I do not have a strong opinion on wheter /etc/avrdude.conf or
/ect/avrdude/avrdude.conf is the correct path, but atm I do not think there is
any point in deviating from upstream.

However, avrdude.conf is, as I see it, not a config file. avrdude.conf contains
information on various devices supported by avrdude, and should therefore be
stored in /usr/share/avrdude/. This should, of cause, be discussed with
upstream, just wanted to hear your thoughts before sending an email.

I've been thinking this over a bit, and although I don't like deviating from
upstream when not necessary, this is not _that_ important to me. Since Ralf
feels strongly about putting it in a separate dir, put it in a separate dir
under ./etc, as Ralf suggested.

About moving it to /usr/share, I'm not in favor of that, it is a config file, it
may be changed by the end user to support strange or new devices, so lets just
leave it in /etc .

Now with that solved, can you please post a new version then we can get this
moving forward again.

(In reply to comment #15)
> I've been thinking this over a bit, and although I don't like deviating from
> upstream when not necessary, this is not _that_ important to me. Since Ralf
> feels strongly about putting it in a separate dir, put it in a separate dir
> under ./etc, as Ralf suggested.
Well, I do NOT feel strong about it. 

It has only a recommendation and I argued against you objecting my recommendation.
As usual, in such occasions, I leave the final word on things like this to the
maintainer.

Ok Trond, so it looks like you can choose between /etc/avrdude.conf and
/etc/avrdude/avrdude.conf

Either way you need todo in %prep:
sed -i 's|/usr/local/etc/avrdude.conf|<actual location>|g' doc/avrdude.info
And if you go for /etc/avrdude/avrdude.conf also:
sed -i 's|/etc/avrdude.conf|<actual location>|g' doc/avrdude.info avrdude.1

To fixup the docs

I decided to create a separate folder for it, even though it is only one file.

I have also added generation of documentation, and fixed the things that have
been mentioned in the comments. rpmlint reports no errors.

Updated files are here: ftp://open-gnss.org/pub/fedora/avrdude/

Still needs some work.

MUST FIX:
=========
* rpmlint says:
[hans@shalem ~]$ rpmlint /usr/src/redhat/SRPMS/avrdude-5.3.1-3.src.rpm
/usr/src/redhat/RPMS/x86_64/avrdude-5.3.1-3.x86_64.rpm
/usr/src/redhat/RPMS/x86_64/avrdude-debuginfo-5.3.1-3.x86_64.rpm
E: avrdude info-dir-file /usr/share/info/dir
  Wether or not this happen may depend on the build environment (I don't
  use mock with my slow internet connection). So add:
  "rm -f $RPM_BUILD_ROOT%{_infodir}/dir"
  At the end of %install, the -f ensure that this won't cause the build to
  fail when the dir file isn't generated.
  
  And change under %files: "%{_infodir}/*" to "%{_infodir}/%{name}.info"
  I would like to recommend not to use too much wildcards under %files in
  general, so that errors like this one get caught by the checking for missing
  files check.

* Since you've enabled rebuilding off the docs, using the sed line I gave you on
  avrdude.info doesn't help, instead use it on avrdude.texi

* Since you've choosen for /etc/avrdude/avrdude.conf, you must also use sed
  to search replace /etc/avrdude.conf with /etc/avrdude/avrdude.conf in both
  doc/avrdude.texi and avrdude.1, like this:
  sed -i 's|/etc/avrdude.conf|/etc/avrdude/avrdude.conf|g' doc/avrdude.texi
avrdude.1

(In reply to comment #19)
> Still needs some work.
> 
> MUST FIX:
> =========
> * rpmlint says:
> [hans@shalem ~]$ rpmlint /usr/src/redhat/SRPMS/avrdude-5.3.1-3.src.rpm
> /usr/src/redhat/RPMS/x86_64/avrdude-5.3.1-3.x86_64.rpm
> /usr/src/redhat/RPMS/x86_64/avrdude-debuginfo-5.3.1-3.x86_64.rpm
> E: avrdude info-dir-file /usr/share/info/dir
>   Wether or not this happen may depend on the build environment (I don't
>   use mock with my slow internet connection). So add:
>   "rm -f $RPM_BUILD_ROOT%{_infodir}/dir"
>   At the end of %install, the -f ensure that this won't cause the build to
>   fail when the dir file isn't generated.

I could not reproduce this error, but I added "rm [..]" anyway. Works fine now.
  
>   And change under %files: "%{_infodir}/*" to "%{_infodir}/%{name}.info"
>   I would like to recommend not to use too much wildcards under %files in
>   general, so that errors like this one get caught by the checking for missing
>   files check.

Fixed. No more wildcards :)

> * Since you've enabled rebuilding off the docs, using the sed line I gave you on
>   avrdude.info doesn't help, instead use it on avrdude.texi
> 
> * Since you've choosen for /etc/avrdude/avrdude.conf, you must also use sed
>   to search replace /etc/avrdude.conf with /etc/avrdude/avrdude.conf in both
>   doc/avrdude.texi and avrdude.1, like this:
>   sed -i 's|/etc/avrdude.conf|/etc/avrdude/avrdude.conf|g' doc/avrdude.texi
> avrdude.1

Fixed. I checked the man and info page, and they are both ok.

Updated files at: ftp://open-gnss.org/pub/fedora/avrdude (as usual...)

Looking good:

Approved by Hans de Goede

Now go get yourself a Fedora Account if you haven't done that already sign the
CLA and then request cvs-extras group member ship. One you've done that I'll be
notified by the account system that someone (you) needs a sponsor and I'll
sponsor you.

When this is completed (IOW you are a cvs-extras member) you can then ask for
CVS-branches as described here:
http://fedoraproject.org/wiki/CVSAdminProcedure

And then get your system ready to import packages and import the package as
described here:
http://fedoraproject.org/wiki/Extras/Contributors

New Package CVS Request
=======================
Package Name: avrdude
Short Description: Software for programming Atmel AVR Microcontroller
Owners: trond.danielsen
Branches: FC-5 FC-6
InitialCC: 

New Package CVS Request
=======================
Package Name: avrdude
Short Description: Software for programming Atmel AVR Microcontroller
Owners: trond.danielsen
Branches: FC-5 FC-6
InitialCC:

(sorry, forgot the fedora-cvs flag the first time)

To CVS admin, correction could you make that (iow add me to the initialCC):

New Package CVS Request
=======================
Package Name: avrdude
Short Description: Software for programming Atmel AVR Microcontroller
Owners: trond.danielsen
Branches: FC-5 FC-6
InitialCC: j.w.r.degoede

Done

p.s.

Trond, please keep me / us posted on any other cross-compil-ish packages you
submit, I don't know if I'll have the time to review all of them but I would
like to stay in the loop and I'm not on the review-list.

I most certainly will.

Have you thought more about the SIG that was discussed earlier? I could help to
create a page on the wiki, but I am not sure what permissions I need.