Bug 980851

1. Why this? > "%define _binaries_in_noarch_packages_terminate_build 0"

2. No need for "BuildRoot" tag and "%defattr(-,root,root)" and "rm -rf %{buildroot}", Unless you are packaging for EPEL, but please, when creating the branch of el6, reedit the file, try not to preserve such lines in modern packaging system.

3. For the files warned by rpmlint, are those just configs which can be replaced when updating the package? If not add noreplace

4. %setup -q is ok, no need for -n with a name which is the default as -q.

5. I haven't tested the package, but are these perl requires cannot be detected by RPM when installing:

Requires:       perl(Text::Template)
Requires:       perl(Config::IniFiles)
Requires:       perl(File::Which)
Requires:       perl(File::Slurp)
Requires:       perl(Data::Dumper)

If they can be automatically added when install xen-tools in a pure clean environment, please remove them IMO.

6. Hmm..a README under /etc, will you consider a move of it?

7. Consider the glob, I think you can shrink the size of spec. And use %{name} to replace xen-tools in %files if you like.

I'm not a sponsor, but these reviews hope can be considered.

Thanks.

> 2.

A good suggestion.  %defattr is not needed by any active distribution release anymore, because all include RPM >= 4.4. When packaging for EPEL, think twice about how often you would copy from Fedora instead of creating a more losely connected branch.


> 4.

-q is short for "quiet output", without -q %setup would list the contents of tarballs when extracting them. Dropping -q can be helpful sometimes.

For -n the default is %{name}-%{version}, and therefore it could be dropped, but if there is frequent need to package pre-/post-releases, it can be convenient to keep the -n %name-%version parameter since that makes it easy to adjust the top builddir as necessary for pre-/post-release directory names.

First of all, thanks a lot for the review!

(In reply to Christopher Meng from comment #1)
> 1. Why this? > "%define _binaries_in_noarch_packages_terminate_build 0"
> 
AhAh, here's an easy one! The reason why it is there is that I forgot to remove it! :-(

Thanks for noticing it, I'll kill it and update the spec.

> 2. No need for "BuildRoot" tag and "%defattr(-,root,root)" and "rm -rf
> %{buildroot}", Unless you are packaging for EPEL, but please, when creating
> the branch of el6, reedit the file, try not to preserve such lines in modern
> packaging system.
> 
Mmm... sorry but I'm not sure I got everything you said. I put/kept those lines in the spec file because I found similar ones in most of the spec files I looked at while seeking "inspiration" for writing mine. :-)

Also, I'm not sure I want to package this for EPEL yet. Anyway, if you're saying that I do not need those lines for now, and I should only re-consider adding them later, if/when I decide to package this for EPEL, then I'm fine with that and will remove them.

If, OTOH, you were saying something different, could you please excuse me and clarify?

> 3. For the files warned by rpmlint, are those just configs which can be
> replaced when updating the package? If not add noreplace
> 
Yes, I have a couple of (noreplace), for things in /etc that I want preserved across updates. Those ones it complains about are either sample, template or explanatory files that I actually _do_ want to be updated, so I feel like I should not (noreplace) them. Should I suppress the warning somehow else?

> 4. %setup -q is ok, no need for -n with a name which is the default as -q.
> 
Yes, I knew it was redundant, and it was there for the reasons Michael (Comment 2) is mentioning. However, I agree, and I'll kill it until I'll need it. Thanks again.

> 5. I haven't tested the package, but are these perl requires cannot be
> detected by RPM when installing:
> 
> Requires:       perl(Text::Template)
> Requires:       perl(Config::IniFiles)
> Requires:       perl(File::Which)
> Requires:       perl(File::Slurp)
> Requires:       perl(Data::Dumper)
> 
Oh, I see... I'll double check that and remove the unnecessary 'Requires:', thanks.

> 6. Hmm..a README under /etc, will you consider a move of it?
> 
I see what you mean. However, that files provides specific instructions on what happens to the files in /etc/xen-tools/role.d/. It's a lot like '/etc/grub.d/README'. So, yes, I can think about moving it, if that is considered preferrable, but it looks to me that it can actually live there too. I'm living it there for now, let's see what other thinks...

> 7. Consider the glob, I think you can shrink the size of spec. And use
> %{name} to replace xen-tools in %files if you like.
> 
What do you mean by "Consider the glob, I think you can shrink the size of spec"? About using %{name}, good point, I will do that.

> I'm not a sponsor, but these reviews hope can be considered.
> 
Well, I'm certainly really happy about it... Thanks a lot for doing this, revised spec and pkgs coming soon.

New version of spec file and (S)RPMs, coping with Christopher's review in Comment 1:

Spec URL: http://dariof.fedorapeople.org/SPECS/xen-tools.spec

SRPM URL:
 - F18 http://dariof.fedorapeople.org/SRPMS/xen-tools-4.3.1-2.fc18.src.rpm
 - F19 http://dariof.fedorapeople.org/SRPMS/xen-tools-4.3.1-2.fc19.src.rpm

RPM URL:
 - F18 http://dariof.fedorapeople.org/RPMS/noarch/xen-tools-4.3.1-2.fc18.noarch.rpm
 - F19 http://dariof.fedorapeople.org/RPMS/noarch/xen-tools-4.3.1-2.fc19.noarch.rpm

Koji builds:
 - F18 http://koji.fedoraproject.org/koji/taskinfo?taskID=5569759
 - F19 http://koji.fedoraproject.org/koji/taskinfo?taskID=5569758

Everything else (including rpmlint output) is unchanged.

Oh, I bumped the release number, is that ok?

Regarding Christopher's comment #5, it looks like only perl(Text::Template) and perl(File::Slurp) are automatically detected, so I removed those, but am keeping the explicit 'Requires:' tag for the other modules.

Thanks and Regards, Dario

(In reply to Michael Schwendt from comment #2)
> > 2.
> 
> A good suggestion.  %defattr is not needed by any active distribution
> release anymore, because all include RPM >= 4.4. When packaging for EPEL,
> think twice about how often you would copy from Fedora instead of creating a
> more losely connected branch.
> 
Ok, I did get rid of that in the new version I just submitted.

> > 4.
> 
> -q is short for "quiet output", without -q %setup would list the contents of
> tarballs when extracting them. Dropping -q can be helpful sometimes.
> 
> For -n the default is %{name}-%{version}, and therefore it could be dropped,
> but if there is frequent need to package pre-/post-releases, it can be
> convenient to keep the -n %name-%version parameter since that makes it easy
> to adjust the top builddir as necessary for pre-/post-release directory
> names.
>
Yeah, I sort of had it there for that reason, but I decided to kill it for now.

Thanks a lot for the comments.

(In reply to Dario Faggioli from comment #3)
> Mmm... sorry but I'm not sure I got everything you said. I put/kept those
> lines in the spec file because I found similar ones in most of the spec
> files I looked at while seeking "inspiration" for writing mine. :-)

I'm not good at teaching history, simple fact is, when RPM in some elder version, it has some rules which made people works harder, people in general should not care about where is the buildroot path, why should RPM clean after install(why it cannot clean automatically?) and so on.

> Also, I'm not sure I want to package this for EPEL yet. Anyway, if you're
> saying that I do not need those lines for now, and I should only re-consider
> adding them later, if/when I decide to package this for EPEL, then I'm fine
> with that and will remove them.

I think you _can_ package it.( CentOS just launched a project can run xen4 on EL6 machines) But RHEL use KVM as default toy and no Xen in EPEL, so I'm not sure. In another hand, from the spec I don't see any Requires has xen-related stuffs. So you can create one for EPEL. If you create a spec for EPEL, please modify again.(questions welcome)

> Yes, I have a couple of (noreplace), for things in /etc that I want
> preserved across updates. Those ones it complains about are either sample,
> template or explanatory files that I actually _do_ want to be updated, so I
> feel like I should not (noreplace) them. Should I suppress the warning
> somehow else?

If you consider that those files don't have needs for user to edit, treat rpmlint issues as false positives.

> > 7. Consider the glob, I think you can shrink the size of spec. And use
> > %{name} to replace xen-tools in %files if you like.
> > 
> What do you mean by "Consider the glob, I think you can shrink the size of
> spec"? About using %{name}, good point, I will do that.

Hmm... My initial thought is that you can use one line:

%{_mandir}/man8/*.8*

or two lines:

%{_mandir}/man8/xen*.8*
%{_mandir}/man8/xt*.8*

to replace such a tons of lines:

%{_mandir}/man8/xen-create-image.8*
%{_mandir}/man8/xen-delete-image.8*
%{_mandir}/man8/xen-resize-guest.8*
%{_mandir}/man8/xt-create-xen-config.8*
%{_mandir}/man8/xt-guess-suite-and-mirror.8*
%{_mandir}/man8/xen-create-nfs.8*
%{_mandir}/man8/xt-install-image.8*
%{_mandir}/man8/xt-customize-image.8*
%{_mandir}/man8/xen-update-image.8*
%{_mandir}/man8/xen-list-images.8*

But this is up to you, there are advantages and disadvantages.

> I put/kept those lines in the spec file because I found similar
> ones in most of the spec files

Those lines are not needed anymore, because RPM has been developed further. These sections in the Packaging Guidelines try to explain it:

 * https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
 * https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean
 * https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

https://fedoraproject.org/wiki/Category:Package_Maintainers

(In reply to Michael Schwendt from comment #7)
> > I put/kept those lines in the spec file because I found similar
> > ones in most of the spec files
> 
> Those lines are not needed anymore, because RPM has been developed further.
> These sections in the Packaging Guidelines try to explain it:
> 
>  * https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
>  * https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean
>  * https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
> 
> https://fedoraproject.org/wiki/Category:Package_Maintainers
>
Sure, and in fact I happily killed them... Anyway, thanks for the useful references! :-)

(In reply to Christopher Meng from comment #6)
> (In reply to Dario Faggioli from comment #3)
> > Also, I'm not sure I want to package this for EPEL yet. Anyway, if you're
> > saying that I do not need those lines for now, and I should only re-consider
> > adding them later, if/when I decide to package this for EPEL, then I'm fine
> > with that and will remove them.
> 
> I think you _can_ package it.( CentOS just launched a project can run xen4
> on EL6 machines) But RHEL use KVM as default toy and no Xen in EPEL, so I'm
> not sure. 
>
Yes, that's exactly why I'm not sure about that either. I'm well aware of the Xen4CentOS6 project (it was one of my colleagues and friend who did most of the work on the Xen side), but still... I guess I can double check this, in future, with the CentOS guys.

> In another hand, from the spec I don't see any Requires has
> xen-related stuffs. So you can create one for EPEL. If you create a spec for
> EPEL, please modify again.(questions welcome)
> 
Yeah, this does not really have any Xen specific dependency, it's really just a bunch of perl and bash scripts. Will see, and will modify the spec accordingly (and ask questions), if I decide to do so.

> If you consider that those files don't have needs for user to edit, treat
> rpmlint issues as false positives.
>
That is my feeling about them, yes. Thanks.
 
> Hmm... My initial thought is that you can use one line:
> 
> %{_mandir}/man8/*.8*
> 
> or two lines:
> 
> %{_mandir}/man8/xen*.8*
> %{_mandir}/man8/xt*.8*
> 
> to replace such a tons of lines:
> 
> %{_mandir}/man8/xen-create-image.8*
> %{_mandir}/man8/xen-delete-image.8*
> ...
>
Oh, ok, I see what you mean now. Well, let me think about it.

Thanks a lot again

I hope you can find a sponsor ASAP.

And please provide us new spec/SRPM.

Thanks.

(In reply to Christopher Meng from comment #10)
> I hope you can find a sponsor ASAP.
> 
I was hoping that too, but nothing has happened yet. Well, I can't right now, but I'll try to "refresh" the situation a bit next week.

> And please provide us new spec/SRPM.
> 
Sorry, 'new' with respect to what? The one submitted in Comment4 already addresses all the points you raised in your review, which is still the only one I've got, as well as Michael's comments.

Perhaps you mean 'new' wrt Fedora versions (like F20)?

> Thanks.
>
Thanks to you,
Dario

Sorry, nothing here exactly. 

Hope you can find sponsor.

Dario, package has a FTBFS due to POD failure, here an extract of the error.

cd bin; for i in *-*[!y]; do pod2man --release=4.3.1 --official --section=8 $i ../man/$i.8; done
xen-create-image around line 758: Non-ASCII character seen before =encoding in 'Stéphane'. Assuming UTF-8
POD document had syntax errors at /usr/bin/pod2man line 69.
xen-create-nfs around line 90: Non-ASCII character seen before =encoding in 'Stéphane'. Assuming UTF-8
POD document had syntax errors at /usr/bin/pod2man line 69.
xen-delete-image around line 137: Non-ASCII character seen before =encoding in 'Stéphane'. Assuming UTF-8
POD document had syntax errors at /usr/bin/pod2man line 69.
xen-list-images around line 64: Non-ASCII character seen before =encoding in 'Stéphane'. Assuming UTF-8
POD document had syntax errors at /usr/bin/pod2man line 69.
xen-update-image around line 87: Non-ASCII character seen before =encoding in 'Stéphane'. Assuming UTF-8
POD document had syntax errors at /usr/bin/pod2man line 69.
xt-create-xen-config around line 111: Non-ASCII character seen before =encoding in 'Stéphane'. Assuming UTF-8
POD document had syntax errors at /usr/bin/pod2man line 69.
xt-customize-image around line 67: Non-ASCII character seen before =encoding in 'Stéphane'. Assuming UTF-8
POD document had syntax errors at /usr/bin/pod2man line 69.
xt-guess-suite-and-mirror around line 36: Non-ASCII character seen before =encoding in 'Stéphane'. Assuming UTF-8
POD document had syntax errors at /usr/bin/pod2man line 69.
xt-install-image around line 89: Non-ASCII character seen before =encoding in 'Stéphane'. Assuming UTF-8
POD document had syntax errors at /usr/bin/pod2man line 69.
make: *** [manpages] Error 255
error: Bad exit status from /var/tmp/rpm-tmp.Ci4epT (%install)

seems that error was fixed in fbc903877a6c5a5858c5ed44a91253b364de522c

this just occurs with perl 5.18, available in F20 and rawhide

Why the package doesn't have xen-hypervisor as dependency?

Another question is that why we need Requires:       debootstrap?

(In reply to Eduardo Echeverria from comment #14)
> this just occurs with perl 5.18, available in F20 and rawhide
>
Hi, and first of all, sorry for the delay.. I've been away from this for a bit.

Thanks also for reporting the issue... I'll look into that.

Regards,
Dario

(In reply to Eduardo Echeverria from comment #15)
> Why the package doesn't have xen-hypervisor as dependency?
>
Hi, and first of all, sorry for the delay.. I've been away from this for a bit.

I thought about that, and although this is a lot about Xen, there are not specific install or runtime dependencies for that. Actually, it would be possible for someone to want to use this from somewhere where Xen is not installed (yet?)... Very uncommon, for sure, but possible.

Anyway, that is not a bit deal, I can add a 'Requires: xen' if wanted.

Thanks and Regards,
Dario

(In reply to Christopher Meng from comment #16)
> Another question is that why we need Requires:       debootstrap?
>
debootstrap is what is used to install debian-based distro in the xen images the tool is meant at creating. Without it, it really would be pointless to have it in the first place.

I honestly consider it a runtime dependency.

I can of course relax that, but really, I think it would be rather pointless.

Thanks and Regards,
Dario

(In reply to Dario Faggioli from comment #19)
> (In reply to Christopher Meng from comment #16)
> > Another question is that why we need Requires:       debootstrap?
> >
> debootstrap is what is used to install debian-based distro in the xen images
> the tool is meant at creating. Without it, it really would be pointless to
> have it in the first place.

Ok, I understand now.

(In reply to Christopher Meng from comment #20)
> (In reply to Dario Faggioli from comment #19)
> > (In reply to Christopher Meng from comment #16)
> > > Another question is that why we need Requires:       debootstrap?
> > >
> > debootstrap is what is used to install debian-based distro in the xen images
> > the tool is meant at creating. Without it, it really would be pointless to
> > have it in the first place.
> 
> Ok, I understand now.
>
I think I can add a comment there anyway.

Dairo

Can you update your package so it builds under rawhide?

No response. Closing as dead review. If you ever want to continue, please resubmit.