Bug 751564 - Review Request: aeolus-audrey-agent - The Aeolus Audrey Startup Agent
Summary: Review Request: aeolus-audrey-agent - The Aeolus Audrey Startup Agent
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Jim Meyering
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-11-05 16:34 UTC by Joe Vlcek
Modified: 2016-04-26 20:53 UTC (History)
4 users (show)

Fixed In Version: aeolus-audrey-agent-0.4.0-9.fc16
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-12-13 21:54:27 UTC
Type: ---
Embargoed:
meyering: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
generate man page (2.51 KB, patch)
2011-11-08 16:34 UTC, Jim Meyering
no flags Details | Diff
tweak spec file so "make rpms" succeeds (2.54 KB, patch)
2011-11-09 08:28 UTC, Jim Meyering
no flags Details | Diff

Description Joe Vlcek 2011-11-05 16:34:54 UTC
Spec URL:
http://joev.fedorapeople.org/audrey-agent/v1/aeolus-audrey-agent.spec

SRPM URL:
http://joev.fedorapeople.org/audrey-agent/v1/aeolus-audrey-agent-0.3.1-6.fc15.src.rpm

Koji Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3491043

rpmlint output:
% rpmlint ~/rpmbuild/RPMS/noarch/aeolus-audrey-agent-0.3.1-6.fc15.noarch.rpm
aeolus-audrey-agent.noarch: E: explicit-lib-dependency python-httplib2
aeolus-audrey-agent.noarch: W: no-manual-page-for-binary audrey
1 packages and 0 specfiles checked; 1 errors, 1 warnings.

I believe the 1 rpmlint error may be erroneous as the dependency flagged has "lib"
in the name and there are other python-XYZ requires not flagged.

The lack of a manual page for audrey is addressed by an descriptive usage
message.

Description: 

I just finished packaging the Aeolus Audrey Startup Agent and I would
appreciate a review.

The Aeolus Audrey Startup Agent, a script which runs on a booting
cloud instances to retrieve configuration data from the Aeolus
Config Server.

Comment 1 Thomas Spura 2011-11-06 11:24:45 UTC
Some quick comments:
- Please use _bindir and not app_root.
- python-argparse is not available anymore. The argparse module is part of plain python.
- You have no defattr, so I'm assuming you want to build for Fedora only.
  Then you can also nuke of %clean and BuildRoot
- Please use mkdir and cp directly like rm too to be consistent.
- The first mkdir is not needed anyway, the second one creates all directories needed.
- Where come the sources from? Please add a full link.
- extra_release is not needed too. When you want to add an extra_release sometime in the future, just add a number there (or bump the relase). No need for "%global extra_release 1".

Comment 2 Jim Meyering 2011-11-06 14:28:06 UTC
Hi Joe,

I'll review this.  I haven't investigated the python-httplib2 thing yet,
but wonder if it's complaining that there's an explicit dependency
on an arch-dependent library, given that this is a "noarch" package.

Regarding the no-man-page warning, there's a nice help2man program that
can automatically generate a man page from the output of ./PROG --help.
It's worth doing, if only to save folks the hassle of running "man PROG"
and finding no manual -- even if the manual has no more information than
PROG --help.  I'll look at adding Makefile rules to generate the man
page for you (that's how the 100+ coreutils man pages are generated).

One nit in the above, in case it's also in the spec, "runs on a
... instances" -- either drop the "a" or drop the trailing "s" in
"instances".

Comment 3 Jim Meyering 2011-11-06 15:51:51 UTC
Hi again,

I've begun looking at the .py file and my first attempt to elicit --help
output failed.  I ran ./audrey_startup.py --help as non-root from the
source directory and got a stack trace followed by this:

    IOError: [Errno 13] Permission denied: '/var/log/audrey.log'

It'd be nice (at least as long as there's no man page) if there were
a --help option that would print the concise description and list brief
option descriptions.  Typically, --help prints output to stdout (not stderr)
and exits successfully without doing anything else.

Looking through the source, I see there's a --pw=PASSWORD option.
That means if one is specifying a password, it will be visible
via "ps".  It would be better to allow the password to be read
from a file or from a TTY, so that it cannot easily be snooped.

In the sources I see mention of "unittest" support.
Is there an easy way to run those unit tests?

Comment 4 Jim Meyering 2011-11-07 11:59:12 UTC
Joe,
The sole rpmlint error, that "E: explicit-lib-dependency python-httplib2",
is indeed a false positive, per this:
https://lists.fedoraproject.org/pipermail/devel/2010-March/133955.html

As to why we still have to deal with such false positives 18 months later,
my guess is that people are looking forward to python 3.0.
There, ISTR python-httplib2 is renamed not to include "lib".

Comment 5 Jim Meyering 2011-11-07 12:16:35 UTC
Joe,

Please use %{buildroot} consistently, rather than $RPM_BUILD_ROOT.

BTW, is there an "upstream" repository or release tarball?
So far I haven't found one.

Comment 6 Jim Meyering 2011-11-07 12:20:16 UTC
Actually, per the packaging guidelines, you may use either
%{buildroot} or $RPM_BUILD_ROOT, as long as you do so consistently.

Comment 7 Jim Meyering 2011-11-07 17:35:27 UTC
Joe gave me this upstream URL
  https://github.com/aeolusproject/audrey

Comment 8 Joe Vlcek 2011-11-07 18:40:18 UTC
(In reply to comment #1)
> Some quick comments:
> - Please use _bindir and not app_root.
> - python-argparse is not available anymore. The argparse module is part of
> plain python.
> - You have no defattr, so I'm assuming you want to build for Fedora only.
>   Then you can also nuke of %clean and BuildRoot
> - Please use mkdir and cp directly like rm too to be consistent.
> - The first mkdir is not needed anyway, the second one creates all directories
> needed.
> - Where come the sources from? Please add a full link.
> - extra_release is not needed too. When you want to add an extra_release
> sometime in the future, just add a number there (or bump the relase). No need
> for "%global extra_release 1".

Thank you for the comments Thomas! I appreciate the help.

Regarding:
> - python-argparse is not available anymore. The argparse module is part of
> plain python.

I do want to build for RHEL and Fedora. Don't I need to specify it for RHEL?

Comment 9 Jim Meyering 2011-11-07 18:41:40 UTC
%defattr is no longer required, per this:
  https://fedorahosted.org/fpc/ticket/77

Comment 10 Joe Vlcek 2011-11-07 20:58:36 UTC
Thank you Jim and Thomas for the review input.

I believe I've address everything except creating the man page and the
python-argpase issue, which I am waiting for a reply to my inquiry.

The latest versions of the important documents can be found here:

Please note "/v2" not "/v1"

Spec URL:
http://joev.fedorapeople.org/audrey-agent/v2/aeolus-audrey-agent.spec

SRPM URL:
http://joev.fedorapeople.org/audrey-agent/v2/aeolus-audrey-agent-0.3.1-7.fc15.src.rpm

Koji Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3495654

Comment 11 Jim Meyering 2011-11-08 12:55:53 UTC
Hi Joe,
FYI, the optparse module has been deprecated since python2.7
in favor of the new-in-2.7 argparse module.

  http://docs.python.org/library/optparse.html

Depending on which version of Python is available in other target systems, you may want to update.

Comment 12 Jim Meyering 2011-11-08 13:05:21 UTC
For the record, here's the latest --help output:

usage: audrey_startup.py [-h] [-e ENDPOINT] [-k OAUTH_KEY] [-s OAUTH_SECRET]
                         [-p] [-L {DEBUG,INFO,WARNING,ERROR,CRITICAL}]
                         [--version]

Audrey Start

optional arguments:
  -h, --help            show this help message and exit
  -e ENDPOINT, --endpoint ENDPOINT
                        Config Server endpoint url
  -k OAUTH_KEY, --key OAUTH_KEY
                        oAuth Key
  -s OAUTH_SECRET, --secret OAUTH_SECRET
                        oAuth Secret
  -p, --pwd             Log and look for configs in pwd
  -L {DEBUG,INFO,WARNING,ERROR,CRITICAL}, --log_level {DEBUG,INFO,WARNING,ERROR,CRITICAL}
                        Logging Level

Like --pw=PASSWORD before it, the oauth arguments
should not be exposed on the command line.
--------------------------------------

Also, I see two uses of a hard-coded URL:

    $ grep 169 audrey_startup.py
    EC2_USER_DATA_URL = 'http://169.254.169.254/latest/user-data'
	    self.ec2_user_data_url = 'http://169.254.169.254/latest/user-data'

Obviously the literal URL should appear only once, but the
real question is whether it should appear at all.

Shouldn't it be specified via an option?

Comment 13 Joe Vlcek 2011-11-08 13:31:41 UTC
(In reply to comment #12)
> For the record, here's the latest --help output:
> 
> usage: audrey_startup.py [-h] [-e ENDPOINT] [-k OAUTH_KEY] [-s OAUTH_SECRET]
>                          [-p] [-L {DEBUG,INFO,WARNING,ERROR,CRITICAL}]
>                          [--version]
> 
> Audrey Start
> 
> optional arguments:
>   -h, --help            show this help message and exit
>   -e ENDPOINT, --endpoint ENDPOINT
>                         Config Server endpoint url
>   -k OAUTH_KEY, --key OAUTH_KEY
>                         oAuth Key
>   -s OAUTH_SECRET, --secret OAUTH_SECRET
>                         oAuth Secret
>   -p, --pwd             Log and look for configs in pwd
>   -L {DEBUG,INFO,WARNING,ERROR,CRITICAL}, --log_level
> {DEBUG,INFO,WARNING,ERROR,CRITICAL}
>                         Logging Level
> 
> Like --pw=PASSWORD before it, the oauth arguments
> should not be exposed on the command line.
> --------------------------------------
> 
> Also, I see two uses of a hard-coded URL:
> 
>     $ grep 169 audrey_startup.py
>     EC2_USER_DATA_URL = 'http://169.254.169.254/latest/user-data'
>      self.ec2_user_data_url = 'http://169.254.169.254/latest/user-data'
> 
> Obviously the literal URL should appear only once, but the
> real question is whether it should appear at all.
> 
> Shouldn't it be specified via an option?

Thank you Jim.

A bad push merge by Greg Blomquist, resulted in the OAUTH arguments
not being prompted for. I will work with Greg to fix the merge.

The 169 URL is the documented method for accessing AWS EC2 user-data
http://docs.amazonwebservices.com/AWSEC2/latest/UserGuide/index.html?AESDG-chapter-instancedata.html

So I believe it is OK for it to be hard-coded however I agree it
should not be hard-coded twice. I will change this to:

EC2_USER_DATA_URL = 'http://169.254.169.254/latest/user-data'
self.ec2_user_data_url = EC2_USER_DATA_URL

Do you think that would be acceptable?

Comment 14 Jim Meyering 2011-11-08 15:01:20 UTC
sure.  Thanks.

Comment 15 Jim Meyering 2011-11-08 16:34:22 UTC
Created attachment 532333 [details]
generate man page

Joe,

Once you've added a --version option, apply the patch above.
Then, you'll have an automatically-generated man page.
However, please also change the minimal "Audrey Start"
description to be more like the 2-3-line description
in the RPM spec file.  Otherwise, that two-word description
gets propagated to the man page like this:


AUDREY(8)               System Administration Utilities              AUDREY(8)



NAME
       audrey - Aeolus startup agent

DESCRIPTION
       usage: audrey [-h] [-e ENDPOINT] [-k OAUTH_KEY] [-s OAUTH_SECRET] [-p]

              [-L {DEBUG,INFO,WARNING,ERROR,CRITICAL}] [--version]

       Audrey Start

   optional arguments:
       -h, --help
              show this help message and exit

       -e ENDPOINT, --endpoint ENDPOINT
              Config Server endpoint url

       -k OAUTH_KEY, --key OAUTH_KEY
              oAuth Key

       -s OAUTH_SECRET, --secret OAUTH_SECRET
              oAuth Secret

       -p, --pwd
              Log and look for configs in pwd

       -L  {DEBUG,INFO,WARNING,ERROR,CRITICAL},  --log_level {DEBUG,INFO,WARN-
       ING,ERROR,CRITICAL}
              Logging Level

       --version
              show program's version number and exit

Comment 16 Joe Vlcek 2011-11-08 21:42:41 UTC
(In reply to comment #11)
> Hi Joe,
> FYI, the optparse module has been deprecated since python2.7
> in favor of the new-in-2.7 argparse module.
> 
>   http://docs.python.org/library/optparse.html
> 
> Depending on which version of Python is available in other target systems, you
> may want to update.

Seems this was done already with the oauth changes.

Comment 17 Joe Vlcek 2011-11-09 00:18:24 UTC
The latest changes can be found here:
https://github.com/aeolusproject/audrey/tree/joev

After applying the patch in attachment 532333 [details] "make rpms" fails.

It seems the man page is created:
/home/jvlcek/rpmbuild/BUILDROOT/aeolus-audrey-agent-0.4.0-7.fc15.x86_64/usr/share/man/man8/audrey.8

But then gets compressed to:
/home/jvlcek/rpmbuild/BUILDROOT/aeolus-audrey-agent-0.4.0-7.fc15.x86_64/usr/share/man/man8/audrey.8.gz


Tail of "make rpms":

+ cp audrey_startup.py /home/jvlcek/rpmbuild/BUILDROOT/aeolus-audrey-agent-0.4.0-7.fc15.x86_64/usr/bin/audrey
+ cp audrey.8 /home/jvlcek/rpmbuild/BUILDROOT/aeolus-audrey-agent-0.4.0-7.fc15.x86_64/usr/share/man/man8
+ ls -aFlR /home/jvlcek/rpmbuild/BUILDROOT/aeolus-audrey-agent-0.4.0-7.fc15.x86_64/usr/share/man/man8
/home/jvlcek/rpmbuild/BUILDROOT/aeolus-audrey-agent-0.4.0-7.fc15.x86_64/usr/share/man/man8:
total 12
drwxr-xr-x. 2 jvlcek jvlcek 4096 Nov  8 19:15 ./
drwxr-xr-x. 3 jvlcek jvlcek 4096 Nov  8 19:15 ../
-rw-r--r--. 1 jvlcek jvlcek 1071 Nov  8 19:15 audrey.8
+ /usr/lib/rpm/find-debuginfo.sh --strict-build-id /home/jvlcek/rpmbuild/BUILD/aeolus-audrey-agent-0.4.0
+ /usr/lib/rpm/check-buildroot
+ /usr/lib/rpm/redhat/brp-compress
+ /usr/lib/rpm/redhat/brp-strip-static-archive /usr/bin/strip
+ /usr/lib/rpm/brp-python-bytecompile /usr/bin/python 1
+ /usr/lib/rpm/redhat/brp-python-hardlink
+ /usr/lib/rpm/redhat/brp-java-repack-jars
Processing files: aeolus-audrey-agent-0.4.0-7.fc15.noarch
error: File not found: /home/jvlcek/rpmbuild/BUILDROOT/aeolus-audrey-agent-0.4.0-7.fc15.x86_64/usr/share/man/man8/audrey.8
Executing(%doc): /bin/sh -e /var/tmp/rpm-tmp.cIYiNm
+ umask 022
+ cd /home/jvlcek/rpmbuild/BUILD
+ cd aeolus-audrey-agent-0.4.0
+ DOCDIR=/home/jvlcek/rpmbuild/BUILDROOT/aeolus-audrey-agent-0.4.0-7.fc15.x86_64/usr/share/doc/aeolus-audrey-agent-0.4.0
+ export DOCDIR
+ /bin/mkdir -p /home/jvlcek/rpmbuild/BUILDROOT/aeolus-audrey-agent-0.4.0-7.fc15.x86_64/usr/share/doc/aeolus-audrey-agent-0.4.0
+ cp -pr COPYING /home/jvlcek/rpmbuild/BUILDROOT/aeolus-audrey-agent-0.4.0-7.fc15.x86_64/usr/share/doc/aeolus-audrey-agent-0.4.0
+ exit 0


RPM build errors:
    File not found: /home/jvlcek/rpmbuild/BUILDROOT/aeolus-audrey-agent-0.4.0-7.fc15.x86_64/usr/share/man/man8/audrey.8
make: *** [rpms] Error 1

Comment 18 Jim Meyering 2011-11-09 08:28:02 UTC
Created attachment 532490 [details]
tweak spec file so "make rpms" succeeds

There were two problems.  I've attached a revised patch:

    tweak spec file so "make rpms" succeeds
    
    * audrey_startup/aeolus-audrey-agent.spec.in: Install into buildroot,
    not directly into /usr/man.
    (%files): List audrey.8*, not audrey.8, to get the .gz file, too.

Comment 19 Joe Vlcek 2011-11-09 15:36:17 UTC
(In reply to comment #18)
> Created attachment 532490 [details]
> tweak spec file so "make rpms" succeeds
> 
> There were two problems.  I've attached a revised patch:
> 
>     tweak spec file so "make rpms" succeeds
> 
>     * audrey_startup/aeolus-audrey-agent.spec.in: Install into buildroot,
>     not directly into /usr/man.
>     (%files): List audrey.8*, not audrey.8, to get the .gz file, too.

Thank you Jim!

I had caught the "Install into buildroot not directly into /usr/man

But I was not sure of the correct way to handle the .gz file issue.

So again thank you.

I've pushed the latest changes here:
https://github.com/aeolusproject/audrey/tree/joev

I have a question: Instead of hard coding the version in the audrey_start.py
file as:
AUDREY_VER = '0.5.1'

I would like to track it with the RPM version number.

Do you have any suggestions on how best to approach that?

Thank you!
   Joe

Comment 20 Jim Meyering 2011-11-09 15:49:23 UTC
IMHO, the only way is to maintain a template (say audrey.in.py) under version control and have "AUDREY_VER = '@VERSION@' in it.
Then part of the build process would be to generate audrey.py from that
template while performing the simple @VERSION@-substitution, just as is
done for the spec file.  Note that you should retain the .py suffix,
and hence .in.py, rather than .py.in, so that more tools automatically
detect the file as containing python source.

Comment 21 Thomas Spura 2011-11-09 16:05:45 UTC
(In reply to comment #8)
> Regarding:
> > - python-argparse is not available anymore. The argparse module is part of
> > plain python.
> 
> I do want to build for RHEL and Fedora. Don't I need to specify it for RHEL?

How about %if it then properly?
e.g. 
%if ! (0%{?fedora} > 14 || 0%{?rhel} > 6)
BuildRequires: python-argparse
%endif

I guess this is the only sane way to do this, cause python will loose the
Provides for python-argparse some time in the future.

Comment 22 Joe Vlcek 2011-11-09 23:51:49 UTC
(In reply to comment #21)
> (In reply to comment #8)
> > Regarding:
> > > - python-argparse is not available anymore. The argparse module is part of
> > > plain python.
> > 
> > I do want to build for RHEL and Fedora. Don't I need to specify it for RHEL?
> 
> How about %if it then properly?
> e.g. 
> %if ! (0%{?fedora} > 14 || 0%{?rhel} > 6)
> BuildRequires: python-argparse
> %endif
> 
> I guess this is the only sane way to do this, cause python will loose the
> Provides for python-argparse some time in the future.

Thank you again Thomas. I'll do that.

Can you please confirm, should it be BuildRequires or Requires:

Thank you!
   Joe

Comment 23 Joe Vlcek 2011-11-10 00:26:13 UTC
Thank you Jim and Thomas for the review input and all the help with this.

I believe I've address everything.

The latest versions of the important documents can be found here:

Please note it's now in: "/v3"

Spec URL:
http://joev.fedorapeople.org/audrey-agent/v3/aeolus-audrey-agent.spec

SRPM URL:
http://joev.fedorapeople.org/audrey-agent/v3/aeolus-audrey-agent-0.4.0-9.fc15.src.rpm

Koji Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3501875

The upstream URL to the source is:
  https://github.com/aeolusproject/audrey

Comment 24 Jim Meyering 2011-11-10 15:13:46 UTC
Hi Joe,
In the spec, this looks wrong:

  %if ! (0%{?fedora} > 14 || 0%{?rhel} > 6)
  Requires: python-argparse
  %endif

The problem is more apparent if you rewrite without the "!":

  %if (0%{?fedora} <= 14 && 0%{?rhel} <= 6)
  Requires: python-argparse
  %endif

Shouldn't it be || in place of && here?
(i.e., && in place of || in the original)

  %if (0%{?fedora} <= 14 || 0%{?rhel} <= 6)
  Requires: python-argparse
  %endif

Comment 25 Jim Meyering 2011-11-10 15:20:21 UTC
Hi Joe,
I installed the new .src.rpm, then ran
  tar xf ~/rpmbuild/SOURCES/aeolus-audrey-agent-0.4.0.tar.gz &&
  cd aeolus-audrey-agent && make
which printed this:

  make: *** No rule to make target `audrey_startup.in.py', needed by\
   `audrey_startup.py'.  Stop.

which means the new, required file is not included in the tarball.

Comment 26 Joe Vlcek 2011-11-10 20:28:28 UTC
Hi Jim,

Huge "Thank you"! for all the help with this.

I believe I've address everything.

The latest versions of the important documents can be found here:

Please note it's now in: "/v4"

Spec URL:
http://joev.fedorapeople.org/audrey-agent/v4/aeolus-audrey-agent.spec


SRPM URL:
http://joev.fedorapeople.org/audrey-agent/v4/aeolus-audrey-agent-0.4.0-9.fc15.src.rpm


Koji Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3505835

The upstream URL to the source is:
  https://github.com/aeolusproject/audrey

Comment 27 Jim Meyering 2011-11-10 21:34:54 UTC
Joe, with this, I think I am done:

  $ rpmlint aeolus-audrey-agent-0.4.0-9.fc15.src.rpm
  aeolus-audrey-agent.src: W: invalid-license ASL2.0
  aeolus-audrey-agent.src: W: invalid-url Source0: aeolus-audrey-agent-0.4.0.tar.gz

Please fix #2: replace "ASL2" by "ASL 2.0"

The first warning would be well enough addressed by the comments if you had
created/pushed a gpg-signed git tag and if the instructions said to use that.

    # to build source tarball
    # git clone git:aeolusproject/audrey.git
    # make dist

However, without that, the above clone instructions are not
enough to reproduce your source RPM, which is the goal, after all.
Once you've fixed these things, just put the tarball in
 http://joev.fedorapeople.org/audrey-agent/ and use its URL
in the Source0: directive.

Now for the .rpm:

  $ rpmlint /h/j/rpmbuild/RPMS/noarch/aeolus-audrey-agent-0.4.0-9.fc16.noarch.rpm
  aeolus-audrey-agent.noarch: E: explicit-lib-dependency python-httplib2
  aeolus-audrey-agent.noarch: W: invalid-license ASL2.0
  aeolus-audrey-agent.noarch: E: non-standard-executable-perm /usr/bin/audrey 0555L
  1 packages and 0 specfiles checked; 2 errors, 1 warnings

Run rpmlint with the -i option and it'll give you more details.
You can address the non-standard-executable-perm error by using
"install" instead of "cp".  It will do what rpmlint wants: i.e.,
permissions of 0755, not 0555.

As I understand it, the python-httplib2 error is unwarranted
and thus ignorable.

Address the above and you'll have my approval.
Two "MUST" items are marked "NO" below because of the above.

=========================================================
From https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

ok  MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.[1]
ok  MUST: The package must be named according to the Package Naming Guidelines .
ok  MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. [2] .
ok  MUST: The package must meet the Packaging Guidelines .
ok  MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
    MUST: The License field in the package spec file must match the actual license. [3]
ok  MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.[4]
ok  MUST: The spec file must be written in American English. [5]
ok  MUST: The spec file for the package MUST be legible. [6]

NO (upstream source URL not specified)
__  MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.

ok  MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [7]
NA  MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. [8]
ok  MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
ok  MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.[9]
NA  MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. [10]
ok  MUST: Packages must NOT bundle copies of system libraries.[11]
NA  MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. [12]
ok  MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. [13]
ok  MUST: A Fedora package must not list a file more than once in the spec file's %files listings. (Notable exception: license texts in specific situations)[14]
NO(0555 vs 0755, does this really matter?  maybe for consistency)
__  MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. [15]
ok  MUST: Each package must consistently use macros. [16]
ok  MUST: The package must contain code, or permissable content. [17]
ok  MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). [18]
ok  MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. [18]
NA  MUST: Header files must be in a -devel package. [19]
NA  MUST: Static libraries must be in a -static package. [20]
NA  MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. [19]
NA  MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name}%{?_isa} = %{version}-%{release} [21]
NA  MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[20]
NA  MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. [22]
NA  MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. [23]
NA  MUST: All filenames in rpm packages must be valid UTF-8. [24]



SHOULD Items:
Items marked as SHOULD are things that the package (or reviewer) SHOULD do, but is not required to do.

ok  SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [25]
NO (there are no translations)  SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. [26]
ok  SHOULD: The reviewer should test that the package builds in mock. [27]
NA  SHOULD: The package should compile and build into binary rpms on all supported architectures. [28]
NO (I haven't done this)  SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.
NA  SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. [29]
NA  SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [21]
NA  SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. [30]
NA  SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. [31]
ok  SHOULD: your package should contain man pages for binaries/scripts. If it doesn't, work with upstream to add them where they make sense.[32]

Comment 28 Joe Vlcek 2011-11-10 23:07:36 UTC
Thank you again Jim!

With this, I think we are done.

The latest versions of the important documents can be found here:

Please note it's now in: "/v5"

Spec URL:
http://joev.fedorapeople.org/audrey-agent/v5/aeolus-audrey-agent.spec

SRPM URL:
http://joev.fedorapeople.org/audrey-agent/v5/aeolus-audrey-agent-0.4.0-9.fc15.src.rpm

Koji Build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3506507

The upstream URL to the source is:
  https://github.com/aeolusproject/audrey

Comment 29 Jim Meyering 2011-11-11 12:47:49 UTC
approved

Comment 30 Joe Vlcek 2011-11-30 14:42:27 UTC
New Package SCM Request
=======================
Package Name: aeolus-audrey-agent
Short Description: The Aeolus Audrey Startup Agent
Owners: joev
Branches: f16
InitialCC:

Comment 31 Gwyn Ciesla 2011-11-30 15:16:00 UTC
Git done (by process-git-requests).

Comment 32 Fedora Update System 2011-11-30 20:40:13 UTC
aeolus-audrey-agent-0.4.0-9.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/aeolus-audrey-agent-0.4.0-9.fc16

Comment 33 Fedora Update System 2011-12-04 02:45:37 UTC
aeolus-audrey-agent-0.4.0-9.fc16 has been pushed to the Fedora 16 testing repository.

Comment 34 Fedora Update System 2011-12-13 21:54:27 UTC
aeolus-audrey-agent-0.4.0-9.fc16 has been pushed to the Fedora 16 stable repository.


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