Is python-kickstart a thing? I see only pykickstart here.

You can remove the dependencies on ldconfig, they will be added automatically when you use ldconfig in the %post/%postun scriplets.

In %description, there's a stray "L".

"User can specify only INI file and scripts
and other stuff needed by oscap is generated automatically." → something is wrong with this sentence.

Requires:   %{name}                   <------ remove this one
Requires:   %{name} = %{version}

Can the package be noarch?

I this will not be built for old EPELs, remove Group, BuildRoot, %clean, %defattr.

Use %post -p /sbin/ldconfig for %post. It does not require bash invocation.

Use %{_docdir} instead of %{_datadir}/doc.

Remove 'cp %SOURCE1 .', you copy it from the source location later on anyway.

Remove 'mkdir -p -m 755 $RPM_BUILD_ROOT%{_rpmconfigdir}/macros.d' and add -D on the next line.

The same for 'install -d -m 755 $RPM_BUILD_ROOT%{_mandir}/man1'.

The tests fail for me with:
======================================================================
ERROR: test_opts (tests.test_preup.TestCLI)
basic test of several options
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/preupgrade-assistant-0.11.5/tests/test_preup.py", line 111, in test_opts
    "--contents", "content/FOOBAR6_7", "--cleanup"])
  File "/builddir/build/BUILD/preupgrade-assistant-0.11.5/preup/cli.py", line 22, in __init__
    self.opts, self.args = self.parser.parse_args(args=args)
  File "/usr/lib/python2.7/optparse.py", line 1402, in parse_args
    self.error(str(err))
  File "/usr/lib/python2.7/optparse.py", line 1584, in error
    self.exit(2, "%s: error: %s\n" % (self.get_prog_name(), msg))
  File "/usr/lib/python2.7/optparse.py", line 1574, in exit
    sys.exit(status)
SystemExit: 2

Why would anyone use optparse instead of argparse in this day and age is beyond me, but it's not a packaging issue, ... so this is just a side note, not part of the review.

Well, this package is going to be used as on Fedora 22 as on RHEL 6.
Upstream on GitHub should have common code.
Therefore optparse has been used.
argparse is available only in EPEL 6.

you are right, in Fedora 22 only pykickstart exists.
Since Fedora 23 pykickstart and python3-pykickstart exist.

I am trying to build the package in Copr
http://copr-fe.cloud.fedoraproject.org/coprs/phracek/preupgrade-assistant/

but I saw that there are some problems with python3.

SRPM Url: https://phracek.fedorapeople.org/preupgrade-assistant-0.11.5-2.fc21.src.rpm
Spec Url: https://phracek.fedorapeople.org/preupgrade-assistant.spec

Use %license for LICENSE. See https://fedorahosted.org/fpc/ticket/411#comment:11 for a fallback.

Now it fails with:
File "/builddir/build/BUILD/preupgrade-assistant-0.11.5/preuputils/oscap_group_xml.py", line 8, in <module>
    import ConfigParser
ImportError: No module named 'ConfigParser'

There's only configparser afaik.

Then it fails with:
File "/builddir/build/BUILD/preupgrade-assistant-0.11.5/preuputils/script_utils.py", line 6, in <module>
    import xml_utils
ImportError: No module named 'xml_utils'

This seems to be an incompatible absolute import.

I have the feeling nobody ever run this code?!

It would be great if you could check if it builds in mock for you.

Spec file mostly looks good. I'm wondering though, before this gets set in stone: binaries like create_group_xml are rather generic. Maybe you could rename them to preupg-create-group-xml, preupg-xccdf-compose, etc.

Today finally my mock is running.

But python3 does not exist rawhide yet.
http://koji.fedoraproject.org/koji/packageinfo?packageID=9781
And therefore ConfigParser is not available yet:(

ConfigParser class is defined in configparser.py file, though.
ConfigParser (configparser.py) is part of subpackage python3-libs.

On Fedora 22 ConfigParser is here: http://koji.fedoraproject.org/koji/rpminfo?fileStart=450&rpmID=5873335&fileOrder=name&buildrootOrder=-id&buildrootStart=50#filelist

Thanks for good idea with renaming binaries. It really makes sense.
I am going to do that.

I tried to build package in Copr, but F22 branch does not exist yet.

I have updated SPEC file so that in F22 it uses python2. But Since F23 is uses python3.

Spec URL: https://phracek.fedorapeople.org/preupgrade-assistant.spec
SRPM URL: https://phracek.fedorapeople.org/preupgrade-assistant-0.11.6-1.fc21.src.rpm

Hopefully the latest correction.
Both mock builds were successful (rawhide and F22).

Rawhide build in Copr is also done.
http://copr-fe.cloud.fedoraproject.org/coprs/phracek/preupgrade-assistant/builds/

(In reply to Petr Hracek from comment #4)
> Today finally my mock is running.
> 
> But python3 does not exist rawhide yet.
> http://koji.fedoraproject.org/koji/packageinfo?packageID=9781
> And therefore ConfigParser is not available yet:(
When I look at the list, I see three F23 builds right at the top. I'm not sure why do you think that there are no rawhide builds.

> ConfigParser class is defined in configparser.py file, though.
> ConfigParser (configparser.py) is part of subpackage python3-libs.
Yeah, it got renamed. You need to either rename it too, or if you're providing Py2 compat, use something like:
  try:
    import configparser
  except ImportError:
    import ConfigParser as configparser

> Thanks for good idea with renaming binaries. It really makes sense.
> I am going to do that.
Cool, thanks.

> I tried to build package in Copr, but F22 branch does not exist yet.
You might need to update you mock package first. It carries the buildroot definitions. F22 and F23 build roots have been working for the last few weeks or so.

> I have updated SPEC file so that in F22 it uses python2. But Since F23 is
> uses python3.
This makes things much more complicated for no good reason. The spec file is much more complicated, and it'll also be confusing for users. I'd strongly advise against that.

Although not strictly required, the intent of %license is to be used with bare filename, so that the file is installed in /usr/share/licenses. Please remove LICENSE from %{_pkgdocdir}
and add in %files:
  %license LICENSE

fedora-review says:
- Sources used to build the package match the upstream source, as provided in
  the spec URL.
  Note: Upstream MD5sum check error, diff is in /var/tmp/1195279-preupgrade-
  assistant/diff.txt
  See: http://fedoraproject.org/wiki/Packaging/SourceURL

E: explicit-lib-dependency python3-libs
Yep, such dependencies are added automatically.

W: spurious-executable-perm /usr/share/man/man1/preupg.1.gz

E: non-executable-script /usr/lib/python3.4/site-packages/preuputils/postupgrade.py 0644L /usr/bin/python2

preupgrade-assistant.src:96: W: macro-in-comment %{__python2}
preupgrade-assistant.src:97: W: macro-in-comment %else
preupgrade-assistant.src:98: W: macro-in-comment %{__python3}
preupgrade-assistant.src:99: W: macro-in-comment %endif

I hope you can reverse the Python2 / 3 split. I'll hold on actually testing the package and further review until you make the decision. Testing both versions is twice as much work :)

Thanks for huge comments.

As you can see in koji Python3 in rawhide hasn't built correctly and it still fails.

I am going to change LICENSE file as you recommended.

Regarding configparser. All is working properly.
configparser.py provides class ConfigParser in F22 and in F23 too.
This was not changed.

(In reply to Petr Hracek from comment #6)
> As you can see in koji Python3 in rawhide hasn't built correctly and it
> still fails.
Oh, I see it now. But the previous build should still be available, so it should not matter for you.

> Regarding configparser. All is working properly.
> configparser.py provides class ConfigParser in F22 and in F23 too.
> This was not changed.
The class name did not change. But the module name did.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #7)
> (In reply to Petr Hracek from comment #6)
> > As you can see in koji Python3 in rawhide hasn't built correctly and it
> > still fails.
> Oh, I see it now. But the previous build should still be available, so it
> should not matter for you.
> 
Yes you are right
> > Regarding configparser. All is working properly.
> > configparser.py provides class ConfigParser in F22 and in F23 too.
> > This was not changed.
> The class name did not change. But the module name did.

Well, I have checked the class name and module name configparser.py and both are identical. F22 and F23.

Spec URL: https://phracek.fedorapeople.org/preupgrade-assistant.spec
SRPM URL: https://phracek.fedorapeople.org/preupgrade-assistant-0.11.7-1.fc21.src.rpm

Would it be possible to execute fedora-review and paste result?
In F21 I have problems with fedora-review.

Created attachment 996036 [details]
fedora-review output

(In reply to Petr Hracek from comment #8)
> > > Regarding configparser. All is working properly.
> > > configparser.py provides class ConfigParser in F22 and in F23 too.
> > > This was not changed.
> > The class name did not change. But the module name did.
> 
> Well, I have checked the class name and module name configparser.py and both
> are identical. F22 and F23.
It changed between Python 2 and 3. In comment #c5 there's a code snippet to make the code compatible with both versions.

OK, I see you removed %check. This is *not* the way to go. If the tests find a problem, fix the problem, don't remove the tests!

We are going in circles. Let's go back to the beginning, and start with the basic questions:

- what python version is preupgrade-assistant supposed to run? Is it going to be packaged for F21? Is the answer the same for both F23 and F22 and F21? 

Some data points:
- in F22 pykickstart is at 1.99.66 and does *not* provide a python 3 version
- in F23 pykickstart is at 2.0 and does provide python3-kickstart
- the version in F21 is even older and does Python 2 only
- the code in ./preuputils/oscap_group_xml.py is atm Python2 only

> Spec URL: https://phracek.fedorapeople.org/preupgrade-assistant.spec
> SRPM URL:
> https://phracek.fedorapeople.org/preupgrade-assistant-0.11.7-1.fc21.src.rpm
> 
> Would it be possible to execute fedora-review and paste result?
> In F21 I have problems with fedora-review.
Attached.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #9)
> Created attachment 996036 [details]
> fedora-review output
> 
> (In reply to Petr Hracek from comment #8)
> > > > Regarding configparser. All is working properly.
> > > > configparser.py provides class ConfigParser in F22 and in F23 too.
> > > > This was not changed.
> > > The class name did not change. But the module name did.
> > 
> > Well, I have checked the class name and module name configparser.py and both
> > are identical. F22 and F23.
> It changed between Python 2 and 3. In comment #c5 there's a code snippet to
> make the code compatible with both versions.
Only Python3 is supported by preupgrade-assistant
> 
> OK, I see you removed %check. This is *not* the way to go. If the tests find
> a problem, fix the problem, don't remove the tests!
> 
I am going to add them later on. This BZ blocks a Feature for Fedora 22.
> We are going in circles. Let's go back to the beginning, and start with the
> basic questions:
> 
> - what python version is preupgrade-assistant supposed to run? Is it going
> to be packaged for F21? Is the answer the same for both F23 and F22 and F21? 
> 
Preupgrade-assistant is going to be used only for F22 and later. Not F21.
> Some data points:
> - in F22 pykickstart is at 1.99.66 and does *not* provide a python 3 version
I know that. Therefore in SPEC file is mentioned if fedora is bigger then 23 then include python3-kickstart.
> - in F23 pykickstart is at 2.0 and does provide python3-kickstart
> - the version in F21 is even older and does Python 2 only
No F21.
> - the code in ./preuputils/oscap_group_xml.py is atm Python2 only
I have forgot that. I am going to change that in the next version.
> 
> > Spec URL: https://phracek.fedorapeople.org/preupgrade-assistant.spec
> > SRPM URL:
> > https://phracek.fedorapeople.org/preupgrade-assistant-0.11.7-1.fc21.src.rpm
> > 
> > Would it be possible to execute fedora-review and paste result?
> > In F21 I have problems with fedora-review.
> Attached.
Thanks for the review.

The preupgrade-assistant is already used on RHEL 6 systems before fedup.
I have decided to release them in Fedora. Therefore code have to be compatible for Python3, Python2.7 and Python2.6. It is a bit complicated.

And for your time with review.

But question is how to make preupgrade-assistant project compatible with Python2.6.
Python2.7 is wrong of course because it is not delivered to F21. Sorry for mystification.
I do not want to create a separate branch.

I thought that if oscap_group_xml is called from Python3 than it does not make sence if /usr/bin/python2 is mentioned.

But I can create a patch for RHEL6 which will replace /usr/bin/python3 with /usr/bin/python2 of course.

(In reply to Petr Hracek from comment #10)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #9)
> > Created attachment 996036 [details]
> > fedora-review output
> > 
> > (In reply to Petr Hracek from comment #8)
> > > > > Regarding configparser. All is working properly.
> > > > > configparser.py provides class ConfigParser in F22 and in F23 too.
> > > > > This was not changed.
> > > > The class name did not change. But the module name did.
> > > 
> > > Well, I have checked the class name and module name configparser.py and both
> > > are identical. F22 and F23.
> > It changed between Python 2 and 3. In comment #c5 there's a code snippet to
> > make the code compatible with both versions.
> Only Python3 is supported by preupgrade-assistant
OK.
 
> > OK, I see you removed %check. This is *not* the way to go. If the tests find
> > a problem, fix the problem, don't remove the tests!
> > 
> I am going to add them later on. This BZ blocks a Feature for Fedora 22.
The tests are helpful. They already caught a silly bug, so it seems
right to keep them, even in F22.

> > We are going in circles. Let's go back to the beginning, and start with the
> > basic questions:
> > 
> > - what python version is preupgrade-assistant supposed to run? Is it going
> > to be packaged for F21? Is the answer the same for both F23 and F22 and F21? 
> > 
> Preupgrade-assistant is going to be used only for F22 and later. Not F21.
> > Some data points:
> > - in F22 pykickstart is at 1.99.66 and does *not* provide a python 3 version
> I know that. Therefore in SPEC file is mentioned if fedora is bigger then 23
> then include python3-kickstart.
But you cannot use pykickstart (a Python2-only package) with a program
running under Python 3. If preupgrade-assistant is Python3-only, then
any Requires:pykickstart or BuildRequires:pykickstart can to be removed.

Can pykickstart version in F22 be updated to the same version as F23?
This would solve the problem with missing dependency for you.

> > - in F23 pykickstart is at 2.0 and does provide python3-kickstart
> > - the version in F21 is even older and does Python 2 only
> No F21.
OK.

> > - the code in ./preuputils/oscap_group_xml.py is atm Python2 only
> I have forgot that. I am going to change that in the next version.

Well, configparser is now corrected.

Spec URL: https://phracek.fedorapeople.org/preupgrade-assistant.spec
SRPM URL: https://phracek.fedorapeople.org/preupgrade-assistant-0.11.7-2.fc21.src.rpm

from the pykickstart point of view I am not a maintainer but I can ask.
But I think that it won't be updated.

Tests are going to be turn on definitelly. Also in F22.

In F22 can be used only pykickstart
In F23 python3-pykickstart is enough.

(In reply to Petr Hracek from comment #14)
> from the pykickstart point of view I am not a maintainer but I can ask.
> But I think that it won't be updated.
> 
> Tests are going to be turn on definitelly. Also in F22.
> 
> In F22 can be used only pykickstart
How? pykickstart provides a module for Python2, but you will be running under Python 3.

> In F23 python3-pykickstart is enough.

Hi Chris,

I would like to ask you if you can merge pykickstart which is available in rawhide (F23) to F22.

pykickstart in F22 supports only python2, right?
pykickstart in F23 supports both version.

Greetings
Petr

I have discussed this issue with Chris and pykickstart will not be backported.

Therefore I suggest to build preupgrade-assistant with Python2 support for F22
and to build preupgrade-assistant with Python3 support for F23.

(In reply to Petr Hracek from comment #17)
> Therefore I suggest to build preupgrade-assistant with Python2 support for
> F22 and to build preupgrade-assistant with Python3 support for F23.
OK.

Can you submit an updated spec file?

New files are available:
Spec URL: https://phracek.fedorapeople.org/preupgrade-assistant.spec
SRPM URL: https://phracek.fedorapeople.org/preupgrade-assistant-0.11.7-3.fc21.src.rpm

- Why BR:pykickstart R:pykickstart in F23?

- Can you add %check back?

- Why the extra directory level in %{_rpmconfigdir}/macros.d/macros.preupgrade-assistant/macros.preupgrade-assistant? I don't see other packages doing this.

- %preupgrade_build is defined to the old name /usr/bin/create_group_xml.

- preupgrade-assistant.rpm has an empty /usr/share/doc/preupgrade directory and README files are in /usr/share/preupgrade.

- I installed the rpm on F21. /usr/bin/preupg fails with:
Traceback (most recent call last):
  File "/usr/bin/preupg", line 8, in <module>
    from preup.application import Application
  File "/usr/lib/python2.7/site-packages/preup/application.py", line 18, in <module>
    from preuputils.compose import XCCDFCompose
ImportError: No module named preuputils.compose

Installing also -devel subpackage fixes that. Maybe you should merge -devel back into the main package. They are both really tiny, so the split is probably not worth the trouble.

- Looking at the first message:
The Preupgrade Assistant is a diagnostics tool 
and does not perform the actual upgrade.
Make sure you back up your system and all of your data now,
before using the Upgrade Tool to avoid potential data loss.
Do you want to continue? y/n

If it is only a diagnostic tool, why is it unsafe to run?
Also, why does it require root privileges? Isn't the RPM database public?

- In the man page: "All common log files are stored in /var/cache/preupgrade/common". Isn't /var/log/preupgrade used?

- This package seems useless without preupgrade-assistant-contents. Is this available somewhere?

Spec URL: https://phracek.fedorapeople.org/preupgrade-assistant.spec
SRPM URL: https://phracek.fedorapeople.org/preupgrade-assistant-0.11.7-4.fc21.src.rpm

(In reply to Zbigniew Jędrzejewski-Szmek from comment #20)
> - Why BR:pykickstart R:pykickstart in F23?
> 
You are right python3-kickstart should require pykickstart in F23
> - Can you add %check back?
> 
Done
> - Why the extra directory level in
> %{_rpmconfigdir}/macros.d/macros.preupgrade-assistant/macros.preupgrade-
> assistant? I don't see other packages doing this.
> 
Typo
> - %preupgrade_build is defined to the old name /usr/bin/create_group_xml.
> 
Fixed
> - preupgrade-assistant.rpm has an empty /usr/share/doc/preupgrade directory
> and README files are in /usr/share/preupgrade.
> 
README is in /usr/share/doc/preupgrade directory.
README file should be in both directories. The file is copied to /root/preupgrade/ directory after an assessment.
User can see after a upgrade/migration what results mean.
But I will corrected it after package review so that only one README file is enough.

> - I installed the rpm on F21. /usr/bin/preupg fails with:
> Traceback (most recent call last):
>   File "/usr/bin/preupg", line 8, in <module>
>     from preup.application import Application
>   File "/usr/lib/python2.7/site-packages/preup/application.py", line 18, in
> <module>
>     from preuputils.compose import XCCDFCompose
> ImportError: No module named preuputils.compose
> 
> Installing also -devel subpackage fixes that. Maybe you should merge -devel
> back into the main package. They are both really tiny, so the split is
> probably not worth the trouble.
> 
Merged
> - Looking at the first message:
> The Preupgrade Assistant is a diagnostics tool 
> and does not perform the actual upgrade.
> Make sure you back up your system and all of your data now,
> before using the Upgrade Tool to avoid potential data loss.
> Do you want to continue? y/n
> 
> If it is only a diagnostic tool, why is it unsafe to run?
> Also, why does it require root privileges? Isn't the RPM database public?
We need to have a access to all files and directories. Root access is really needed. Some data are accessible only under root account.
> 
> - In the man page: "All common log files are stored in
> /var/cache/preupgrade/common". Isn't /var/log/preupgrade used?
Those directories are different.
- /var/log/preupgrade directory is used for logs generated by preupgrade-assistant itself as a program
- /var/cache/preupgrade/common directory contains special files used by contents
> 
> - This package seems useless without preupgrade-assistant-contents. Is this
> available somewhere?

Draft for Packaging Guidelines is already finished (https://fedoraproject.org/wiki/User:Phracek/Draft:Packaging:PreupgradeAssistant)
Currently now I would like to add it to http://fedoraproject.org/wiki/Packaging:Guidelines#Application_Specific_Guidelines

I will send a mail to devel list which describes how to create a content for preupgrade-assistant.

I will create several bugzilla's to specific components for creating contents. Likie mariadb, postgresql, etc.

(In reply to Petr Hracek from comment #21)
> Spec URL: https://phracek.fedorapeople.org/preupgrade-assistant.spec
> SRPM URL:
> https://phracek.fedorapeople.org/preupgrade-assistant-0.11.7-4.fc21.src.rpm
It seems that the problem from comment #c3 has resurfaced. In F23 mock:

running build_ext
Traceback (most recent call last):
  File "setup.py", line 61, in <module>
    test_suite      = 'tests.suite',
  File "/usr/lib64/python3.4/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/usr/lib64/python3.4/distutils/dist.py", line 955, in run_commands
    self.run_command(cmd)
  File "/usr/lib64/python3.4/distutils/dist.py", line 974, in run_command
    cmd_obj.run()
  File "/usr/lib/python3.4/site-packages/setuptools/command/test.py", line 142, in run
    self.with_project_on_sys_path(self.run_tests)
  File "/usr/lib/python3.4/site-packages/setuptools/command/test.py", line 122, in with_project_on_sys_path
    func()
  File "/usr/lib/python3.4/site-packages/setuptools/command/test.py", line 163, in run_tests
    testRunner=self._resolve_as_ep(self.test_runner),
  File "/usr/lib64/python3.4/unittest/main.py", line 92, in __init__
    self.parseArgs(argv)
  File "/usr/lib64/python3.4/unittest/main.py", line 139, in parseArgs
    self.createTests()
  File "/usr/lib64/python3.4/unittest/main.py", line 146, in createTests
    self.module)
  File "/usr/lib64/python3.4/unittest/loader.py", line 146, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/lib64/python3.4/unittest/loader.py", line 146, in <listcomp>
    suites = [self.loadTestsFromName(name, module) for name in names]
  File "/usr/lib64/python3.4/unittest/loader.py", line 105, in loadTestsFromName
    module = __import__('.'.join(parts_copy))
  File "/builddir/build/BUILD/preupgrade-assistant-0.11.7/tests/__init__.py", line 2, in <module>
    from tests import test_preup
  File "/builddir/build/BUILD/preupgrade-assistant-0.11.7/tests/test_preup.py", line 6, in <module>
    from preup.application import Application
  File "/builddir/build/BUILD/preupgrade-assistant-0.11.7/preup/application.py", line 18, in <module>
    from preuputils.compose import XCCDFCompose
  File "/builddir/build/BUILD/preupgrade-assistant-0.11.7/preuputils/compose.py", line 12, in <module>
    from preuputils.oscap_group_xml import OscapGroupXml
  File "/builddir/build/BUILD/preupgrade-assistant-0.11.7/preuputils/oscap_group_xml.py", line 13, in <module>
    from preuputils.xml_utils import print_error_msg, XmlUtils
  File "/builddir/build/BUILD/preupgrade-assistant-0.11.7/preuputils/xml_utils.py", line 11, in <module>
    from preuputils import script_utils
  File "/builddir/build/BUILD/preupgrade-assistant-0.11.7/preuputils/script_utils.py", line 6, in <module>
    import xml_utils
ImportError: No module named 'xml_utils'

> (In reply to Zbigniew Jędrzejewski-Szmek from comment #20)
> > - Why BR:pykickstart R:pykickstart in F23?
> > 
> You are right python3-kickstart should require pykickstart in F23
Are you sure? The only reason to do that would be if python3-kickstart called binaries (ksflatten, ksshell, ksvalidator, ksverdiff) provided by pykickstart.

The reason for my original question was that this dependency seems spurious since pykickstart is for Python2, and preupgrade-asistant is running under Python3 in F23. In the spec file I see you moved R/BR:pykicstart underneath %if 0%{?fedora} == 22, so this seems fine now.

> > - preupgrade-assistant.rpm has an empty /usr/share/doc/preupgrade directory
> > and README files are in /usr/share/preupgrade.
> > 
> README is in /usr/share/doc/preupgrade directory.
> README file should be in both directories. The file is copied to
> /root/preupgrade/ directory after an assessment.
> User can see after a upgrade/migration what results mean.
> But I will corrected it after package review so that only one README file is
> enough.
I think it is fine as is. You have to be careful here because of the rule that packages cannot require anything in %doc at runtime. So having two copies of this very small file is probably the best option.

> > - Looking at the first message:
> > The Preupgrade Assistant is a diagnostics tool 
> > and does not perform the actual upgrade.
> > Make sure you back up your system and all of your data now,
> > before using the Upgrade Tool to avoid potential data loss.
> > Do you want to continue? y/n
> > 
> > If it is only a diagnostic tool, why is it unsafe to run?
> > Also, why does it require root privileges? Isn't the RPM database public?
> We need to have a access to all files and directories. Root access is really
> needed. Some data are accessible only under root account.
OK, so it needs to run as root. By why the scary warning?

> > - In the man page: "All common log files are stored in
> > /var/cache/preupgrade/common". Isn't /var/log/preupgrade used?
> Those directories are different.
> - /var/log/preupgrade directory is used for logs generated by
> preupgrade-assistant itself as a program
> - /var/cache/preupgrade/common directory contains special files used by
> contents
OK.

> > - I installed the rpm on F21. /usr/bin/preupg fails with:
> > Traceback (most recent call last):
> >   File "/usr/bin/preupg", line 8, in <module>
> >     from preup.application import Application
> >   File "/usr/lib/python2.7/site-packages/preup/application.py", line 18, in
> > <module>
> >     from preuputils.compose import XCCDFCompose
> > ImportError: No module named preuputils.compose
> > 
> > Installing also -devel subpackage fixes that. Maybe you should merge -devel
> > back into the main package. They are both really tiny, so the split is
> > probably not worth the trouble.
> > 
> Merged
Hm, I don't see this. In the spec file there's still "%package devel".

> > - This package seems useless without preupgrade-assistant-contents. Is this
> > available somewhere?
> 
> Draft for Packaging Guidelines is already finished
> (https://fedoraproject.org/wiki/User:Phracek/Draft:Packaging:
> PreupgradeAssistant)
> Currently now I would like to add it to
> http://fedoraproject.org/wiki/Packaging:
> Guidelines#Application_Specific_Guidelines
> 
> I will send a mail to devel list which describes how to create a content for
> preupgrade-assistant.
> 
> I will create several bugzilla's to specific components for creating
> contents. Likie mariadb, postgresql, etc.
OK, thanks.

Two comments on the guidelines:
The Python example does not take into account the Python2/Python3 split. I think you must explain that the script must run under Python2 in F22 and under Python3 in F23 (and have #!/usr/bin/python3). Alternatively, you could package the module for Python2 in F23 too, in a python-preupgrade-assistant subpackage, to allow people to use Python2 for the scripts in F22 and F23, or Python2 in F22 and Python3 in F23.

%doc %{preupgrade_dir}/%{name}/*.txt → this looks wrong, packages are not allowed to use files in %doc at runtime. The whole section can be simplified to:
%files -n preupgrade-assistant-%{name}
%{preupgrade_dir}/%{name}/

Spec URL: https://phracek.fedorapeople.org/preupgrade-assistant.spec
SRPM URL: https://phracek.fedorapeople.org/preupgrade-assistant-0.11.7-5.fc21.src.rpm

(In reply to Zbigniew Jędrzejewski-Szmek from comment #22)
> (In reply to Petr Hracek from comment #21)
> > Spec URL: https://phracek.fedorapeople.org/preupgrade-assistant.spec
> > SRPM URL:
> > https://phracek.fedorapeople.org/preupgrade-assistant-0.11.7-4.fc21.src.rpm
> It seems that the problem from comment #c3 has resurfaced. In F23 mock:
> 
> running build_ext
> Traceback (most recent call last):
>   File "setup.py", line 61, in <module>
>     test_suite      = 'tests.suite',
>   File "/usr/lib64/python3.4/distutils/core.py", line 148, in setup
>     dist.run_commands()
>   File "/usr/lib64/python3.4/distutils/dist.py", line 955, in run_commands
>     self.run_command(cmd)
>   File "/usr/lib64/python3.4/distutils/dist.py", line 974, in run_command
>     cmd_obj.run()
>   File "/usr/lib/python3.4/site-packages/setuptools/command/test.py", line
> 142, in run
>     self.with_project_on_sys_path(self.run_tests)
>   File "/usr/lib/python3.4/site-packages/setuptools/command/test.py", line
> 122, in with_project_on_sys_path
>     func()
>   File "/usr/lib/python3.4/site-packages/setuptools/command/test.py", line
> 163, in run_tests
>     testRunner=self._resolve_as_ep(self.test_runner),
>   File "/usr/lib64/python3.4/unittest/main.py", line 92, in __init__
>     self.parseArgs(argv)
>   File "/usr/lib64/python3.4/unittest/main.py", line 139, in parseArgs
>     self.createTests()
>   File "/usr/lib64/python3.4/unittest/main.py", line 146, in createTests
>     self.module)
>   File "/usr/lib64/python3.4/unittest/loader.py", line 146, in
> loadTestsFromNames
>     suites = [self.loadTestsFromName(name, module) for name in names]
>   File "/usr/lib64/python3.4/unittest/loader.py", line 146, in <listcomp>
>     suites = [self.loadTestsFromName(name, module) for name in names]
>   File "/usr/lib64/python3.4/unittest/loader.py", line 105, in
> loadTestsFromName
>     module = __import__('.'.join(parts_copy))
>   File
> "/builddir/build/BUILD/preupgrade-assistant-0.11.7/tests/__init__.py", line
> 2, in <module>
>     from tests import test_preup
>   File
> "/builddir/build/BUILD/preupgrade-assistant-0.11.7/tests/test_preup.py",
> line 6, in <module>
>     from preup.application import Application
>   File
> "/builddir/build/BUILD/preupgrade-assistant-0.11.7/preup/application.py",
> line 18, in <module>
>     from preuputils.compose import XCCDFCompose
>   File
> "/builddir/build/BUILD/preupgrade-assistant-0.11.7/preuputils/compose.py",
> line 12, in <module>
>     from preuputils.oscap_group_xml import OscapGroupXml
>   File
> "/builddir/build/BUILD/preupgrade-assistant-0.11.7/preuputils/
> oscap_group_xml.py", line 13, in <module>
>     from preuputils.xml_utils import print_error_msg, XmlUtils
>   File
> "/builddir/build/BUILD/preupgrade-assistant-0.11.7/preuputils/xml_utils.py",
> line 11, in <module>
>     from preuputils import script_utils
>   File
> "/builddir/build/BUILD/preupgrade-assistant-0.11.7/preuputils/script_utils.
> py", line 6, in <module>
>     import xml_utils
> ImportError: No module named 'xml_utils'
> 
Fixed with a bunch of fixes for F23 too.
Mock builds on F22 and F23 works. Finally:)
> > (In reply to Zbigniew Jędrzejewski-Szmek from comment #20)
> > > - Why BR:pykickstart R:pykickstart in F23?
> > > 
> > You are right python3-kickstart should require pykickstart in F23
> Are you sure? The only reason to do that would be if python3-kickstart
> called binaries (ksflatten, ksshell, ksvalidator, ksverdiff) provided by
> pykickstart.
> 
In F23 pykickstart requires python3-pykickstart based from pykickstart.spec
Therefore only python3-kickstart is needed.
Fixed.

> The reason for my original question was that this dependency seems spurious
> since pykickstart is for Python2, and preupgrade-asistant is running under
> Python3 in F23. In the spec file I see you moved R/BR:pykicstart underneath
> %if 0%{?fedora} == 22, so this seems fine now.
> 
> > > - preupgrade-assistant.rpm has an empty /usr/share/doc/preupgrade directory
> > > and README files are in /usr/share/preupgrade.
> > > 
> > README is in /usr/share/doc/preupgrade directory.
> > README file should be in both directories. The file is copied to
> > /root/preupgrade/ directory after an assessment.
> > User can see after a upgrade/migration what results mean.
> > But I will corrected it after package review so that only one README file is
> > enough.
> I think it is fine as is. You have to be careful here because of the rule
> that packages cannot require anything in %doc at runtime. So having two
> copies of this very small file is probably the best option.
> 
> > > - Looking at the first message:
> > > The Preupgrade Assistant is a diagnostics tool 
> > > and does not perform the actual upgrade.
> > > Make sure you back up your system and all of your data now,
> > > before using the Upgrade Tool to avoid potential data loss.
> > > Do you want to continue? y/n
> > > 
> > > If it is only a diagnostic tool, why is it unsafe to run?
> > > Also, why does it require root privileges? Isn't the RPM database public?
> > We need to have a access to all files and directories. Root access is really
> > needed. Some data are accessible only under root account.
> OK, so it needs to run as root. By why the scary warning?
> 
For Fedora system modified so that only first sentence can be mentioned, though.
> > > - In the man page: "All common log files are stored in
> > > /var/cache/preupgrade/common". Isn't /var/log/preupgrade used?
> > Those directories are different.
> > - /var/log/preupgrade directory is used for logs generated by
> > preupgrade-assistant itself as a program
> > - /var/cache/preupgrade/common directory contains special files used by
> > contents
> OK.
> 
> > > - I installed the rpm on F21. /usr/bin/preupg fails with:
> > > Traceback (most recent call last):
> > >   File "/usr/bin/preupg", line 8, in <module>
> > >     from preup.application import Application
> > >   File "/usr/lib/python2.7/site-packages/preup/application.py", line 18, in
> > > <module>
> > >     from preuputils.compose import XCCDFCompose
> > > ImportError: No module named preuputils.compose
> > > 
> > > Installing also -devel subpackage fixes that. Maybe you should merge -devel
> > > back into the main package. They are both really tiny, so the split is
> > > probably not worth the trouble.
> > > 
> > Merged
> Hm, I don't see this. In the spec file there's still "%package devel".
> 
Deleted all. Only preupgrade-assistant package exists.
> > > - This package seems useless without preupgrade-assistant-contents. Is this
> > > available somewhere?
> > 
> > Draft for Packaging Guidelines is already finished
> > (https://fedoraproject.org/wiki/User:Phracek/Draft:Packaging:
> > PreupgradeAssistant)
> > Currently now I would like to add it to
> > http://fedoraproject.org/wiki/Packaging:
> > Guidelines#Application_Specific_Guidelines
> > 
> > I will send a mail to devel list which describes how to create a content for
> > preupgrade-assistant.
> > 
> > I will create several bugzilla's to specific components for creating
> > contents. Likie mariadb, postgresql, etc.
> OK, thanks.
> 
> Two comments on the guidelines:
> The Python example does not take into account the Python2/Python3 split. I
> think you must explain that the script must run under Python2 in F22 and
> under Python3 in F23 (and have #!/usr/bin/python3). Alternatively, you could
> package the module for Python2 in F23 too, in a python-preupgrade-assistant
> subpackage, to allow people to use Python2 for the scripts in F22 and F23,
> or Python2 in F22 and Python3 in F23.
> 
> %doc %{preupgrade_dir}/%{name}/*.txt → this looks wrong, packages are not
> allowed to use files in %doc at runtime. The whole section can be simplified
> to:
> %files -n preupgrade-assistant-%{name}
> %{preupgrade_dir}/%{name}/

Can be.

I think you didn't upload a new version of the spec and srpm. srpm returns 404.

All works fine. I am able to download them.

Could you please repeat it?

Ah, OK, fedora-review was confused because the link to the previous version is quoted below your latest link. *I* was confused because you forgot to update the spec file :). But I can pull it out of the srpm.

In macros:
%preupgrade_name Fedora%{preupg_number}_%postupg_number
%preupgrade_dir /usr/share/preupgrade/%fedora_preupgrade_name ← name does not match

Requires:       openscap%{?_isa} >= 0:1.0.8-1
...

Why %{?_isa}? This would make this package arch-dependent, and should be dropped from all Requires.
Packaging Guidelines currently forbid using %{_isa} in BR. There's a ticket open to relax this, but in this case I don't think there's any benefit to having it, so those should be dropped too. 

> For Fedora system modified so that only first sentence can be mentioned, though.
I still see the old text.

===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Unknown or generated". 41 files have unknown license. Detailed output of
     licensecheck in /var/tmp/review-preupgrade-assistant/licensecheck.txt
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[!]: Package uses nothing in %doc for runtime.
%doc %{_datadir}/preupgrade/README.kickstart     ← remove %doc
%doc %{_datadir}/preupgrade/README               ← remove %doc

Also:
%dir %{_docdir}/preupgrade                       ← add %doc here, otherwise the directory would be created even if package is installed without doc files.
%doc %{_docdir}/preupgrade/README


[x]: Package consistently uses macros (instead of hard-coded directory names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[ ]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[x]: Python eggs must not download any dependencies during the build process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[ ]: Package contains BR: python2-devel or python3-devel
BR: python-devel should be changed to python2-devel.

[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[ ]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[!]: Spec use %global instead of %define unless justified.
     Note: %define requiring justification: %define egg_name %(echo %{name} |
     sed s/-/_/)
Yep, %define should be replaced with %global.

[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.

===== EXTRA items =====

Generic:
[!]: Spec file according to URL is the same as in SRPM.
     Note: Spec file as given by url is not the same as in SRPM (see attached
     diff).
     See: (this test has no URL)
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).


Rpmlint
-------
Checking: preupgrade-assistant-0.11.7-5.fc23.noarch.rpm
          preupgrade-assistant-0.11.7-5.fc23.src.rpm
preupgrade-assistant.noarch: W: spelling-error %description -l en_US upgradeability -> upgrade ability, upgrade-ability, biodegradability
preupgrade-assistant.noarch: W: spelling-error %description -l en_US inplace -> in place, in-place, Laplace
preupgrade-assistant.noarch: W: spelling-error %description -l en_US postupgrade -> post upgrade, post-upgrade, postgraduate
Bogus.

preupgrade-assistant.noarch: W: log-files-without-logrotate /var/log/preupgrade
The package is fine without log rotation, because it is only invoked manually.

OTOH, I'd much prefer if it logged to the journal.

preupgrade-assistant.noarch: W: no-manual-page-for-binary preupg-create-group-xml
preupgrade-assistant.noarch: W: no-manual-page-for-binary preupg-xccdf-compose
preupgrade-assistant.src: W: spelling-error %description -l en_US upgradeability -> upgrade ability, upgrade-ability, biodegradability
preupgrade-assistant.src: W: spelling-error %description -l en_US inplace -> in place, in-place, Laplace
preupgrade-assistant.src: W: spelling-error %description -l en_US postupgrade -> post upgrade, post-upgrade, postgraduate

preupgrade-assistant.src:174: W: macro-in-%changelog %license
Yes, please double the percent sign.

2 packages and 0 specfiles checked; 0 errors, 10 warnings.



So, I gave the three binaries a spin:
/usr/bin/preupg → fails because of lack of content in /usr/share/preupgrade. OK for now.

$ /usr/bin/preupg-create-group-xml
Traceback (most recent call last):
  File "/usr/bin/preupg-create-group-xml", line 84, in <module>
    main()
  File "/usr/bin/preupg-create-group-xml", line 30, in main
    if not os.path.exists(args[0]):
IndexError: list index out of range

$ /usr/bin/preupg-xccdf-compose 
Traceback (most recent call last):
  File "/usr/bin/preupg-xccdf-compose", line 24, in <module>
    main()
  File "/usr/bin/preupg-xccdf-compose", line 20, in main
    xccdf_compose = XCCDFCompose(args[0])
IndexError: list index out of range

Not a biggie, but it would be nice if they printed a nicer error than a backtrace.

Final URLs:
Spec URL: https://phracek.fedorapeople.org/preupgrade-assistant.spec
SRPM URL: https://phracek.fedorapeople.org/preupgrade-assistant-0.11.7-6.fc21.src.rpm

Hopefully review is finally finished.
(In reply to Zbigniew Jędrzejewski-Szmek from comment #26)
> Ah, OK, fedora-review was confused because the link to the previous version
> is quoted below your latest link. *I* was confused because you forgot to
> update the spec file :). But I can pull it out of the srpm.
> 
> In macros:
> %preupgrade_name Fedora%{preupg_number}_%postupg_number
> %preupgrade_dir /usr/share/preupgrade/%fedora_preupgrade_name ← name does
> not match
> 
Fixed
> Requires:       openscap%{?_isa} >= 0:1.0.8-1
> ...
> 
Fixed
> Why %{?_isa}? This would make this package arch-dependent, and should be
> dropped from all Requires.
> Packaging Guidelines currently forbid using %{_isa} in BR. There's a ticket
> open to relax this, but in this case I don't think there's any benefit to
> having it, so those should be dropped too. 
> 
> > For Fedora system modified so that only first sentence can be mentioned, though.
> I still see the old text.
> 
Fixed
> ===== MUST items =====
> 
> Generic:
> [x]: Package is licensed with an open-source compatible license and meets
>      other legal requirements as defined in the legal section of Packaging
>      Guidelines.
> [x]: License field in the package spec file matches the actual license.
>      Note: Checking patched sources after %prep for licenses. Licenses found:
>      "Unknown or generated". 41 files have unknown license. Detailed output
> of
>      licensecheck in /var/tmp/review-preupgrade-assistant/licensecheck.txt
> [x]: Package contains no bundled libraries without FPC exception.
> [x]: Changelog in prescribed format.
> [x]: Sources contain only permissible code or content.
> [-]: Package contains desktop file if it is a GUI application.
> [-]: Development files must be in a -devel package
> [!]: Package uses nothing in %doc for runtime.
> %doc %{_datadir}/preupgrade/README.kickstart     ← remove %doc
> %doc %{_datadir}/preupgrade/README               ← remove %doc
> 
> Also:
> %dir %{_docdir}/preupgrade                       ← add %doc here, otherwise
> the directory would be created even if package is installed without doc
> files.
> %doc %{_docdir}/preupgrade/README
> 
Fixed
> 
> [x]: Package consistently uses macros (instead of hard-coded directory
> names).
> [x]: Package is named according to the Package Naming Guidelines.
> [x]: Package does not generate any conflict.
> [x]: Package obeys FHS, except libexecdir and /usr/target.
> [-]: If the package is a rename of another package, proper Obsoletes and
>      Provides are present.
> [x]: Requires correct, justified where necessary.
> [x]: Spec file is legible and written in American English.
> [-]: Package contains systemd file(s) if in need.
> [x]: Package is not known to require an ExcludeArch tag.
> [-]: Large documentation must go in a -doc subpackage. Large could be size
>      (~1MB) or number of files.
>      Note: Documentation size is 10240 bytes in 1 files.
> [ ]: Package complies to the Packaging Guidelines
> [x]: Package successfully compiles and builds into binary rpms on at least
> one
>      supported primary architecture.
> [x]: Package installs properly.
> [x]: Rpmlint is run on all rpms the build produces.
>      Note: There are rpmlint messages (see attachment).
> [x]: Package requires other packages for directories it uses.
> [x]: Package must own all directories that it creates.
> [x]: Package does not own files or directories owned by other packages.
> [x]: All build dependencies are listed in BuildRequires, except for any that
>      are listed in the exceptions section of Packaging Guidelines.
> [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
> [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
>      beginning of %install.
> [x]: Macros in Summary, %description expandable at SRPM build time.
> [x]: Package does not contain duplicates in %files.
> [x]: Permissions on files are set properly.
> [x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
>      work.
> [x]: Package is named using only allowed ASCII characters.
> [x]: Package do not use a name that already exist
> [x]: Package is not relocatable.
> [x]: Sources used to build the package match the upstream source, as provided
>      in the spec URL.
> [x]: Spec file name must match the spec package %{name}, in the format
>      %{name}.spec.
> [x]: File names are valid UTF-8.
> [x]: Packages must not store files under /srv, /opt or /usr/local
> 
> Python:
> [x]: Python eggs must not download any dependencies during the build process.
> [x]: A package which is used by another package via an egg interface should
>      provide egg info.
> [x]: Package meets the Packaging Guidelines::Python
> [ ]: Package contains BR: python2-devel or python3-devel
> BR: python-devel should be changed to python2-devel.
> 
Fixed
> [x]: Binary eggs must be removed in %prep
> 
> ===== SHOULD items =====
> 
> Generic:
> [-]: If the source package does not include license text(s) as a separate
> file
>      from upstream, the packager SHOULD query upstream to include it.
> [x]: Final provides and requires are sane (see attachments).
> [ ]: Package functions as described.
> [x]: Latest version is packaged.
> [x]: Package does not include license text files separate from upstream.
> [-]: Patches link to upstream bugs/comments/lists or are otherwise justified.
> [-]: Description and summary sections in the package spec file contains
>      translations for supported Non-English languages, if available.
> [x]: Package should compile and build into binary rpms on all supported
>      architectures.
> [x]: %check is present and all tests pass.
> [x]: Packages should try to preserve timestamps of original installed files.
> [!]: Spec use %global instead of %define unless justified.
>      Note: %define requiring justification: %define egg_name %(echo %{name} |
>      sed s/-/_/)
> Yep, %define should be replaced with %global.
Fixed
> 
> [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
> [x]: Sources can be downloaded from URI in Source: tag
> [x]: Reviewer should test that the package builds in mock.
> [x]: Buildroot is not present
> [x]: Package has no %clean section with rm -rf %{buildroot} (or
>      $RPM_BUILD_ROOT)
> [x]: Dist tag is present (not strictly required in GL).
> [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
> [x]: SourceX is a working URL.
> 
> ===== EXTRA items =====
> 
> Generic:
> [!]: Spec file according to URL is the same as in SRPM.
>      Note: Spec file as given by url is not the same as in SRPM (see attached
>      diff).
>      See: (this test has no URL)
> [x]: Rpmlint is run on all installed packages.
>      Note: There are rpmlint messages (see attachment).
> 
> 
> Rpmlint
> -------
> Checking: preupgrade-assistant-0.11.7-5.fc23.noarch.rpm
>           preupgrade-assistant-0.11.7-5.fc23.src.rpm
> preupgrade-assistant.noarch: W: spelling-error %description -l en_US
> upgradeability -> upgrade ability, upgrade-ability, biodegradability
> preupgrade-assistant.noarch: W: spelling-error %description -l en_US inplace
> -> in place, in-place, Laplace
> preupgrade-assistant.noarch: W: spelling-error %description -l en_US
> postupgrade -> post upgrade, post-upgrade, postgraduate
> Bogus.
> 
> preupgrade-assistant.noarch: W: log-files-without-logrotate
> /var/log/preupgrade
> The package is fine without log rotation, because it is only invoked
> manually.
> 
> OTOH, I'd much prefer if it logged to the journal.
> 
> preupgrade-assistant.noarch: W: no-manual-page-for-binary
> preupg-create-group-xml
Will be fixed later on
> preupgrade-assistant.noarch: W: no-manual-page-for-binary
> preupg-xccdf-compose
Will be fixed later on
> preupgrade-assistant.src: W: spelling-error %description -l en_US
> upgradeability -> upgrade ability, upgrade-ability, biodegradability
> preupgrade-assistant.src: W: spelling-error %description -l en_US inplace ->
> in place, in-place, Laplace
> preupgrade-assistant.src: W: spelling-error %description -l en_US
> postupgrade -> post upgrade, post-upgrade, postgraduate
> 
> preupgrade-assistant.src:174: W: macro-in-%changelog %license
> Yes, please double the percent sign.
> 
> 2 packages and 0 specfiles checked; 0 errors, 10 warnings.
> 
> 
> 
> So, I gave the three binaries a spin:
> /usr/bin/preupg → fails because of lack of content in /usr/share/preupgrade.
> OK for now.
> 
> $ /usr/bin/preupg-create-group-xml
> Traceback (most recent call last):
>   File "/usr/bin/preupg-create-group-xml", line 84, in <module>
>     main()
>   File "/usr/bin/preupg-create-group-xml", line 30, in main
>     if not os.path.exists(args[0]):
> IndexError: list index out of range
> 
> $ /usr/bin/preupg-xccdf-compose 
> Traceback (most recent call last):
>   File "/usr/bin/preupg-xccdf-compose", line 24, in <module>
>     main()
>   File "/usr/bin/preupg-xccdf-compose", line 20, in main
>     xccdf_compose = XCCDFCompose(args[0])
> IndexError: list index out of range
> 
> Not a biggie, but it would be nice if they printed a nicer error than a
> backtrace.

I am going to fix that after the review.

fedora-review has nothing interesting to say, except
> preupgrade-assistant.src:174: W: macro-in-%changelog %license

Rpmlint
-------
Checking: preupgrade-assistant-0.11.7-6.fc23.noarch.rpm
          preupgrade-assistant-0.11.7-6.fc23.src.rpm
preupgrade-assistant.noarch: W: spelling-error %description -l en_US upgradeability -> upgrade ability, upgrade-ability, biodegradability
preupgrade-assistant.noarch: W: spelling-error %description -l en_US inplace -> in place, in-place, Laplace
preupgrade-assistant.noarch: W: spelling-error %description -l en_US postupgrade -> post upgrade, post-upgrade, postgraduate
preupgrade-assistant.noarch: W: log-files-without-logrotate /var/log/preupgrade
preupgrade-assistant.noarch: W: no-manual-page-for-binary preupg-create-group-xml
preupgrade-assistant.noarch: W: no-manual-page-for-binary preupg-xccdf-compose
preupgrade-assistant.src: W: spelling-error %description -l en_US upgradeability -> upgrade ability, upgrade-ability, biodegradability
preupgrade-assistant.src: W: spelling-error %description -l en_US inplace -> in place, in-place, Laplace
preupgrade-assistant.src: W: spelling-error %description -l en_US postupgrade -> post upgrade, post-upgrade, postgraduate
preupgrade-assistant.src:175: W: macro-in-%changelog %license
2 packages and 0 specfiles checked; 0 errors, 10 warnings.

Requires
--------
preupgrade-assistant (rpmlib, GLIBC filtered):
    /bin/bash
    /usr/bin/python3
    bash
    coreutils
    findutils
    gawk
    grep
    openscap
    openscap-engine-sce
    openscap-utils
    python(abi)
    python3-kickstart
    python3-requests
    python3-setuptools
    python3-six
    rpm-python3
    sed

Provides
--------
preupgrade-assistant:
    preupgrade-assistant


Package is APPROVED.

New Package SCM Request
=======================
Package Name: preupgrade-assistant
Short Description: Preupgrade assistant a tool for assess system before an upgrade
Upstream URL: https://github.com/phracek/preupgrade-assistant
Owners: phracek
Branches: f22

Typo;)

New Package SCM Request
=======================
Package Name: preupgrade-assistant
Short Description: A tool to assess system before an upgrade
Upstream URL: https://github.com/phracek/preupgrade-assistant
Owners: phracek
Branches: f22

Git done (by process-git-requests).

scm-commits rawhide: https://lists.fedoraproject.org/pipermail/scm-commits/Week-of-Mon-20150309/1534131.html
scm-commits F22: https://lists.fedoraproject.org/pipermail/scm-commits/Week-of-Mon-20150309/1534138.html

preupgrade-assistant-0.11.7-1.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/preupgrade-assistant-0.11.7-1.fc22

preupgrade-assistant-0.11.7-1.fc22 has been pushed to the Fedora 22 stable repository.