Bug 980851 - Review Request: xen-tools - a Xen VM provisioning/installation tool
Review Request: xen-tools - a Xen VM provisioning/installation tool
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2013-07-03 07:05 EDT by Dario Faggioli
Modified: 2016-02-08 08:48 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-02-08 08:48:35 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Dario Faggioli 2013-07-03 07:05:13 EDT
Spec URL: http://dariof.fedorapeople.org/SPECS/xen-tools.spec

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

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

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

Description: xen-tools is a collection of simple Perl scripts which allow you to                                                   
easily create new guest Xen domains.                                                  
                                                                                                                      
Once installed and configured you can create a new Xen instance in a                                                  
matter of minutes. Each new Xen domain will be complete with:                                                         
                                                                                                                      
 * All networking details setup, with either multiple                                                                 
   static IP addresses or DHCP.                                                                                       
 * An installation of OpenSSH.                                                                                        
 * An arbitrary set of partitions.                                                                                    
                                                                                                                      
Your new instance will be completed by having the user accounts from                                                  
your guest system copied over, and you may optionally boot the                                                        
image as soon as it has been created.

Fedora Account System Username: dariof

This is my first package and I am therefore seeking a sponsor.

Upstream is here:
http://www.xen-tools.org/
http://www.xen-tools.org/software/xen-tools/

The software is already packaged and included in Debian (and derived).

Upstream has been informed about me doing this here:
http://xen-tools.org/pipermail/xen-tools-discuss/2013-July/000983.html

rpmlint:
$ rpmlint SPECS/xen-tools.spec xen-tools-4.3.1-1.fc19.noarch.rpm 
xen-tools.noarch: W: conffile-without-noreplace-flag /etc/xen-tools/xm-nfs.tmpl
xen-tools.noarch: W: conffile-without-noreplace-flag /etc/xen-tools/role.d/README
xen-tools.noarch: W: conffile-without-noreplace-flag /etc/xen-tools/xm.tmpl
xen-tools.noarch: W: conffile-without-noreplace-flag /etc/xen-tools/partitions.d/sample-server
xen-tools.noarch: W: conffile-without-noreplace-flag /etc/bash_completion.d/xen-tools
xen-tools.noarch: W: devel-file-in-non-devel-package /usr/bin/xt-create-xen-config
1 packages and 1 specfiles checked; 0 errors, 6 warnings

xt-create-xen-config _is_not_ a devel file, it's actually an executable (one of the many perl scripts):
$ rpmls /home/dario/Downloads/xen-tools-4.3.1-1.fc19.noarch.rpm|grep xen-config
-rwxr-xr-x  /usr/bin/xt-create-xen-config
$ file /usr/bin/xt-create-xen-config 
/usr/bin/xt-create-xen-config: Perl script, UTF-8 Unicode text executable

Regarding the others, replacing them on update is actually desirable
Comment 1 Christopher Meng 2013-07-03 07:54:03 EDT
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.
Comment 2 Michael Schwendt 2013-07-03 08:03:41 EDT
> 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.
Comment 3 Dario Faggioli 2013-07-03 11:30:19 EDT
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.
Comment 4 Dario Faggioli 2013-07-03 12:43:23 EDT
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
Comment 5 Dario Faggioli 2013-07-03 12:49:37 EDT
(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.
Comment 6 Christopher Meng 2013-07-03 13:18:15 EDT
(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.
Comment 7 Michael Schwendt 2013-07-03 15:11:18 EDT
> 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
Comment 8 Dario Faggioli 2013-07-04 03:52:56 EDT
(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! :-)
Comment 9 Dario Faggioli 2013-07-04 03:59:20 EDT
(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
Comment 10 Christopher Meng 2013-07-17 01:59:54 EDT
I hope you can find a sponsor ASAP.

And please provide us new spec/SRPM.

Thanks.
Comment 11 Dario Faggioli 2013-07-26 06:13:39 EDT
(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
Comment 12 Christopher Meng 2013-07-26 07:12:33 EDT
Sorry, nothing here exactly. 

Hope you can find sponsor.
Comment 13 Eduardo Echeverria 2013-09-11 03:24:04 EDT
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
Comment 14 Eduardo Echeverria 2013-09-11 03:33:34 EDT
this just occurs with perl 5.18, available in F20 and rawhide
Comment 15 Eduardo Echeverria 2013-09-11 03:56:49 EDT
Why the package doesn't have xen-hypervisor as dependency?
Comment 16 Christopher Meng 2013-09-11 04:19:08 EDT
Another question is that why we need Requires:       debootstrap?
Comment 17 Dario Faggioli 2013-09-23 14:04:14 EDT
(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
Comment 18 Dario Faggioli 2013-09-23 14:07:46 EDT
(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
Comment 19 Dario Faggioli 2013-09-23 14:10:03 EDT
(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
Comment 20 Christopher Meng 2013-09-30 05:45:05 EDT
(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.
Comment 21 Dario Faggioli 2013-10-03 02:54:54 EDT
(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
Comment 22 Miroslav Suchý 2015-08-21 05:47:13 EDT
Can you update your package so it builds under rawhide?
Comment 23 Miroslav Suchý 2016-02-08 08:48:35 EST
No response. Closing as dead review. If you ever want to continue, please resubmit.

Note You need to log in before you can comment on or make changes to this bug.