Bug 838901 - Review Request: autotest-framework - Framework for fully automated testing
Review Request: autotest-framework - Framework for fully automated testing
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Jiri Popelka
Fedora Extras Quality Assurance
:
: 548522 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-10 08:02 EDT by Martin Krizek
Modified: 2012-11-20 11:23 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-11-20 11:09:51 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jpopelka: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Martin Krizek 2012-07-10 08:02:45 EDT
Spec URL: http://mkrizek.fedorapeople.org/autotest.spec
SRPM URL: http://mkrizek.fedorapeople.org/autotest-0.14.2-1.fc17.src.rpm
Description: Autotest is a framework for fully automated testing
Fedora Account System Username: mkrizek

I worked with upstream developers to make Autotest more packaging friendly and *a lot* of patches has been pushed upstream, however, there is a one patchset included in the spec file that is already upstream but hasn't been included in the latest release. The rest of the spec file patches are packaging only.

Autotest web frontend depends on Google Web Toolkit devel package (gwt-devel) and since gwt-devel is not in official Fedora repository, the web frontend was moved to autotest-web subpackage that is built only when "--with gwt".

I opened an issue on upstream bug tracker to discuss adding man pages:
https://github.com/autotest/autotest/issues/445
Comment 1 Martin Krizek 2012-07-10 08:03:36 EDT
*** Bug 548522 has been marked as a duplicate of this bug. ***
Comment 2 Ralf Corsepius 2012-07-10 10:47:40 EDT
This package's name conflicts with a tool of the same name, which is part of autoconf for ca. a decade.

I'd therefore rather not see the package pass a review without a renamer.
Comment 3 Lucas Meneghel Rodrigues 2012-07-11 16:56:17 EDT
Martin,

Then the package could be named autotest-kernel then... it's not the best name ever, but well...
Comment 4 Martin Krizek 2012-07-12 03:30:56 EDT
What about autotest-framework?
Comment 5 Martin Krizek 2012-07-20 06:12:03 EDT
I renamed the package to 'autotest-framework'. New versions of the spec file and SRPM follows:

Spec URL: http://mkrizek.fedorapeople.org/autotest-framework.spec
SRPM URL: http://mkrizek.fedorapeople.org/autotest-framework-0.14.2-2.fc17.src.rpm
Comment 6 Lucas Meneghel Rodrigues 2012-07-23 13:17:22 EDT
I can't say I'm a big fan of autotest-framework, but well, it seems as good as anything. Thanks Martin.
Comment 7 Lucas Meneghel Rodrigues 2012-07-27 14:15:34 EDT
Any updates on this one, folks? We're happy to work on any issues you guys might find out.
Comment 8 Jiri Popelka 2012-08-03 11:35:42 EDT
Package Review
==============

Key:
- = N/A
x = Pass
! = Fail

==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Sources contain only permissible code or content.
[x]: MUST %config files are marked noreplace or the reason is justified.
[-]: MUST Macros in Summary, %description expandable at SRPM build time.
[-]: MUST Package contains desktop file if it is a GUI application.
[-]: MUST Development files must be in a -devel package
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Fully versioned dependency in subpackages, if present.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[-]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[!]: MUST License field in the package spec file matches the actual license.

- LGPLv2.1+ isn't a correct short name. Use LGPLv2+ instead.
- client/shared/pexpect.py and frontend/shared/json_html_formatter.py are under
MIT license, see https://fedoraproject.org/wiki/Licensing/MIT#Modern_Style_with_sublicense

[x]: MUST License file installed when any subpackage combination is installed.
[x]: MUST Package consistently uses macro.
[x]: MUST Package is named using only allowed ascii characters.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST No %config files under /usr.
[x]: MUST Package does not generate any conflict.
[!]: MUST Package obeys FHS.

These "FHS issues" were discussed in bug #548522. Can you make a summary here
what's been the progress since then.

[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Package is not relocatable.
[x]: MUST Requires correct, justified where necessary.
[!]: MUST Rpmlint is run on all rpms the build produces.

The output is quite long but most of it is about non-standard-uid/gid of
various files, which is caused by %defattr(-,autotest,autotest,-).
It was discussed in bug #548522 too.
Would it be possible to make a summary here, why is it set so ?

There's however one thing that could be fixed easily I think:
W: devel-file-in-non-devel-package /usr/lib/python2.7/site-packages/autotest/client/tools/setidle.c

[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: MUST File names are valid UTF-8.
[!]: MUST Package contains systemd file(s) if in need.

You need to obey https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#New_Packages

[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX is a working URL.
[x]: SHOULD Spec use %global instead of %define.


Note:
If you don't aim at EPEL, then you can safely remove:
BuildRoot and python_sitelib definition, %clean section, cleaning of buildroot in %install, see
http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
http://fedoraproject.org/wiki/Packaging/Guidelines#.25clean
https://fedoraproject.org/wiki/Packaging:Python#Macros
Comment 9 Lucas Meneghel Rodrigues 2012-08-03 11:55:44 EDT
[-]: MUST Package contains desktop file if it is a GUI application.

^ Not a GUI application, not applicable.

[-]: MUST Development files must be in a -devel package

^ I don't think this applies either.

[!]: MUST Package obeys FHS.

Extensive work was done on this one, now entry points are installed on /usr/bin, test modules are 'read only', theree's a setup.py that installs everything to the correct paths. In fact, we took over one year working on the issues, and the autotest changelog can show all the changes made just for the sake of getting this package accepted.

[!]: MUST Package contains systemd file(s) if in need.

I don't understand, we ship a systemd service file in the autotest source tree, and I believe the rpm package leverages it.
Comment 10 Lucas Meneghel Rodrigues 2012-08-03 11:56:15 EDT
By the way, we *do* aim for EPEL.
Comment 11 Jiri Popelka 2012-08-03 12:06:46 EDT
(In reply to comment #9)
> [-]: MUST Package contains desktop file if it is a GUI application.
> 
> ^ Not a GUI application, not applicable.
> 
> [-]: MUST Development files must be in a -devel package
> 
> ^ I don't think this applies either.

Key:
- = N/A

> [!]: MUST Package obeys FHS.
> 
> Extensive work was done on this one, now entry points are installed on
> /usr/bin, test modules are 'read only', theree's a setup.py that installs
> everything to the correct paths. In fact, we took over one year working on
> the issues, and the autotest changelog can show all the changes made just
> for the sake of getting this package accepted.

Thanks.
 
> [!]: MUST Package contains systemd file(s) if in need.
> 
> I don't understand, we ship a systemd service file in the autotest source
> tree, and I believe the rpm package leverages it.

Yes, but the scriptlets aren't correct, see
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#New_Packages
Comment 12 Ralf Corsepius 2012-08-05 02:19:10 EDT
I do not see any need for the files below 
/usr/lib/python2.7/site-packages/autotest
and /etc/autotest
being owned by "autotest:autotest".

Similarly for many other directories inside of the *server package


/usr/share/autotest being owned by "autotest:autotest" clearly contradicts the FHS (You can not assume user "autotest" being present on other machines in a network) and /usr/share/doc/* being owned by "autotest:autotest" violates the FPG (These files must be owned by root:root).
Comment 13 Martin Krizek 2012-08-07 12:35:15 EDT
Jiri, Ralf, thank you for review, updated spec file and srpm that should fix mentioned issues follow:

Spec URL: http://mkrizek.fedorapeople.org/autotest-framework.spec
SRPM URL: http://mkrizek.fedorapeople.org/autotest-framework-0.14.2-3.fc17.src.rpm

Diff can be seen here: https://github.com/mkrizek/autotest.spec/commit/5ce7e7bab1215f279fa06b78845115a32e47b962

Please note, that we still test if those changes didn't break anything, thanks.
Comment 14 Jiri Popelka 2012-08-08 05:12:29 EDT
rpmlint output still lists some isssues, for example:
the non-executable-script error could be avoided with
http://fedoraproject.org/wiki/Packaging_tricks#Remove_shebang_from_Python_libraries
but that's not a blocker.

Ralf, do you have any other notes ?
Comment 15 Ralf Corsepius 2012-08-09 01:26:55 EDT
(In reply to comment #14)
> rpmlint output still lists some isssues, for example:
> the non-executable-script error could be avoided with
> http://fedoraproject.org/wiki/
> Packaging_tricks#Remove_shebang_from_Python_libraries
> but that's not a blocker.
Yes, nevertheless they should be looked after.

> Ralf, do you have any other notes ?
The ownerships look much better now.

However I am having difficulties in understanding why autotest:autotest is needed below /var/lib/autotest, rsp. how this is supposed to work.

# rpm -qlvp  /var/lib/mock/fedora-rawhide-x86_64/result/autotest-framework-0.14.2-3.fc18.noarch.rpm | grep -v root
drwxr-xr-x    2 autotestautotest                    0 Aug  9 07:05 /var/lib/autotest
drwxr-xr-x    2 autotestautotest                    0 Aug  9 07:05 /var/lib/autotest/.ssh
-rw-r--r--    1 autotestautotest                    0 Aug  9 07:05 /var/lib/autotest/.ssh/id_rsa
-rw-r--r--    1 autotestautotest                    0 Aug  9 07:05 /var/lib/autotest/.ssh/id_rsa.pub
drwxr-xr-x    2 autotestautotest                    0 Aug  9 07:05 /var/lib/autotest/results
drwxr-xr-x    2 autotestautotest                    0 Aug  9 07:05 /var/lib/autotest/tests

This setup implies only the user "autotest" can write to the directories while everybody can read the ssh-keys. Is this really the desired effect?
Comment 16 Martin Krizek 2012-08-09 06:59:25 EDT
I fixed the non-executable-script error and permissions on /var/lib/autotest/.ssh directory. Thanks.

Spec URL: http://mkrizek.fedorapeople.org/autotest-framework.spec
SRPM URL: http://mkrizek.fedorapeople.org/autotest-framework-0.14.2-4.fc17.src.rpm

Diff can be seen here: https://github.com/mkrizek/autotest.spec/commit/20fb879001f2bc8c2f660fbe39fd7104a0596017

Lucas, do you have any comments on why /var/lib/autotest have autotest:autotest permissions? I think you are better person to explain. :)
Comment 17 Lucas Meneghel Rodrigues 2012-08-09 08:17:12 EDT
As far as I understand, /var/lib/autotest in the rpm is a working area for autotest, where the programs can write stuff to. Other than allowing the autotest user to write to that directory, there's no other reason for that. Bottom line, any appropriate permission that will let the autotest user to access (read and write) this area should be fair game.
Comment 18 Lucas Meneghel Rodrigues 2012-08-14 12:43:28 EDT
Hi guys, could you recap what's the status on this review? I'm not sure whether there are more work items to be completed here.
Comment 19 Jiri Popelka 2012-08-15 12:58:14 EDT
I have no more objections. Ralf ?
Comment 20 Ralf Corsepius 2012-08-15 13:17:33 EDT
(In reply to comment #19)
> I have no more objections. Ralf ?

Sorry, I haven't yet had any opportunity to look into the package again (vacation and other commitments) ;)
Comment 21 Martin Krizek 2012-08-24 05:24:40 EDT
Ralf, can you please estimate when you will be able to look into/approve this? It's been a while since the review started and we fixed all mentioned issues and we're happy to fix any more that you might find. Thanks!
Comment 22 Ralf Corsepius 2012-08-24 07:30:15 EDT
Sorry, no ... feel free to go ahead if you feel confident.
Comment 23 Jiri Popelka 2012-08-24 08:50:31 EDT
APPROVED then.
Comment 24 Martin Krizek 2012-09-26 06:28:55 EDT
Thanks everyone who participated in the review! (I am requesting branches only now as I was waiting for a new Autotest release.)

New Package SCM Request
=======================
Package Name: autotest-framework
Short Description: Autotest is a framework for fully automated testing.
Owners: mkrizek
Branches: f16 f17 f18 el5 el6
InitialCC:
Comment 25 Gwyn Ciesla 2012-10-01 06:57:34 EDT
Git done (by process-git-requests).
Comment 26 Gwyn Ciesla 2012-11-20 11:23:06 EST
Unsetting flag.

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