[Bug 226658] Merge Review: xsane

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


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


Nils Philippsen <nphilipp at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|needinfo?                   |




--- Comment #21 from Nils Philippsen <nphilipp at redhat.com>  2009-08-05 08:47:24 EDT ---
Sorry for the late response.

The xsane-0.997-3.fc12 package is building right now with the changes below.

(In reply to comment #16)
> (In reply to comment #15)
> > Thanks for your effort, but I'll not use your patch because it's not very
> > readable (better use unified diffs: diff -u) and I want to discuss some things:
> 
> Right. You could have generated an unified diff by patching a local copy of the
> spec file, though :)

Yuck ;-).

> > (In reply to comment #11)
> > > - Instead of make clean'ing in between building the different versions, I'd
> > > suggest using off-root builds.
> > 
> > Later you said you had problems with that, what were these?
> 
> The automake stuff isn't configured properly. First of all, configure reads
> some xsane. files (at least xsane.NEWS), so one needs to copy them to the build
> dir. After that things got even messier since it seemed that there were
> problems with header files etc as well.
> 
> This could of course be overcome rather simply: do a manual setup of the build
> tree by extracting the tarball twice into separate subdirs (gimp and nogimp)
> where the builds can be made separately.
> 
> This of course doubles the size of the -debuginfo package (although I'm not
> sure if debugging xsane-gimp is even possible now!).

Oh, I'd rather not have that ;-). It seems we need to fix the autofoo stuff in
the package to use off-root builds, but I guess this is "out of scope" for this
review anyway.

> > > - No need to define desktop_vendor as it is only used in two places and is not
> > > even supposed to be changed.
> > 
> > "Existing packages that use a vendor tag must continue to do so for the life of
> > the package." As this is a merge review, i.e. the package isn't new, I'll leave
> > it as it is.
> 
> Yes, that's exactly my point. As there are only two instances where the macro
> is used, I'd replace them both with "fedora".

Now I understand you ;-). Fixed.

> > > MUST: A package must own all directories that it creates or require the package
> > > that owns the directory. NEEDSWORK
> > > - Package must not own
> > >  %dir %{_datadir}/applications
> > > which is a standard system directory.
> > > - gimp package must Requires: gimp for dir ownership and not own
> > >  %dir %{_sysconfdir}/gimp
> > >  %dir %{_sysconfdir}/gimp/plugins.d
> > 
> > fixed (-gimp subpackage already requires gimp)
> 
> Only in %post and %postun phases.

Fixed.

> > > MUST: Files only listed once in %files listings. NEEDSWORK
> > > - %{_datadir}/sane, %{_datadir}/sane/xsane and
> > > %{_datadir}/sane/xsane/xsane-eula.txt are owned by both xsane and xsane-gimp.
> > > Is there a good reason for this? If xsane-gimp needs those files, it'd be wiser
> > > to let xsane own them and put a Requires: %{name} = %{version}-%{release} to
> > > xsane-gimp.
> > 
> > xsane and xsane-gimp can be installed independently, they both need the same
> > xsane-eula.txt file. I believe the rule you're hinting on means files listed
> > twice for the same package (e.g. twice in "%files gimp")
> 
> http://fedoraproject.org/wiki/Packaging/Guidelines#DuplicateFiles
> 
> "A Fedora package must not list a file more than once in the spec file's %files
> listings. If you think your package is a valid exception to this, please bring
> it to the attention of the Packaging Committee so they can improve on this
> Guideline."
> 
> Now you have xsane-eula.txt and the directories listed twice. Of course, you
> can get around this by putting xsane-eula.txt in %doc of the xsane-gimp
> package.

No, I can't as xsane-gimp won't work properly without it in that exact location
;-). Anyway, I've moved the eula and documentation to a -common subpackage
which gets pulled in by both xsane and xsane-gimp.

> > > MUST: Permissions on files must be set properly. NEEDSWORK
> > > - Use %defattr(-,root,root,-) instead of %defattr(-,root,root).
> > 
> > The latter (which I use) is functionally equivalent to the former (which isn't
> > mandatory either).
> 
> Hmm, the guideline at
>  http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions
> states:
> "Unless you have a very good reason to deviate from that, you should use
> %defattr(-,root,root,-) for all %files sections in your package."
> Please, just use the version of the guideline.  

As I read the guideline I'm already compliant, because the one and the other
mean exactly the same thing. There is no tangible benefit from substituting the
short-hand version with the long-hand one other than the latter matches
verbatim what's written in the guideline.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.