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
*** Bug 548522 has been marked as a duplicate of this bug. ***
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.
Martin, Then the package could be named autotest-kernel then... it's not the best name ever, but well...
What about autotest-framework?
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
I can't say I'm a big fan of autotest-framework, but well, it seems as good as anything. Thanks Martin.
Any updates on this one, folks? We're happy to work on any issues you guys might find out.
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
[-]: 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.
By the way, we *do* aim for EPEL.
(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
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).
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.
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 ?
(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?
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. :)
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.
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.
I have no more objections. Ralf ?
(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) ;)
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!
Sorry, no ... feel free to go ahead if you feel confident.
APPROVED then.
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:
Git done (by process-git-requests).
Unsetting flag.