Links give 404.

I think you should post the openqa package for review too. Sometimes it's easier to review dependent packages in parallel.

sorry, correct links:

https://www.happyassassin.net/reviews/os-autoinst/os-autoinst.spec
https://www.happyassassin.net/reviews/os-autoinst/os-autoinst-4.2.1-5.fc24.src.rpm

openqa can't be done until https://bugzilla.redhat.com/show_bug.cgi?id=1267037 is done, as it depends on that.

You can always put the review request up, and add Depends on: 1267037. Parallelization is useful :)

Since it seems like it might help with QA, I'll do the review.

Fedora <= 21 is no more, so some conditions in the spec file can be simplified.

Group tag is not necessary.

s/Openvswitch/Open vSwitch/ in the summary, maybe.

Is R:git needed, or would just R:git-core be enough?

find %{buildroot} -type f -name .packlist -print0 | xargs -0 --no-run-if-empty rm -f
→ find %{buildroot} -type f -name .packlist -delete ?
or even %global NO_PACKLIST?

find %{buildroot} -depth -type d -and -not -name distri -print0 | xargs -0 --no-run-if-empty rmdir 2>/dev/null || true
→ find %{buildroot} -depth -type d -and -not -name distri -exec rmdir {} \; ?

Dunno, I'd just put %_libexedri/os-autoinst in %files, because it's going to be a pain on every upgrade...

https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd are required for os-autoinst-openvswitch.service.

fedora-review says:
- Package contains BR: python2-devel or python3-devel

- 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 is included in %license.
  Note: License file COPYING is marked as %doc instead of %license
  See:
  http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

[ ]: %config files are marked noreplace or the reason is justified.
     Note: No (noreplace) in %config
     /etc/dbus-1/system.d/org.opensuse.os_autoinst.switch.conf

This is an upstream issue, maybe you can report it:
[!]: Package should not use obsolete m4 macros
     Note: Some obsoleted macros found, see the attachment.
     See: https://fedorahosted.org/FedoraReview/wiki/AutoTools
AutoTools: Obsoleted m4s found
------------------------------
  AC_PROG_LIBTOOL found in: os-autoinst-
  5ce7dc56e6a7478e225f9018e02ed890a327cfbf/configure.ac:24

os-autoinst.x86_64: E: version-control-internal-file /usr/libexec/os-autoinst/distri/.gitignore

adamwill's scratch build of os-autoinst-4.2.1-6.fc24.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12292149

adamwill's scratch build of os-autoinst-4.2.1-6.fc24.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12292161

Thanks for the review! As a general note: the spec is based on the openSUSE spec, so most of the stuff you mention comes from that (e.g. the find commands).

"Fedora <= 21 is no more, so some conditions in the spec file can be simplified." - well...you can't just drop the Fedora bit because then the conditional would always fail for Fedora builds. We could just make it '%if 0%{?fedora} || 0%{?rhel} > 7', I guess, but that doesn't seem like a huge improvement. I think I'm just gonna leave it for now.

openSUSE still has Requires: git-core , but looking through the code I actually don't think there is any use of git at runtime any more, so I'm just going to drop that dep entirely.

The find stuff can be improved indeed (these again come direct from the openSUSE spec), I don't think I want to use NO_PACKLIST in case I ever manage to get all the bits built for EPEL 7. I have no idea whether the second 'find and destroy' command is even needed any more, I guess I should look and see what it actually does.

There are different philosophies on the file list - just including the entire directory is simpler but means you don't necessarily notice big changes to the package loadout. I have my own preferences, but at this point I'd actually like to simply follow what the openSUSE folks do, because it makes it easier to track their changes in our package.

I assume upstream has a reason to not mark the dbus config file as (noreplace), but OTOH we don't use any of that stuff in our openQA and I don't really care about it, so I'm fine with changing it :P

"- Package contains BR: python2-devel or python3-devel" - er, what? 'python' does not appear anywhere in the spec, nor is there any python in the package, so I don't know what this is about.

Revised spec and SRPM:

https://www.happyassassin.net/reviews/os-autoinst/os-autoinst.spec
https://www.happyassassin.net/reviews/os-autoinst/os-autoinst-4.2.1-6.fc24.src.rpm

Quick note, the %license compatibility macro is wrong. It should be "%{!?_licensedir:%global license %doc}" so that the %doc part is actually executed instead of being treated as a comment.

Yeah, %doc not %%doc. https://fedorahosted.org/fpc/ticket/586 has been filed.

+ license is OK (GPLv2+, this matches COPYING file and headers in individual files)
+ license file is present, %license is used
+ latest version
+ upstream and source links are OK
+ builds and installs OK
+ provides and requires look OK
+ rpm scriptlets look sane

- INSTALL.asciidoc is not useful for package users, can be skipped
- /usr/libexec/os-autoinst/dmidata calls /usr/bin/dmidecode, do you need R:dmidecode?
- os-autoinst-openvswitch.rpm should have R:%{name}{?_isa} = %{version}-%{release}
- rpmlint:
os-autoinst.src:107: W: macro-in-comment %license
os-autoinst.src:177: W: macro-in-%changelog %license
os-autoinst.x86_64: E: version-control-internal-file /usr/libexec/os-autoinst/distri/.gitignore

Suggestion: %autosetup would obviate the need for individual %patch lines.

Normally perl requires are autogenerated, but in this case all dependencies are also in BuildRequires, so it makes sense to use the same list for both. So doing it manually is fine. At least that's my understanding, I'm not much of a perl person.

/var/lib/os-autoinst/audio/aplay-1.wav → shouldn't this be /usr/share/os-autoinst?

/usr/libexec/os-autoinst → shouldn't this be in /usr/lib64/perl5/vendor_perl ?

"Suggestion: %autosetup would obviate the need for individual %patch lines."

Yeah, it would, I can switch over at some point.

"Normally perl requires are autogenerated, but in this case all dependencies are also in BuildRequires, so it makes sense to use the same list for both. So doing it manually is fine. At least that's my understanding, I'm not much of a perl person."

More or less, yeah. I think we also get auto-generated requires, but it all works fine (duplicated requires aren't really a big deal). Again this is something that comes from the SUSE spec (and is especially nice to keep in line with upstream as it lets me catch when they add/remove dependencies easily).

"/usr/libexec/os-autoinst → shouldn't this be in /usr/lib64/perl5/vendor_perl"

That would be an upstream decision really (upstream explicitly installs to libexecdir), but I think for one thing os-autoinst is not intended to be used as a vendor module. If you want to ask upstream I can hook you up with them...

"/var/lib/os-autoinst/audio/aplay-1.wav → shouldn't this be /usr/share/os-autoinst?"

possibly? I don't actually know what it's for. Again, though, if that's going to change it would make the most sense to patch upstream's install routine, I much prefer that to relocating things in downstream packages.

"os-autoinst.x86_64: E: version-control-internal-file /usr/libexec/os-autoinst/distri/.gitignore"

I suspect this isn't in fact an error (openQA's general design expects various bits to be checked out of git, and thus including .gitignores isn't necessarily a mistake), but I think it may be functionally obsolete at this point. I'll check with upstream.

I'll update this probably tomorrow.

(In reply to awilliam from comment #11)
> "/usr/libexec/os-autoinst → shouldn't this be in
> /usr/lib64/perl5/vendor_perl"
> 
> That would be an upstream decision really (upstream explicitly installs to
> libexecdir), but I think for one thing os-autoinst is not intended to be
> used as a vendor module. If you want to ask upstream I can hook you up with
> them...
> 
> "/var/lib/os-autoinst/audio/aplay-1.wav → shouldn't this be
> /usr/share/os-autoinst?"
> 
> possibly? I don't actually know what it's for. Again, though, if that's
> going to change it would make the most sense to patch upstream's install
> routine, I much prefer that to relocating things in downstream packages.

Neither is a big deal, but at least the second one is a FHS violation.
It's not a big file, so if you tell me that it would be more than a
small amount of work or otherwise problematic to move (e.g. because that's
the upstream documented path), then I'm fine with leaving it as is.

I'm going to ask upstream about the decision to default to libexecdir for the perl bits. It would in fact be quite easy to install them anywhere else at all (we'd just have to redefine pkglibexecdir for the make steps), but I'd like to talk it over with them before changing it.

aplay-1.wav is a bit mysterious: so far as I can tell, it really doesn't have any reason to exist. It doesn't seem to be used for anything. I opened a ticket upstream to ask if there's any reason for its existence: https://github.com/os-autoinst/os-autoinst/issues/385

"- /usr/libexec/os-autoinst/dmidata calls /usr/bin/dmidecode, do you need R:dmidecode?"

no, I don't think so; the tool seems to be there only for admins to use manually in a fairly specific situation (you want to have the VM pretend to be a laptop) - see https://github.com/os-autoinst/os-autoinst/commit/0b8f9bf75558add140a4d455a9f857fde6644127 . A requires doesn't seem reasonable.

os-autoinst 4.3 came out recently. I've rebased the spec to that, and addressed a lot of the issues raised. However, I noticed a significant problem with the 'perl bits in libexecdir stuff' which I hadn't noticed before. The perl auto-provides / auto-requires filters produce provides and requires for all those perl bits, so os-autoinst has a bunch of bogus provides: and requires: like this:

    perl(autotest)
    perl(backend::baseclass)
    perl(backend::console_proxy)
    perl(backend::driver)
    perl(backend::ipmi)
    perl(backend::qemu)
    perl(backend::s390x)
    perl(backend::svirt)

etc etc. They're bogus because they're not in the standard location, obviously - any other package which actually wanted a perl module called backend::qemu or whatever wouldn't actually be able to load it in the standard fashion (iso2video - the main executable in os-autoinst - futzes about with the perl include path to load the modules).

So I'm inclined to agree with you that the installation to libexecdir should just be considered wrong, or at the very least, a bad idea (I'm guessing SUSE doesn't use perl auto-provides / auto-requires). I've opened a ticket upstream to discuss this:

https://github.com/os-autoinst/os-autoinst/issues/387

Here's the latest spec and .src.rpm. For now I'm just leaving the bogus provides/requires in, so at least the package *works*, but I agree it probably shouldn't pass review until this is resolved.

https://www.happyassassin.net/reviews/os-autoinst/os-autoinst.spec
https://www.happyassassin.net/reviews/os-autoinst/os-autoinst-4.3-1.fc24.src.rpm

Without further ado, continuing from comment #8:
+ latest version (4.3)
+ provides/requires look sine (with the patch below)
+ rpmlint has nothing interesting to say

Just add the following:

+%global __provides_exclude_from %{_libexecdir}/os-autoinst
 %{?perl_default_filter}
-%global __requires_exclude_from %{_docdir}
-# FIXME: there are a bunch of auto-generated provides that we should
-# really filter, but doing so is hard because we also auto-*require*
-# them, and I don't want a gigantic auto-require filter. Probably we
-# should get upstream to install to the perl vendor dir, properly
-# namespaced, so the auto-provides and auto-requires won't be dirty
-# dirty lies.

[https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering#Perl,
%_docdir is already filtered by default].

rpmlint:
os-autoinst.x86_64: W: spelling-error %description -l en_US bootloader -> boot loader, boot-loader, boatload
os-autoinst.x86_64: E: script-without-shebang /usr/libexec/os-autoinst/consoles/localXvnc.pm
...
This is because those files are executable. It's never particularly important,
and especially here since the installation of those files might change anyway.

os-autoinst.x86_64: E: non-standard-executable-perm /usr/lib64/perl5/vendor_perl/auto/tinycv/tinycv.so 555
Doesn't matter.

os-autoinst.x86_64: W: no-manual-page-for-binary isotovideo
os-autoinst.x86_64: W: no-manual-page-for-binary debugviewer
os-autoinst.x86_64: W: no-manual-page-for-binary snd2png
os-autoinst-openvswitch.x86_64: W: spelling-error Summary(en_US) vSwitch -> v Switch, switch, vs witch
os-autoinst-openvswitch.x86_64: W: spelling-error %description -l en_US vSwitch -> v Switch, switch, vs witch
os-autoinst-openvswitch.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 30 errors, 8 warnings.

All OK.

Package is APPROVED.

"Just add the following:"

Nope, that's not good enough - because then the package still *requires* all those things, and nothing provides them. Filtering out the provides is the easy part; filtering the requires is much harder. Upstream doesn't seem to be sold on moving the modules, though, so I'll have to do it somehow.

(You can of course do:

%global __provides_exclude_from %{_libexecdir}/os-autoinst
%global __requires_exclude_from %{_libexecdir}/os-autoinst

but then you lose all the *genuine* auto-requires as well). On %docdir - I can read wiki pages too, but I don't just throw lines in spec files for no reason, and without that line, I do get spurious Requires: from files in docdir, perhaps because the files that go there get installed by 'make install', they're not listed in %doc in the more common fashion.

Right. Something like this seems to work:
%global __provides_exclude_from %{_libexecdir}/os-autoinst
%global __requires_exclude ^perl\\((autotest|backend|basetest|bmwqemu|commands|consoles|cv|distribution|lockapi|mmapi|needle|ocr|testapi)
%{?perl_default_filter}

It's not very pretty, but at least if upstream adds more modules, the package will become uninstallable, so it cannot become out of date without someone noticing.


Please add:
Requires:  perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))


> On %docdir - I can read wiki pages too, but I don't just throw lines in spec files for no reason

I don't throw comments in my review for no good reason either ;)
I checked with rpmbuild --showrc that %perl_default_filter has
%global __requires_exclude_from %{?__requires_exclude_from:%__requires_exclude_from|}^%{_docdir}
so unless there's some bug, this line should not be necessary.
rpmdiff also says that this line makes no difference.

welp, maybe it changed. eh.

I really hate that regex. I'm going to look at how the filters are implemented and see if I can suggest a new one; it would seem to make sense to have one which does 'don't generate any Provides: from these files, and don't generate any Requires: for those Provides: either' - I'm sure this can't be the only package which has that problem.

Oh, now I remember why I had it that way.

If you read the wiki page, it tells you to do %{?perl_default_filter} first, then redefine any of the filters you want to override, including the original definition, as per their example:

%{?perl_default_filter}
%global __requires_exclude perl\\(VMS|perl\\(Win32|my_additional_pattern

That's what I was doing. In 4.2 I needed to exclude provides from another file, so the block looked like this:

%{?perl_default_filter}
%global __requires_exclude_from %{_docdir}|%{_libexecdir}/os-autoinst/autoinstallstep.pm

in 4.3, autoinstallstep.pm is gone, so I removed that file from the regex, which just left this:

%{?perl_default_filter}
%global __requires_exclude_from %{_docdir}

which as you pointed out is silly because it's just recreating the definition from perl_default_filter - but now I remember how it got that way.

The block you suggest won't work, because %{?perl_default_filter} will just override the __provides_exclude_from and __requires_exclude definitions above it. As per the wiki page we'll have to do perl_default_filter first, then redefine __provides_exclude_from and __requires_exclude, including the default exclusions along with the ones we need. (I just looked into whether my exclude-requires-from-this-file-and-also-dont-generate-provides-for-them idea would work and unfortunately it doesn't look simple, because the generation of provides and requires is done separately and you can't really pass information from one to the other very easily).

> The block you suggest won't work
Nah, the wiki is wrong. Somebody updated the macros to merge with previous values (see the example in comment #17). Somebody should file a FPC ticket to update the wiki.

oh, that's handy, if it works - I'll double-check. Thanks.

https://fedorahosted.org/fpc/ticket/591

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/os-autoinst

Now built for Rawhide and update pending for 23. Thanks a lot for the detailed review! I really appreciate it. openQA coming soon. :)

openQA review request: https://bugzilla.redhat.com/show_bug.cgi?id=1304882