Bug 473348

This is not an official review.

1.  When applying patch #2, patch returns:

patch unexpectedly ends in middle of line

While this doesn't abort the build, the patch file should be fixed to not generate this error.

2.  This would fail to run with SELinux activated. You should fix the context of the CGI script, per http://fedoraproject.org/wiki/PackagingDrafts/SELinux.  Creating a drraw-selinux subpackage would probably be ideal in this case.

3.  Patch #2 appears to be a bug fix.  If you have filed a bug with the upstream source, you should reference this in your spec file; if not, you should file one and reference it.  See http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

(In reply to comment #1)
> This is not an official review.
> 
> 1.  When applying patch #2, patch returns:
> 
> patch unexpectedly ends in middle of line
> 
I've removed this patch and replaced it with a patch that update to latest trunk. There's only a handful of changes since the last beta.

> While this doesn't abort the build, the patch file should be fixed to not
> generate this error.
> 
> 2.  This would fail to run with SELinux activated. You should fix the context
> of the CGI script, per http://fedoraproject.org/wiki/PackagingDrafts/SELinux. 
> Creating a drraw-selinux subpackage would probably be ideal in this case.
> 
I'm not SELinux proficient, but I'll try to take care of that. Thanks for the pointer. In your opinion, does such a requirement block the review or can we save that for later ?

> 3.  Patch #2 appears to be a bug fix.  If you have filed a bug with the
> upstream source, you should reference this in your spec file; if not, you
> should file one and reference it.  See
> http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

There's no bug tracker for this project. This was reported on their mailing list by a colleague of mine, as well as some patch for new features. Actually, even the svn repository is not public, but after a few mails with upstream, I now have read-only access.

I'll post new spec and srpms when the selinux issue is addressed. Thanks for the pre-review.

Updated spec and srpm :
http://www.bachelot.org/fedora/SPECS/drraw.spec
http://www.bachelot.org/fedora/SRPMS/drraw-2.2-0.2.b1.fc10.src.rpm

New version, drraw 2.2 beta 2 :

http://www.bachelot.org/fedora/SPECS/drraw.spec
http://www.bachelot.org/fedora/SRPMS/drraw-2.2-0.4.b2.fc10.src.rpm

New version, add a Group: tag to -selinux subpackage.

http://www.bachelot.org/fedora/SPECS/drraw.spec
http://www.bachelot.org/fedora/SRPMS/drraw-2.2-0.5.b2.fc10.src.rpm

also not an official review (yet) :)

Installing your drraw-selinux subpackage didn't actually make any difference
for me (stock up-to-date f10 install, selinux set to enforcing).

The error logged to syslog advised me to run 'sealert -l something', which
in turn recommended that I run 'chcon -t bin_t /usr/share/drraw/drraw.cgi',
which fixed the issue (again, independently of whether the -selinux subpackage
was installed or not).

Digging through the selinux packaging wiki link from comment #1, I ended up at

http://www.redhat.com/archives/fedora-selinux-list/2006-April/msg00115.html

which seems to suggest that in your case a subpackage is unnecessary, and all
you really need is:

%post
chcon -t bin_t %{_datadir}/%{name}/drraw.cgi

and no need to worry about %postun at all. chcon is part of coreutils, so no need to drag in policycoreutils (probably no need for a -selinux subpackage at all).

Once your package gets accepted, file bug against selinux-policy and request that your package be handled (again, as per the wiki page in comment #1)

PING

It's been more than a year with no progress; This bug should be closed soon if
there is no response, shouldn't it?

(In reply to comment #7)
> PING
> 
> It's been more than a year with no progress; This bug should be closed soon if
> there is no response, shouldn't it?    

The review is not assigned to anyone. The only reported issue for now is about the SE Linux stuff. I tried to blindly addressed it by adding a -selinux subpackage with what I think is the correct fix. However, I wasn't able to test it, that's why I said blindly. I don't know much when it comes to SE Linux, but I believe the proposed change in comment #6 is not the proper fix. If someone step up to the plate for the review, I'll setup a VM and take another look at the SE Linux issue, but I don't think this is a blocker.

Xavier,

I'm assigning this bug to me, and proceeding the formal review as follows.


Good:
* Package is named drraw which follows the upstream project name
* spec file naming follows package naming
* License in spec and sources is BSD which is open source
* License text included in the tarball so no need to include one.
* Spec is legible and American English
* http://web.taranis.org/drraw/dist/drraw-2.2b2.tar.gz
* Source matches upstream
* No locale files
* No shared libraries
* No bundled libraries
* Not relocatable
* No directories created unowned
* No duplicate files
* Default permissions are set
* Package is code
* No large documentation
* No %doc files are used at runtime
* No header files
* Not a GUI application
* Does not own files or directories from other packages
* All filenames are utf8


Needswork:
* Patch files should have upstream issues opened to look good for our purposes

* rpmlint complains about macros in comments:
   drraw.src:68: W: macro-in-comment %{SOURCE2}
   drraw.src:88: W: macro-in-comment %doc
   1 packages and 0 specfiles checked; 0 errors, 2 warnings.

* Minor adjusts needed to sections %post & %unpost at -selinux subpackage:

%post selinux
semanage fcontext -a -t httpd_sys_script_exec_t '%{_datadir}/%{name}(/.*)?' 2>/dev/null || :
semanage fcontext -a -t httpd_sys_rw_content_t '%{_localstatedir}/lib/%{name}(/.*)?' 2>/dev/null || :
restorecon -R %{_datadir}/%{name} %{_localstatedir}/lib/%{name} || :


%postun selinux
if [ $1 -eq 0 ] ; then
semanage fcontext -d -t httpd_sys_script_exec_t '%{_datadir}/%{name}(/.*)?' 2>/dev/null || :
semanage fcontext -d -t httpd_sys_rw_content_t '%{_localstatedir}/lib/%{name}(/.*)?' 2>/dev/null || :
fi

* Test builds in mock & koji

Xavier,

It's been a week since I've posted a formal review to this package of yours without having a single reply from you. Please, let me know if you are facing any difficulty to finish this work, and tell me how can I help you.

If you, for instance, just lost your interest in getting this work reviewed, let me aware as well.

Regards.

(In reply to comment #10)
> Xavier,
> 
> It's been a week since I've posted a formal review to this package of yours
> without having a single reply from you. Please, let me know if you are facing
> any difficulty to finish this work, and tell me how can I help you.
> 
> If you, for instance, just lost your interest in getting this work reviewed,
> let me aware as well.
> 
> Regards.

Hi Rafael,

Thanks for the review. I'm just back from a 2 weeks vacation with no internet access. I've not lost interest and I'll submit a new srpm addressing the issues you outlined asap (probably before the end of the week).

Regards,
Xavier

Rafael,
New package with your comments addressed :
http://www.bachelot.org/fedora/SPECS/drraw.spec
http://www.bachelot.org/fedora/SRPMS/drraw-2.2-0.6.b2.fc13.src.rpm

Thanks for the selinux fix (this is still untested though).

Xavier,

Tanks for replying promptly, and also thanks for addressing the NEEDSWORK suggested. As a result, drraw has been built correctly in mock and koji, and also it has installed and worked fine in my test system.

  http://koji.fedoraproject.org/koji/taskinfo?taskID=2413481 

IMHO this package looks OK, and I'm approving it.

Regards.


APPROVED

New Package SCM Request
=======================
Package Name: drraw
Short Description: Web based presentation front-end for RRDtool
Owners: xavierb
Branches: f12 f13 f14 el5 el6
InitialCC: 


Thanks a lot for the review Rafael :-)

Xavier,

Please, consider including man pages for drraw before shipping it to testing, as it is a SHOULD item on review process.

Regards

Git done (by process-git-requests).

drraw-2.2-0.6.b2.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/drraw-2.2-0.6.b2.fc13

drraw-2.2-0.6.b2.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/drraw-2.2-0.6.b2.fc12

drraw-2.2-0.6.b2.fc14 has been submitted as an update for Fedora 14.
http://admin.fedoraproject.org/updates/drraw-2.2-0.6.b2.fc14

drraw-2.2-0.6.b2.fc14 has been pushed to the Fedora 14 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update drraw'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/drraw-2.2-0.6.b2.fc14

drraw-2.2-0.6.b2.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/drraw-2.2-0.6.b2.el5

drraw-2.2-0.6.b2.el5 has been pushed to the Fedora EPEL 5 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update drraw'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/drraw-2.2-0.6.b2.el5

drraw-2.2-0.6.b2.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

drraw-2.2-0.6.b2.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

drraw-2.2-0.6.b2.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

drraw-2.2-0.6.b2.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.