Bug 454329 - Review Request: PolicyKit-olpc - OLPC-specific PolicyKit overrides
Review Request: PolicyKit-olpc - OLPC-specific PolicyKit overrides
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Rahul Sundaram
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-07 14:31 EDT by Daniel Drake
Modified: 2013-03-13 01:43 EDT (History)
5 users (show)

See Also:
Fixed In Version: 1.2-1.fc9
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-12 10:51:44 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
sundaram: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Daniel Drake 2008-07-07 14:31:02 EDT
Spec URL: http://dev.laptop.org/~dsd/PolicyKit-olpc/
SRPM URL: http://dev.laptop.org/~dsd/PolicyKit-olpc/PolicyKit-olpc-1.0-1.fc9.src.rpm
Description: 
Software for the XO laptop requires some modification to the default policies
installed by certain applications. This package provides OLPC-specific
policy overrides. See http://dev.laptop.org/ticket/7350

This is my first package, and I am seeking a sponsor. Maybe Dennis Gilmore ;-)
Comment 1 Daniel Drake 2008-07-07 14:32:02 EDT
Spec URL: http://dev.laptop.org/~dsd/PolicyKit-olpc/PolicyKit-olpc.spec
(Sorry!)
Comment 2 Rahul Sundaram 2008-07-30 11:37:45 EDT
I can't verify functionality but it seems better to add a package dependency
instead of the file dependency on /var/lib/PolicyKit-public. Afaik, permissive
licenses like MIT or revised 2 clause BSD licenses are considered better from
the legal standpoint as opposed to public domain due to presence of disclaimers. 

http://linuxmafia.com/faq/Licensing_and_Law/public-domain.html

You might want to consider that. 
Comment 3 Daniel Drake 2008-08-04 15:22:47 EDT
Thanks for the feedback! I changed it to MIT and uploaded v1.2.

We already have the appropriate package-specific depends: PolicyKit
We additionally depend on the directory because PolicyKit upstream told us the location where these files are stored may change in future. This dependency will make sure that we notice if/when this change happens.
Comment 4 Rahul Sundaram 2008-08-07 16:48:07 EDT
Sorry for the delay. Approved. Please proceed for cvs admin request. 

https://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure
Comment 5 Daniel Drake 2008-08-07 18:25:26 EDT
New Package CVS Request
=======================
Package Name: PolicyKit-olpc
Short Description: OLPC-specific PolicyKit overrides
Owners: dsd
Branches: F-9
InitialCC: dsd
Cvsextras Commits: yes
Comment 6 Kevin Fenzi 2008-08-08 00:14:45 EDT
cvs done.
Comment 7 Fedora Update System 2008-08-08 12:06:05 EDT
PolicyKit-olpc-1.2-1.fc9 has been submitted as an update for Fedora 9
Comment 8 Daniel Drake 2008-08-12 10:51:44 EDT
Thanks for reviewing, we're all done here.
Comment 9 Fedora Update System 2008-08-12 14:19:33 EDT
PolicyKit-olpc-1.2-1.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 10 Christoph Wickert 2009-07-18 09:35:15 EDT
(In reply to comment #2)
> I can't verify functionality but it seems better to add a package dependency
> instead of the file dependency on /var/lib/PolicyKit-public.

IMO this was a really bad idea, because yum runs out of memory on the XO when resolving file based deps.
Comment 11 Rahul Sundaram 2009-07-18 09:53:35 EDT
Does it make sense to add comments to a review done more than six months back? You seem to actually agree with what I said. Don't quite see the point.
Comment 12 Christoph Wickert 2009-07-18 12:32:14 EDT
I think it does, because I'm referring to a problem you already raised in the review. IMO you should have insisted on following the guidelines here, because they clearly state: "Whenever possible you should avoid file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin."
See http://fedoraproject.org/wiki/Packaging/Guidelines#FileDeps
Comment 13 Rahul Sundaram 2009-07-18 12:47:26 EDT
Read what I have written very carefully one more time. I am advocating against file based dependencies. What exactly is your beef? Drop the witch hunt digging up old reviews.
Comment 14 Christoph Wickert 2009-07-18 13:24:49 EDT
(In reply to comment #13)
> Read what I have written very carefully one more time. I am advocating against
> file based dependencies.

I *did* read what you wrote and I fully understand we both agree on the topic. However I don't understand why you approved the package with the file based dep.

> What exactly is your beef? Drop the witch hunt digging up old reviews.

I want to enhance the review quality in Fedora and IMHO these three word reviews ("Looks good, approved") should be forbidden.
Comment 15 Rahul Sundaram 2009-07-18 15:00:58 EDT
The idea of looking into six month old reviews of only mine because of your desire to improve quality does not sound very convincing. 

You might not know this but Daniel Drake works for OLPC. If there are any performance issues with the choice of a file based dependency, he would fixed it by now. There are no need force a common sense recommendation in the guidelines into a mandatory check point and file based dependencies are NOT forbidden at all. All the guidelines have been followed and the review quality is fine. If you disagree and want to dig up old reviews, go ahead and take it up to FESCo. Until that point, I have nothing further to say on this topic.
Comment 16 Christoph Wickert 2009-07-18 20:20:10 EDT
(In reply to comment #15)
> The idea of looking into six month old reviews of only mine because of your
> desire to improve quality does not sound very convincing.

Please don't take this personally, I'm not picking on you. I'm definitely not only doing this with your reviews, but also with others whenever I stumble upon something that IMO should have been fixed in the review (e. g. bug 502920).

> You might not know this but Daniel Drake works for OLPC. 

Rahul, I *can* read and I do know Daniel from various OLPC-related IRC conversations.

> All the guidelines have been followed and the review quality
> is fine.

It should be obvious for others what you have checked and what not.
Comment 17 Rahul Sundaram 2009-07-19 04:39:00 EDT
Yes, it should be obvious and until the point you find a actual problem, there is nothing to discuss.

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