Bug 638647

It doesn't look like there has been any movement on this review request since I submitted it.  Have I missed some required data or made some other mistake?

A couple comments :
- in file section, %(_bindir)/usr/sbin/momd should be %{_sbindir}/momd
- files in /usr/share/doc/mom/examples should rather be installed in %{_docdir}/%{name}-%{version} and thus you won't need to specify it as %doc
- %doc is missing the COPYING file

trimmed rpmlint output :
mom.src:4: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 4)
mom.x86_64: E: no-binary
mom.x86_64: E: non-executable-script /usr/lib/python2.6/site-packages/mom/Collectors/GuestNetworkDaemon.py 0644L /usr/bin/env
mom.x86_64: W: service-default-enabled /etc/rc.d/init.d/momd
mom.x86_64: E: malformed-line-in-lsb-comment-block # system resources
mom.x86_64: W: incoherent-subsys /etc/rc.d/init.d/momd $prog
mom.x86_64: W: service-default-enabled /etc/rc.d/init.d/momd
mom-debuginfo.x86_64: E: empty-debuginfo-package


- please fix space/tabs issue
- package should be noarch
- service shouldn't be enabled by default
- remove broken LSB line in initscript


Also, if this is your first package, you need to be sponsored and thus this bug needs to block FE-NEEDSPONSOR.

oh and also, missing release tag in changelog. 0.2.1 should be 0.2.1-1

Thanks for your review Xavier.  I believe I have properly addressed all of your concerns.  Please find the updated files here:

http://github.com/downloads/aglitke/mom/mom.spec
http://github.com/downloads/aglitke/mom/mom-0.2.1-2.fc13.src.rpm

I'm not a sponsor, so I won't be able to approve the package, but here
are a couple more comments.

- I'd rather see the patch split into 2 patches, one for the initscript
and one for the spec. Actually, the spec part is useless, as the bundled
spec file is not used.
- The initscript part of the patch turns 2 lines into 1 which then will
span over 80 characters, please fix.
- Patches need to have an upstream reference (bug/commit/...).This is
intended to help keep track of the patches. As you're upstream and this
will probably make it in the leext release, this is not a big deal, but
I thought I'd mention it.
- The %build and %install section are not following the template. The build part should not be done from within the %install section. See /etc/rpmdevtools/spectemplate-python.spec.
- The install of the initscript could be made shorter :
install -Dp contrib/momd.init $RPM_BUILD_ROOT/%_initddir/momd
- No need to copy the LICENSE and README files to the docdir, just using
%doc LICENSE README in the %files section will be enough.
- The INSTALL file doesn't contain any useful content, it's not needed
to include it currently. This might be changed later if/when its content
evolves.
- Use macros everywhere possible. When moving the examples, replace
/usr/share/doc by %{_defaultdocdir}.
- It might be better to alter setup.py to install the doc to the proper
location rather than move them after install.
- Be consistent with macros. You use both %_initddir and
%{_defaultdocdir}. Choose one style and stick with it. The preferred
style is usually %{_sbindir}.
- No need for %{_defaultdocdir}/%{name}-%{version} in the %files
section, this will automatically be picked up.
- Adding a blank line between changelog entries improve readability.
- BuildRoot and %clean are not needed anymore with recent Fedora
releases. However, this is still needed for EL5, so keep it if you want
to also have EPEL branches. In this case, you'll also want to replace
%{_initddir} by %{_initrddir}.
- There are some useless comments in the spec (first line and in %post).
- Please consider providing a default/dummy conf file and mark it as
%config(noreplace) in the %files section.
- You might want to use %{version} and possibly %{name] macros in the
Source0 tag, so you don't have to change the version in 2 places when
upgrading to a later version.

Thank you for your review.  The latest files can be found below:

http://github.com/downloads/aglitke/mom/mom.spec
http://github.com/downloads/aglitke/mom/mom-0.2.1-3.fc13.src.rpm

In reply to comment #5)
> I'm not a sponsor, so I won't be able to approve the package, but here
> are a couple more comments.
> 
> - I'd rather see the patch split into 2 patches, one for the initscript
> and one for the spec. Actually, the spec part is useless, as the bundled
> spec file is not used.

Done.  I want to keep the spec file patch so I can track the changes that need to be committed to the MOM tree once this review process is complete.

> - The initscript part of the patch turns 2 lines into 1 which then will
> span over 80 characters, please fix.

Done.

> - Patches need to have an upstream reference (bug/commit/...).This is
> intended to help keep track of the patches. As you're upstream and this
> will probably make it in the leext release, this is not a big deal, but
> I thought I'd mention it.

Thanks.  These patches are a convenience for me so I don't have to keep changing upstream for each iteration of this review.

> - The %build and %install section are not following the template. The build
> part should not be done from within the %install section. See
> /etc/rpmdevtools/spectemplate-python.spec.

Fixed.

> - The install of the initscript could be made shorter :
> install -Dp contrib/momd.init $RPM_BUILD_ROOT/%_initddir/momd

Ok.  Thanks.

> - No need to copy the LICENSE and README files to the docdir, just using
> %doc LICENSE README in the %files section will be enough.

Got it.

> - The INSTALL file doesn't contain any useful content, it's not needed
> to include it currently. This might be changed later if/when its content
> evolves.

Are you referring to the README file?  I was hoping I could leave it included since I will be updating it soon.  I don't like the idea of simply deleting an installed file.

> - Use macros everywhere possible. When moving the examples, replace
> /usr/share/doc by %{_defaultdocdir}.

This should be consistent now.

> - It might be better to alter setup.py to install the doc to the proper
> location rather than move them after install.

Will look into this for a future upstream release.

> - BuildRoot and %clean are not needed anymore with recent Fedora
> releases. However, this is still needed for EL5, so keep it if you want
> to also have EPEL branches. In this case, you'll also want to replace
> %{_initddir} by %{_initrddir}.

I chose to go the backward-compatible route.

> - There are some useless comments in the spec (first line and in %post).

Gone now.

> - Please consider providing a default/dummy conf file and mark it as
> %config(noreplace) in the %files section.

I now duplicate one of the example config files and install it as /etc/momd.conf (which agrees with the init script).

> - You might want to use %{version} and possibly %{name] macros in the
> Source0 tag, so you don't have to change the version in 2 places when
> upgrading to a later version.

Agreed.

Thanks for answering my comments, explanations are fine to me. You're almost there, I expect this to be the last round of comments before the package is ready for a formal review.

rpmlint output :

$ rpmlint mom-0.2.1-3.fc13.noarch.rpm
mom.noarch: W: spelling-error Summary(en_US) virtualization -> actualization, visualization, conceptualization
mom.noarch: W: spelling-error %description -l en_US eg -> Eg, eh, e
mom.noarch: W: spelling-error %description -l en_US cgroups -> groups, c groups, Citigroup
mom.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/mom/Collectors/GuestNetworkDaemon.py 0644L /usr/bin/env
mom.noarch: W: no-manual-page-for-binary momd
mom.noarch: W: incoherent-subsys /etc/rc.d/init.d/momd $prog
1 packages and 0 specfiles checked; 1 errors, 5 warnings.

You can ignore spelling-error.
non-executable-script needs to be fixed, remove the useless shebang. See patch.
no-manual-page-for-binary can be fixed by adding a man page if you wish, but that's certainly not a blocker, feel free to ignore.
incoherent-subsys is a false positive, ignore.

Also, you've misunderstood me about the example conf files. I would still move them to %{_defaultdocdir}/%{name}-%{version} and remove %{_defaultdocdir}/%{name}/examples/*. As is, it would left %{_defaultdocdir}/%{name}/examples unowned anyway. See patch.

There's a typo in the %files section for the conf file. See patch.

Last thing, patches names are not correct. You should suffix them with the name of the package rather than fedora. Less chances to collide with other patches name.

Created attachment 455873 [details]
Proposed spec cleanup

Hi Xavier.  Here is the latest version including some of your updates.  But I noticed an issue with the documentation files that required me to change things a little bit:

The %doc macro erases the directory $RPM_BUILD_ROOT/%{_defaultdocdir}/%{name}-%{version} before copying README and LICENSE there so I end up losing the example files that I copy to that directory as part of the install.  Also, %doc does not appear to support a directory hierarchy so I cannot have an examples subdirectory.

I think the solution is to abandon %doc and specify the documentation files manually.  This idea is represented in this round of files:

http://github.com/downloads/aglitke/mom/mom.spec
http://github.com/downloads/aglitke/mom/mom-0.2.1-4.fc13.src.rpm

Sorry for the misleading comments with the docs, I should obviously have tested that. Your solution and the overall package looks good to me. You just need a sponsor, the formal review should be quick.
Now might be a good time to read https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group if you haven't already.
I'd be happy to help with other reviews if you have other packages to submit.

Xavier, thanks a lot for your help!  I will find a sponsor now.

Following the Licence check, the thorough reviews by Xavier and
the fact that mom would be an interesting addition to the virtualization
tools of Fedora, I would like to sponsor that package.
However I'm getting completely lost on the documentation on how to
sponsor a new contributor

"Once they've made those changes, you can approve the package and click the appropriate link to sponsor them for access."

I don't see how to "approve" a package from bugzilla, or how to click
the  "appropriate" link that I just can't see,

http://fedoraproject.org/wiki/How_to_sponsor_a_new_contributor#Sponsoring_Someone_for_Fedora_Package_Collection

Daniel

Okay, I'm not in the sponsor list, someone from
https://admin.fedoraproject.org/accounts/group/members/packager/*/sponsor?_csrf_token=54a82e5ec35198ab07b106ffa5a098e34ab720b1

need to approve you, I can't,

Daniel

Daniel, take care not to post your _csrf_token value or your session might be stolen.

New Package SCM Request
=======================
Package Name: mom
Short Description: Dynamically manage system resources on virtualization hosts
Owners: aglitke
Branches: devel
InitialCC:

It is too early to submit for a SCM module.  You must first find a sponsor (as this is your first package) and the sponsor will perform a formal review before you can make an SCM request.

BJ, I have already been sponsored by Richard Jones (rjones).  Is there a step that he omitted in approving this package?

I'm sorry, but I do not see any reference to 'rjones' in this tracker.  If he agreed to be your sponsor, he needs to formally take this review (set the fedora-review flag to '?'), and perform the review until it passes.  At that time he would set the fedora-review flag to '+' meaning it has passed review.

Please ask Richard Jones to visit this ticket, and he can take over from there.

I'm sponsoring Adam, but I wasn't aware I had to review the package
as well.

However this package is still missing a review, and consequently
there is no fedora-review+ flag.  Xavier can you complete this
review?

Ah, oops.  My mistake, I was under the impression that the sponsor did the review as well ... I guess I just assumed that because my sponsor did.  I apologize for the bad information.

Richard, you must be the reviewer if you want to be the sponsor:

https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Get_Sponsored

| Review and approval for the first package for new packagers
| must be done by registered sponsors. Subsequent reviews can be done
| by any package maintainer. Informal reviews can always be done by
| anyone interested.

https://fedoraproject.org/wiki/How_to_sponsor_a_new_contributor
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

[...]

> %files
> ...
> %{_defaultdocdir}/%{name}-%{version}/*

Notice that directory %{_defaultdocdir}/%{name}-%{version} is not included:
https://fedoraproject.org/wiki/Packaging:UnownedDirectories

Better replace the line with just:
%{_defaultdocdir}/%{name}-%{version}/

which includes the directory and its contents. The trailing slash is just for readability (to make more clear that the entry is for a directory).

Also be aware that explicitly including files in %{_defaultdocdir}/%{name}-%{version} conflicts with using %doc to include documentation files. As it's a common pitfall for packagers, I recommend you add a comment to the %files section that %doc MUST NOT be used to include further documentation files as long as %{_defaultdocdir}/%{name}-%{version} is used. With recent distribution releases there will be an error. With older ones, you would kill files without any error/warning.


> Requires:       libvirt, libvirt-python

Even if it may be obvious to insiders, please add a comment here that explains why these two Explicit Requires are necessary:
https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

Michael, thanks for your comments.  I have updated the spec file and srpm to incorporate your suggestions.

https://github.com/downloads/aglitke/mom/mom.spec
https://github.com/downloads/aglitke/mom/mom-0.2.1-5.fc14.src.rpm

Could anyone help out with the official review for this package?  Thanks!

The status of this bug is a bit unclear to me.
First are you sponsored now Adam ? If yes, then the FE-NEEDSPONSOR can be cleared and any sponsored packager can do the formal review. If not, then a sponsor needs to sponsor you and do the review, which leads to the second unclear point.
Afaiu, Richard offered to sponsor you, but declined to do the review, which is somewhat against the rule. Richard, are you willing to sponsor Adam and do the review ? If not, then please clear the review flag so another sponsor can take over.

Thanks Xavier.  I think you have interpreted the situation correctly.

Review and sponsorship are completely different things, but in
any case I cannot possibly do a review in the next month.

Hi everyone.

I really want to move this process forward and, since I also know everyone is very busy, I am prepared to sweeten the deal for any person willing to help me get this package into Fedora.  In addition to writing software, I also enjoy making beer.  So, following OSS traditions, I am offering a 6-pack of quality home-brewed beer (you pick the style) shipped to the US address of the Fedora Packager who helps me complete the New Package Process for Memory Overcommitment Manager.

Hello,

First of all, I appologie about my possible not perfect argumentation and my English skills which are even less than that ;-).

I would like to say why I think MOM (Not MOM directly, but the problematics MOM is dealing with) is very important to Fedora and very important for futures and moderns virtualization systems.

Actually there is simply not a lot of real solutions to deal efficiently with memory overcommitment. Ballooning and KSM truly working great independently, but together they are sources of many problems and configuration's complications.

There is a real need of memory overcommitment management systems and MOM is maybe not the best one, but it's working well and is available.

As fedora say in its values "Features First" and (I think) as a "new technologies innovator" being able to purpose to its user an "advanced and efficient virtualization environment" is a necessary objective. A tool like MOM and in a more general way, a working solution to the issues of memory overcommitment in virtualization can be a real and great advantages to Fedora.

Another point, today we are dealing with the "green" and the green by one of its definitions mean to use hardware efficiently and to be equipped in a well-balanced manner compared to its need. In this case, be able to deal with memory will help running as many virtual machines as possible (and keeping them working properly, qa, etc.) on a single system.

Of course there is also the question of the CPU overcommitment (which tools like niceload or simply nice gives basics workaround, but they are realy not perfect to solve that kinds of problems) still need to be solved.

And for all this reason if someone would like to help this project to grown by giving it a chance to be in the fedora world I would be truly grateful to him.

A MOM satisfied user

Seb

I will take a look soon thanks for your works

(In reply to comment #29)
> I will take a look soon thanks for your works

Jonathan, are you a sponsor ? It seems not and this bug blocks FE-NEEDSPONSOR.

Any news ?

...

I guess until a sponsor takes the review, it will not move, unless Adam has been sponsored meanwhile. Anyway, the status of this review is rather unclear, and as it is now, I feel sorry for Adam to be trapped here for so long, so I'm un-assigning it and resetting the review flag, so someone can take the review for good. Indeed, if I'm mistaken, feel free to revert my changes.

I'd be happy to officially sponsor/review.

As an aside, IMO, the "sponsor has to be the initial reviewer" should be more a guideline than a hard-and-fast rule, but meh...

thanks to all the pre-reviews...

naming: ok
license: ok
macros: ok
scriptlets: ok

SHOULD: for f16+ anyway, would be nice to include native systemd support rather than legacy init script, see also:
https://fedoraproject.org/wiki/Packaging:Systemd

otherwise, fairly simple and clean, APPROVED.


Wrt sponsorship, looks like you've already been included in packager group (sponsor rjones ), removing blocker.  feel free to poke either of us if you have questions or need help with anything.

Thanks guys for helping me to move this along.  I'll take a look at systemd and work to get something going in the next few days.

Please don't consider the systemd thing to be a review blocker.  imo, move along with the process, and get this imported into fedora git (then, work on improving things).

New Package SCM Request
=======================
Package Name: mom
Short Description: Dynamically manage system resources on virtualization hosts
Owners: aglitke
Branches: f15 f16 f17
InitialCC:

Git done (by process-git-requests).

Removed f17, as f17==devel at the moment.

Package Change Request
======================
Package Name: mom
New Branches: f17
Owners: aglitke
InitialCC:

f17==devel.  We've not branched yet.

Package Change Request
======================
Package Name: mom
New Branches: el6
Owners: aglitke
InitialCC:

Git done (by process-git-requests).

$ koji latest-pkg f18 mom
Build                                     Tag                   Built by
----------------------------------------  --------------------  ----------------
mom-0.2.2-1.fc17                          f17                   aglitke

looks like we can close this out.

Package Change Request
======================
Package Name: mom
New Branches: el7
Owners: aglitke
InitialCC:

There is no EL-7 at the moment.

Package Change Request
======================
Package Name: mom
New Branches: el7
Owners: aglitke
InitialCC:

Git done (by process-git-requests).