[Bug 1288731] New: Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

Bug ID: 1288731
           Summary: Review Request: os-autoinst - OS-level test automation
           Product: Fedora
           Version: rawhide
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]

Spec URL: https://www.happyassassin.net/reviews/os-autoinst.spec
SRPM URL:
https://www.happyassassin.net/reviews/os-autoinst-4.2.1-5.fc24.src.rpm
Description: The OS-autoinst project aims at providing a means to run fully
automated tests. Especially to run tests of basic and low-level operating
system components such as bootloader, kernel, installer and upgrade, which can
not easily and safely be tested with other automated testing frameworks.
However, it can just as well be used to test applications on top of a newly
installed OS. NOTE: os-autoinst is the test runner for openQA, which we are
working to include in the official repos so we no longer need to use a COPR for
the Fedora openQA deployments. This build is in the COPR -
https://copr.fedoraproject.org/coprs/adamwill/openQA - and is already being
used by the workers for the Fedora openQA deployment.
Fedora Account System Username: adamwill

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

Zbigniew Jędrzejewski-Szmek  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from Zbigniew Jędrzejewski-Szmek  ---
Links give 404.

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

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #2 from [e-mail]  ---
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.s...

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

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #3 from Zbigniew Jędrzejewski-Szmek  ---
You can always put the review request up, and add Depends on: 1267037.
Parallelization is useful :)

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

Zbigniew Jędrzejewski-Szmek  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|[e-mail]    |[e-mail]
              Flags|                            |fedora-review?

--- Comment #4 from Zbigniew Jędrzejewski-Szmek  ---
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

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #5 from Upstream Release Monitoring  ---
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

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #6 from Upstream Release Monitoring  ---
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

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #7 from [e-mail]  ---
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.s...

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

Neal Gompa  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #8 from Neal Gompa  ---
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.

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #9 from Zbigniew Jędrzejewski-Szmek  ---
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

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #10 from Zbigniew Jędrzejewski-Szmek  ---
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 ?

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #11 from [e-mail]  ---
"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.

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #12 from Zbigniew Jędrzejewski-Szmek  ---
(In reply to [e-mail] 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.

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #13 from [e-mail]  ---
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

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #14 from [e-mail]  ---
"- /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/0b8f9bf75558add140a4d455a9...
. 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...

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

Zbigniew Jędrzejewski-Szmek  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |POST
              Flags|fedora-review?              |fedora-review+

--- Comment #15 from Zbigniew Jędrzejewski-Szmek  ---
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#Pe...,
%_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.

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #16 from [e-mail]  ---
"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.

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #17 from Zbigniew Jędrzejewski-Szmek  ---
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.

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #18 from [e-mail]  ---
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.

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #19 from [e-mail]  ---
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).

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #20 from Zbigniew Jędrzejewski-Szmek  ---
> 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.

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #21 from [e-mail]  ---
oh, that's handy, if it works - I'll double-check. Thanks.

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #22 from Zbigniew Jędrzejewski-Szmek  ---
https://fedorahosted.org/fpc/ticket/591

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #23 from Jon Ciesla  ---
Package request has been approved:
https://admin.fedoraproject.org/pkgdb/package/os-autoinst

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

[e-mail]  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|POST                        |CLOSED
         Resolution|---                         |RAWHIDE
        Last Closed|                            |2016-01-21 15:05:38

--- Comment #24 from [e-mail]  ---
Now built for Rawhide and update pending for 23. Thanks a lot for the detailed
review! I really appreciate it. openQA coming soon. :)

[Bug 1288731] Review Request: os-autoinst - OS-level test automation

[Red Hat Bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1288731

--- Comment #25 from [e-mail]  ---
openQA review request: https://bugzilla.redhat.com/show_bug.cgi?id=1304882