Bug 1089770 - Review Request: lxcf - A LXC facility tool
Review Request: lxcf - A LXC facility tool
Status: ASSIGNED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Haïkel Guémar
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-NEEDSPONSOR
  Show dependency treegraph
 
Reported: 2014-04-21 19:47 EDT by NIWA Hideyuki
Modified: 2014-07-30 03:10 EDT (History)
3 users (show)

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


Attachments (Terms of Use)

  None (edit)
Description NIWA Hideyuki 2014-04-21 19:47:28 EDT
Spec URL: http://prdownloads.sourceforge.net/lxcfacility/SRPM/lxcf.spec
SRPM URL: http://prdownloads.sourceforge.net/lxcfacility/SRPM/lxcf-0.5-2.src.rpm
Description: 
LXCF is a LXC facility tool.
LXCF efficiently makes the instance. One-instance making is a few
minutes or less.
The LXCF instance can dynamically change the resource of CPU, MEMORY,
IO and NET. Even if the instance has stopped, the change in the resource
can be changed.
LXCF can be used by installing it on Linux on VM as well as bare metal.
LXCF can be operated stabilizing the job like a past system at a long term.
Moreover, a lot of LXCF instances are generate in a short time. If a lot
of LXCF instances become unnecessary, it is possible to delete it
collectively. Such stateless instance can be operated.
Fedora Account System Username: niwa
Comment 1 Christopher Meng 2014-04-21 20:45:14 EDT
SPEC:

1. %define name           lxcf
%define release        2
%define version        0.5

No please don't do that. It's just a waste of time, please use tags directly.

2. Prefix:                %{_prefix}
Group:                 Applications/Emulators

Remove them, no need to keep them now.

3. In %build:

make clean

Why?

4. Since this package doesn't use configure, you should ensure that cflags(%{optflags}) and ldflags(%{__global_ldflags}) are set properly, currently I think it's not build correctly(conform to the Fedora guidelines).

5. parallel build

https://fedoraproject.org/wiki/Packaging:Guidelines#Parallel_make

6. %defattr(-,root,root)

Remove it.

7. Try:

rpm -E %{_sbindir}
rpm -E %{_mandir}

8. %config(noreplace) %{_sysconfdir}/lxcf/lxcf.conf

Unowned directory:

%{_sysconfdir}/lxcf/

https://fedoraproject.org/wiki/Packaging:UnownedDirectories

9. %{_datadir}/doc/lxcf-%{version}/README
%{_datadir}/doc/lxcf-%{version}/COPYING
%{_datadir}/doc/lxcf-%{version}/ChangeLog
%{_datadir}/doc/lxcf-%{version}/AUTHORS
%{_datadir}/doc/lxcf-%{version}/example/README
%{_datadir}/doc/lxcf-%{version}/example/HA10
%{_datadir}/doc/lxcf-%{version}/example/ops-script

Since Fedora 20 the docdir is unversioned, please change.

10. Bad scriptlet syntax:

https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd

-------------------------------
If you want to step furthur, you should follow

https://fedoraproject.org/wiki/Join_the_package_collection_maintainers

step by step.
Comment 2 Michael Schwendt 2014-04-22 12:23:49 EDT
Not a full review, but one needs to start somewhere.

Consider running "fedora-review -b 1089770" to point that tool at this ticket. It evaluates the "SRPM URL:" and "Spec URL:" lines and performs many helpful checks.


> Summary:               A LXC facility tool

Even in the %summary there is enough room to expand "LXC". Ommitting the leading article improves readability.

  Summary: Linux Containers (LXC) facility tool

https://fedoraproject.org/wiki/Examples_of_good_package_summaries


> Source:                http://prdownloads.sourceforge.net/lxcfacility/source/lxcf-%{version}.tar.gz

https://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net


> Prefix:                %{_prefix}

https://fedoraproject.org/wiki/Packaging:Guidelines#Relocatable_packages


> Requires:              python
> Requires:              python-IPy
> Requires:              virt-manager

Please add comments on explicit dependencies. What exactly is needed from these packages? Files may move. And would these deps need to be arch-specific?



> %build
> make clean

Comment on unusual command invocations like this one. Why is it needed?


> %files
> %defattr(-,root,root)

https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

> %{_prefix}/sbin/lxcf
> %{_libdir}/lxcf/lxcf-init

Directory %_libdir/lxcf is not included yet.

> %{_libdir}/lxcf/lxcf-keygen
> %{_libdir}/lxcf/lxcf-rc
> %{_libdir}/lxcf/lxcf-config
> %{_libdir}/lxcf/lxcf-maintenance
> [...]

A comment in the spec file here would be beneficial, too. You list the specific file names of over 50 files. Is that really necessary? For example, do you want the build to fail if any of those files gets added/dropped/renamed in a future update? If so, a brief comment could explain that. Else, consider using '*' based wildcards to include files more conveniently.


> %doc %attr(-,root,root)

What does that line do?
Comment 3 NIWA Hideyuki 2014-04-25 03:30:41 EDT
Hi, Thank you for a detailed comment on many.  
I mended the point of all the comments. 

Here is the updated spec and SRPM.

Spec URL: http://downloads.sourceforge.net/lxcfacility/SRPM/lxcf.spec
SRPM URL: http://downloads.sourceforge.net/lxcfacility/SRPM/lxcf-0.5-2.src.rpm

Additionally, rpmlint was executed. The result is as follows. 

$ rpmlint -i lxcf-0.5-2.x86_64.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
$ rpmlint -i lxcf-0.5-2.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

I think that there are some problems still. 
My best regards.
Comment 4 NIWA Hideyuki 2014-04-27 22:11:53 EDT
Hi, Thank you very much. 
I corrected them as follows. 

-
> SPEC:
> 
> 1. %define name           lxcf
> %define release        2
> %define version        0.5
> 
> No please don't do that. It's just a waste of time, please use tags directly.
> 

I deleted them. 

> 2. Prefix:                %{_prefix}
> Group:                 Applications/Emulators
> 
> Remove them, no need to keep them now.
> 

I deleted them too.

> 3. In %build:
> 
> make clean
> 
> Why?
> 

I deleted it. 
It was unnecessary. 

> 4. Since this package doesn't use configure, you should ensure that cflags(%{optflags}) and ldflags(%{__global_ldflags}) are set properly, currently I think it's not build correctly(conform to the Fedora guidelines).
> 

The following were put in "%build". 
CFLAGS=$RPM_OPT_FLAGS ; export CFLAGS
LDFLAGS=$RPM_OPT_FLAGS ; export LDFLAGS


> 5. parallel build
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Parallel_make
> 

The following were put in "%build". 
make %{?_smp_mflags}

> 6. %defattr(-,root,root)
> 
> Remove it.
> 

Yes, it deleted it. 

> 7. Try:
> 
> rpm -E %{_sbindir}
> rpm -E %{_mandir}
> 

I executed them. 
$ rpm -E %{_sbindir}
/usr/sbin
$ rpm -E %{_mandir}
/usr/share/man

Path of "%files" was corrected as follows. 
%{_sbindir}/lxcf
%{_mandir}/man1/lxcf.1.gz


> 8. %config(noreplace) %{_sysconfdir}/lxcf/lxcf.conf
> 
> Unowned directory:
> 
> %{_sysconfdir}/lxcf/
> 
> https://fedoraproject.org/wiki/Packaging:UnownedDirectories
> 

I corrected it as follows. 
%config(noreplace) %{_sysconfdir}/lxcf/


> 9. %{_datadir}/doc/lxcf-%{version}/README
> %{_datadir}/doc/lxcf-%{version}/COPYING
> %{_datadir}/doc/lxcf-%{version}/ChangeLog
> %{_datadir}/doc/lxcf-%{version}/AUTHORS
> %{_datadir}/doc/lxcf-%{version}/example/README
> %{_datadir}/doc/lxcf-%{version}/example/HA10
> %{_datadir}/doc/lxcf-%{version}/example/ops-script
> 
> Since Fedora 20 the docdir is unversioned, please change.
> 

I corrected it as follows. 
%doc README COPYING ChangeLog AUTHORS
%doc example/README example/HA10 example/ops-script

> 10. Bad scriptlet syntax:
> 
> https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd
> 

I corrected it as follows. 
%preun   
%systemd_preun lxcf.service
%systemd_preun lxcf-sched.service

%post
mkdir -p %{_var}/log/lxcf/
pkill virt-manager
%{_libdir}/lxcf/lxcf-init
%systemd_post lxcf.service
%systemd_post lxcf-sched.service
/usr/bin/systemctl restart libvirtd.service

%postun
%systemd_postun

> -------------------------------
> If you want to step furthur, you should follow
> 
> https://fedoraproject.org/wiki/Join_the_package_collection_maintainers
> 
> step by step.

Thanks.
Comment 5 NIWA Hideyuki 2014-04-27 22:12:42 EDT
Hi, thank you for the comment. 
I corrected them as follows. 

-
> Not a full review, but one needs to start somewhere.
> 
> Consider running "fedora-review -b 1089770" to point that tool at this ticket. It evaluates the "SRPM URL:" and "Spec URL:" lines and performs many helpful checks.
> 
> 
> > Summary:               A LXC facility tool
> 
> Even in the %summary there is enough room to expand "LXC". Ommitting the leading article improves readability.
> 
>   Summary: Linux Containers (LXC) facility tool
> 
> https://fedoraproject.org/wiki/Examples_of_good_package_summaries
> 

I thought that your description was good. 
Please let me use it for the summary as it is. 
Summary:               Linux Containers (LXC) facility tool


> 
> > Source:                http://prdownloads.sourceforge.net/lxcfacility/source/lxcf-%{version}.tar.gz
> 
> https://fedoraproject.org/wiki/Packaging:SourceURL#Sourceforge.net
> 

I corrected it as follows.
Source0:               http://downloads.sourceforge.net/lxcfacility/source/%{name}-%{version}.tar.gz


> 
> > Prefix:                %{_prefix}
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Relocatable_packages
> 

Prefix was erased. 

> 
> > Requires:              python
> > Requires:              python-IPy
> > Requires:              virt-manager
> 
> Please add comments on explicit dependencies. What exactly is needed from these packages? Files may move. And would these deps need to be arch-specific?
> 

These "Require" was erased. 

> 
> 
> > %build
> > make clean
> 
> Comment on unusual command invocations like this one. Why is it needed?
> 

I erased it by thinking it was unnecessary. 

> 
> > %files
> > %defattr(-,root,root)
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
> 

I erased this. 

> > %{_prefix}/sbin/lxcf
> > %{_libdir}/lxcf/lxcf-init
> 

I corrected it as follows.
%{_sbindir}/lxcf
%{_libdir}/lxcf/  

> Directory %_libdir/lxcf is not included yet.
> 
> > %{_libdir}/lxcf/lxcf-keygen
> > %{_libdir}/lxcf/lxcf-rc
> > %{_libdir}/lxcf/lxcf-config
> > %{_libdir}/lxcf/lxcf-maintenance
> > [...]
> 
> A comment in the spec file here would be beneficial, too. You list the specific file names of over 50 files. Is that really necessary? For example, do you want the build to fail if any of those files gets added/dropped/renamed in a future update? If so, a brief comment could explain that. Else, consider using '*' based wildcards to include files more conveniently.
> 

It is as you say. 
I corrected it as follows.
%{_libdir}/lxcf/  

> 
> > %doc %attr(-,root,root)
> 
> What does that line do?

I erased it because I was unnecessary. 

Thanks.
Comment 6 NIWA Hideyuki 2014-05-03 06:08:27 EDT
Hi, Thank you for a detailed comment on many.  
I mended the point of all the comments. 

Here is the updated spec and SRPM.

Spec URL: http://downloads.sourceforge.net/lxcfacility/SRPM/lxcf.spec
SRPM URL: http://downloads.sourceforge.net/lxcfacility/SRPM/lxcf-0.5-3.src.rpm

Additionally, rpmlint was executed. The result is as follows. 

$ rpmlint -i lxcf-0.5-3.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
$ rpmlint -i lxcf-0.5-3.x86_64.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

I mend if there is problems.

My best regards.
Comment 7 Christopher Meng 2014-05-03 08:03:15 EDT
Please don't set the review flag of bugzilla if you aren't the reviewer of the relevant package.
Comment 8 NIWA Hideyuki 2014-05-04 09:31:03 EDT
(In reply to Christopher Meng from comment #7)
> Please don't set the review flag of bugzilla if you aren't the reviewer of
> the relevant package.

Sorry, because the flag that had been set when the first submitting it had disappeared, I think my mistake and have changed it. 

Do I only have to wait ?

Thanks.
Comment 9 Christopher Meng 2014-05-04 11:20:05 EDT
(In reply to NIWA Hideyuki from comment #8)
> Sorry, because the flag that had been set when the first submitting it had
> disappeared, I think my mistake and have changed it. 
> 
> Do I only have to wait ?

Maybe... ;)

Or you can find a sponsor forwardly...
Comment 10 NIWA Hideyuki 2014-06-15 06:02:30 EDT
I believe LXCF to become a strong function by Fedora. 
It becomes a sponsor and could you help LXCF?

I updated the LXCF package. 

The change point is the following. 
- Addition of snapshot function
  The snap shot of the container of LXCF can be taken.
  It is possible to use it to back up the container.
  Moreover, it is possible to use it to forward the container between servers.
  The lack package automatically installs it by LXCF. 
- Support of RHEL7
  Red Hat Enterprise Linux 7 in addition to Fedora 19 and 20 are supported.
  It is possible to use it by installing the package where Feodora and RHEL7 are the same package.

The following renewed are specifications and SRPM. 
Spec URL: http://downloads.sourceforge.net/lxcfacility/SRPM/lxcf.spec
SRPM URL: http://downloads.sourceforge.net/lxcfacility/SRPM/lxcf-0.7-1.src.rpm

rpmlint was executed. The result is as follows. 
 $ rpmlint -i lxcf-0.7-1.src.rpm 
 1 packages and 0 specfiles checked; 0 errors, 0 warnings.

 $ rpmlint -i lxcf-0.7-1.x86_64.rpm 
 1 packages and 0 specfiles checked; 0 errors, 0 warnings.

My best regards.
Comment 11 Haïkel Guémar 2014-06-24 11:54:10 EDT
Ok, to get sponsored, we need to assess your knowledge of RPM packaging, so please make two informal reviews and link them back here.
http://fedoraproject.org/PackageReviewStatus/NEW.html (Choose one that is not green colored)

Few notes:
Why are you killing virt-manager ?
It does not requires libvirt nor lxc while it relies on them
Minor fixes to the systemd requirements (check our guidelines)
Comment 12 NIWA Hideyuki 2014-06-25 04:07:15 EDT
Thank you for the comment. 
Because I was waiting for a long time, I am very glad. 

(In reply to Haïkel Guémar from comment #11)
> Ok, to get sponsored, we need to assess your knowledge of RPM packaging, so
> please make two informal reviews and link them back here.
> http://fedoraproject.org/PackageReviewStatus/NEW.html (Choose one that is
> not green colored)

I'll review other packages. 
Could you wait a little because it reviews?

> 
> Few notes:
> Why are you killing virt-manager ?
> It does not requires libvirt nor lxc while it relies on them
> Minor fixes to the systemd requirements (check our guidelines)

This is a bug, I correct it. 
libvirtd.service had been done restarted before. 
Therefore, the connection of virt-manager cut and the error was displayed. 
Because it is unnecessary now, I'll delete it. 
Moreover, I check guideline about systemd. 

Thanks.
Comment 13 NIWA Hideyuki 2014-07-07 21:34:18 EDT
I did two informal review. Could you confirm it. 

https://bugzilla.redhat.com/show_bug.cgi?id=1115846#c2
https://bugzilla.redhat.com/show_bug.cgi?id=1094013#c1

And, I renewed my LXCF. 
The following renewed are specifications and SRPM. 

Spec URL: http://downloads.sourceforge.net/lxcfacility/SRPM/lxcf.spec
SRPM URL: http://downloads.sourceforge.net/lxcfacility/SRPM/lxcf-0.8-1.fc20.src.rpm

Thanks.
Comment 14 NIWA Hideyuki 2014-07-10 04:09:22 EDT
I did two informal review. However, because one was perl, the review was done over again. 

https://bugzilla.redhat.com/show_bug.cgi?id=1094013#c1
https://bugzilla.redhat.com/show_bug.cgi?id=1106415#c2

Thanks.
Comment 15 NIWA Hideyuki 2014-07-30 03:10:04 EDT
I updated my LXCF. 
The following updated are specifications and SRPM. 

Spec URL: http://downloads.sourceforge.net/lxcfacility/SRPM/lxcf.spec
SRPM URL: http://downloads.sourceforge.net/lxcfacility/SRPM/lxcf-0.8-1.fc20.src.rpm

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