Bug 548522 - Review Request: autotest-client - Autotest is a framework for fully automated testing
Review Request: autotest-client - Autotest is a framework for fully automated...
Status: CLOSED DUPLICATE of bug 838901
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
StalledSubmitter
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-17 12:22 EST by James Laska
Modified: 2013-09-02 02:43 EDT (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-07-13 10:16:03 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description James Laska 2009-12-17 12:22:53 EST
Spec URL: http://jlaska.fedorapeople.org/autoqa/autotest-client.spec
SRPM URL: http://jlaska.fedorapeople.org/autoqa/autotest-client-0.11.0-3.fc12.src.rpm
Description: Autotest is a framework for fully automated testing. It is designed primarily to test the Linux kernel, though it is useful for many other functions such as qualifying new hardware.  The autotest-client package contains only the client-side software and does not include upstream tests.
Comment 1 Terje Røsten 2010-01-06 10:19:37 EST
Why all the hassle with git patches? What's wrong with find and chmod?

Are the patches sent upstream?
Comment 2 James Laska 2010-01-06 12:40:17 EST
(In reply to comment #1)
> Why all the hassle with git patches? What's wrong with find and chmod?

The initial packaging work was done by Jesse Keating.  Git is being used during the build as it is easier for the packager to manage keeping the local patch set in sync and in a format acceptable for submission upstream.

> Are the patches sent upstream?  

Patch1: 0001-Change-usr-local-to-usr-share-where-necessary.patch

This patch is specific to where Fedora prefers package content to live.  Upstream currently uses /usr/local/autotest, and it was determined that /usr/share/autotest was a more appropriate place for Fedora.  Understandably, there isn't interest in taking this patch upstream.

Patch2: 0002-Set-file-permissions-appropriately.patch
Patch3: 0003-Don-t-hardcode-python2.4.patch

These patches are already fixed upstream, but not in the current 0.11.0 release of autotest.  They will however be included in upstream 0.11.1 and future releases.
Comment 3 James Laska 2010-01-07 10:14:05 EST
Koji scratch-build of autotest-client available at http://koji.fedoraproject.org/koji/taskinfo?taskID=1907647
Comment 4 Till Maas 2010-01-11 08:10:39 EST
The remarks about the patches you gave in comment:2 should go in the spec as comments near the PatchX lines.

Why do you set the default owner/group to autotest? It seems to be wrong, e.g. /usr/share/doc/autotest-client-0.11.0/LICENSE does certainly not need to be owned by autotest:autotest.

Also the directory structure does not seem clean, e.g. I guess /usr/share/autotest/client/bin/autotest should go to /usr/bin/autotest.
Comment 5 James Laska 2010-01-11 14:21:39 EST
(In reply to comment #4)
> The remarks about the patches you gave in comment:2 should go in the spec as
> comments near the PatchX lines.

Okay, as directed, I've added my comments to the autotest-client.spec file.

See updated autotest-client.spec - http://jlaska.fedorapeople.org/autoqa/autotest-client.spec

> Why do you set the default owner/group to autotest? It seems to be wrong, e.g.
> /usr/share/doc/autotest-client-0.11.0/LICENSE does certainly not need to be
> owned by autotest:autotest.
> 
> Also the directory structure does not seem clean, e.g. I guess
> /usr/share/autotest/client/bin/autotest should go to /usr/bin/autotest.    

Unfortunately, it cannot be fully packaged as you might expect new software to be packaged in Fedora.  Autotest is packaged upstream such that *everything* lives in it's own root-directory (much like other web-services like mediawiki or moin).  The source-code for autotest expects this.  We investigated carrying patches and moving content into a directory structure that was more familiar to most people, however that patchset would be *enormous*.

I can change the default owner back to root:root, and identify the exactly sub-directories that autotest requires write access to if you prefer.  But it may not be possible to have everything root:root under /usr/share/autotest for autotest-client.
Comment 6 Till Maas 2010-01-11 16:13:49 EST
(In reply to comment #5)
> (In reply to comment #4)

> > Why do you set the default owner/group to autotest? It seems to be wrong, e.g.
> > /usr/share/doc/autotest-client-0.11.0/LICENSE does certainly not need to be
> > owned by autotest:autotest.
> > 
> > Also the directory structure does not seem clean, e.g. I guess
> > /usr/share/autotest/client/bin/autotest should go to /usr/bin/autotest.    
> 
> Unfortunately, it cannot be fully packaged as you might expect new software to
> be packaged in Fedora.  Autotest is packaged upstream such that *everything*
> lives in it's own root-directory (much like other web-services like mediawiki
> or moin).  The source-code for autotest expects this.  We investigated carrying
> patches and moving content into a directory structure that was more familiar to
> most people, however that patchset would be *enormous*.

Is it really a webservice? It does not look like one. Imho for proper packaging, at least some changes to the structure need to be made like moving the binaries that are supposed to be called by a user /usr/bin and maybe some stuff from tools. Or if these scripts are only supposed to be run a the service, then they should be moved to /usr/libexec.


> I can change the default owner back to root:root, and identify the exactly
> sub-directories that autotest requires write access to if you prefer.  But it
> may not be possible to have everything root:root under /usr/share/autotest for
> autotest-client.    

Yes, please do this. But imho the directories that require write access should also be moved below /var. But I somehow doubt, that would be enough. Why is the autotest user even necesary? There is no init script that seems to run the daemon with this priviliges. Is a user sopposed to su to autotest to use the package?
Comment 7 James Laska 2010-01-11 17:14:26 EST
(In reply to comment #6)
> Is it really a webservice? It does not look like one. 

You are correct, the autotest service (which will be put up for review later) contains the front-end web service portion.  

> Imho for proper
> packaging, at least some changes to the structure need to be made like moving
> the binaries that are supposed to be called by a user /usr/bin and maybe some
> stuff from tools. Or if these scripts are only supposed to be run a the
> service, then they should be moved to /usr/libexec.

> > I can change the default owner back to root:root, and identify the exactly
> > sub-directories that autotest requires write access to if you prefer.  But it
> > may not be possible to have everything root:root under /usr/share/autotest for
> > autotest-client.    
> 
> Yes, please do this. But imho the directories that require write access should
> also be moved below /var. But I somehow doubt, that would be enough. Why is the
> autotest user even necesary? There is no init script that seems to run the
> daemon with this priviliges. Is a user sopposed to su to autotest to use the
> package?    

Upstream has no intention of refactoring the code layout so that it lines up with a downstream distribution filesystem standards.  While it certainly would be advantageous for Fedora, and personally more familiar and easier to work with, it's not something they are willing to adopt at this time.  I'll be happy to note that in the .spec file as well.

Additionally, I'm not seeing that the strict filesystem guideline adherence is a blocker to package review.  As far as I can tell, the packaging guidelines list that as a *should* (not a *must*) requirement [1].

I will update the autotest-client.spec and rebuild with a more conservative approach to the usr/grp ownership of the /usr/share/autotest directory structure.  This will likely take a day or more, I will post a comment here when updates are available.

Many thanks!

[1] https://fedoraproject.org/wiki/Packaging/Guidelines#Filesystem_Layout
Comment 8 Adam Williamson 2010-01-16 01:55:22 EST
James, at least the "/usr as read-only" principle is one it's really worth observing; some people actually do mount /usr over a network as read-only, and will complain vociferously if something expects to be able to write to it. I'm sort of with Till here, really; autotest does seem to be wilfully doing rather odd things, and I think it'd be perfectly reasonable to patch those even if upstream weren't immediately interested in the patch. It's not a question of 'a downstream distribution's' filesystem standards, this is FHS stuff, which all distros pay at least lip service to.

You didn't seem to answer Till's question as to why an autotest user account is necessary.

The comment "This package is specific to Fedora..." is probably supposed to read "This patch is specific to Fedora..."

The description is quite thin. It might be useful to explain briefly what the hell autotest is.



-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers
Comment 9 James Laska 2010-01-18 08:19:38 EST
(In reply to comment #8)
> James, at least the "/usr as read-only" principle is one it's really worth
> observing; some people actually do mount /usr over a network as read-only, and
> will complain vociferously if something expects to be able to write to it. I'm
> sort of with Till here, really; autotest does seem to be wilfully doing rather
> odd things, and I think it'd be perfectly reasonable to patch those even if
> upstream weren't immediately interested in the patch. It's not a question of 'a
> downstream distribution's' filesystem standards, this is FHS stuff, which all
> distros pay at least lip service to.

Yeah, it certainly would be cleaner to have it more in line with what most Fedora packages look like.  Unfortunately, that's a pretty aggressive and *unmaintainable* patch.  Also, while it's a *SHOULD* have, it's not a *MUST* have in the packaging guidelines.

    "Fedora follows the Filesystem Hierarchy Standard with regards to
    filesystem layout. The FHS defines where files should be placed on the
    system.  Fedora packages must follow the FHS. Any deviation from the FHS
    should be rationalized when the package is reviewed. " 

    - https://fedoraproject.org/wiki/Packaging/Guidelines#Filesystem_Layout

That said, in my previous comment I noted that I am attempting to address some of the concerns around permssions.  I've not yet completed that work, and will respond with updated packages at that time.

> You didn't seem to answer Till's question as to why an autotest user account is
> necessary.

It's a test harness that includes a local test scheduler.  I believe upstream may run the harness as the root user.  I think that could be improved, so I've added an 'autotest' user/group account to this package.  This comes into play when running the harness on the local system for processing local jobs.  It also is important when running in a distributed setup (provided by the future autotest package).

> The comment "This package is specific to Fedora..." is probably supposed to
> read "This patch is specific to Fedora..."

Thanks, I'll addressed in the latest .spec file.

> The description is quite thin. It might be useful to explain briefly what the
> hell autotest is.

I thought it did that :)  But, I've expanded on it in the latest .spec file.
Comment 10 Jason Tibbitts 2010-11-14 15:20:24 EST
I note that the spec and srpm links all seem to be 404.  And from reading the commentary, I have a hard time imagining that any package reviewer worth their salt would be willing to approve the FHS violations mentioned.  And to avoid the kind of "upstream does it, and even if it's completely broken I don't want to deviate from upstream" rationalization seen above, I'll propose a guideline change.
Comment 11 James Laska 2010-11-15 10:23:33 EST
For history, the current AutoQA packages (including autotest) can be at http://repos.fedorapeople.org/repos/fedora-qa/autoqa/

A dependency of the autotest package (gwt) is not yet available for review.  I'm not prepared to move forward with 'autotest' until 'gwt' is in a state worthy of package review.  At that time, we can revisit the concerns raised in this review request.  

If automated Fedora QA is desired, assistance will be needed to help massage the current 'autotest' package structure into something more Fedora-friendly.  Upstream has structured their autotest "packaging" in an effort to make it a distributable and usable in any operating system / environment.  That intentional design decision makes it difficult to easily move content out into system-wide locations.  

Nothing is impossible of course, but re-packaging 'autotest' is not a priority focus for Fedora test automation efforts at this time.  I definitely need and welcome expertise+guidance on cleaning up the 'autotest' package in a manner consistent with Fedora packaging guidelines, and sustainable for the Fedora package maintainer.
Comment 12 James Laska 2011-07-13 10:16:03 EDT
I'm closing this request for now.  Autotest will likely come back for review soon when we are able to resolve the FHS issues.  When it does, we can either reopen this request, or start a new review.
Comment 13 Martin Krizek 2012-07-10 08:03:35 EDT

*** This bug has been marked as a duplicate of bug 838901 ***

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