This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 230512 - Review Request: viewvc - Browser interface for CVS and SVN version control repositories
Review Request: viewvc - Browser interface for CVS and SVN version control r...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Bernard Johnson
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-28 23:24 EST by Bojan Smojver
Modified: 2015-08-21 09:39 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-08 22:59:50 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
bojan: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
conditionals for epel (1.42 KB, patch)
2007-03-03 05:23 EST, Bernard Johnson
no flags Details | Diff
Benchmarks under mod_python (3.43 KB, text/plain)
2007-03-03 22:47 EST, Bojan Smojver
no flags Details
Benchmarks under CGI (3.44 KB, text/plain)
2007-03-05 18:32 EST, Bojan Smojver
no flags Details
make enscript only enable when it's available (716 bytes, text/x-patch)
2007-03-05 22:40 EST, Bernard Johnson
no flags Details

  None (edit)
Description Bojan Smojver 2007-02-28 23:24:49 EST
Spec URL: ftp://ftp.rexursive.com/pub/viewvc/viewvc.spec
SRPM URL: ftp://ftp.rexursive.com/pub/viewvc/viewvc-1.0.3-1.src.rpm
Description:
ViewVC is a browser interface for CVS and Subversion version control
repositories. It generates templatized HTML to present navigable directory,
revision, and change log listings. It can display specific versions of files
as well as diffs between those versions. Basically, ViewVC provides the bulk
of the report-like functionality you expect out of your version control tool,
but much more prettily than the average textual command-line program output.
Comment 1 Bojan Smojver 2007-02-28 23:28:15 EST
Example repository (Subversion) view using this RPM:

http://www.rexursive.com/viewvc/
Comment 2 Bernard Johnson 2007-03-01 02:08:51 EST
I was about to package this as well, so I'll provide you a review for it.
Comment 3 Bojan Smojver 2007-03-01 02:20:47 EST
Thanks.
Comment 4 Bernard Johnson 2007-03-01 03:16:38 EST
Here are just a few things you should look into before we get started in earnest.

1) If you have rpmdevtools installed, you can generate a spec file template that
is made for a python program with 'fedora-newrpmspec -t python'

2) You are doing some things the hard way, which will make the package harder to
maintain in the longer run.  Read up on
http://fedoraproject.org/wiki/Packaging/Python regarding automatic byte
compilation, and basic python packaging.
Comment 5 Bernard Johnson 2007-03-01 03:17:37 EST
fixing my flag for review... twiddled the wrong bit
Comment 6 Bojan Smojver 2007-03-01 05:21:29 EST
Thanks for your quick feedback. I'm not a Python guy (just like viewvc :-), so I
had no idea how good the spec file that I pinched off Dag's site was. I'll work
on getting that in line with your suggestions and I'll let you know when I have
something new.
Comment 7 Bojan Smojver 2007-03-01 05:58:09 EST
Hopefully there is some improvement here:

ftp://ftp.rexursive.com/pub/viewvc/viewvc.spec
ftp://ftp.rexursive.com/pub/viewvc/viewvc-1.0.3-2.FC6.src.rpm
Comment 8 Bojan Smojver 2007-03-01 14:44:43 EST
Actually, now that I look at the -2 version of the spec file, it seems to me
that I may have misunderstood the %{python_sitelib} usage on the Packaging
Python page. This directory holds pure python stuff, right? It's not meant as a
replacement for /usr/share/packagename (i.e. architecture independent files)? Or
is it?
Comment 9 Bernard Johnson 2007-03-01 14:48:01 EST
> # sitelib for noarch packages, sitearch for others (remove the unneeded one)
%{!?python_sitelib: %define python_sitelib %(%{__python} -c "from 
> distutils.sysconfig import get_python_lib; print get_python_lib()")}
%{!?python_sitearch: %define python_sitearch %(%{__python} -c "from 
> distutils.sysconfig import get_python_lib; print get_python_lib(1)")}

You can remove the sitearch line if your package is a noarch.

> Requires:       httpd, mod_python, subversion >= 1.2, rcs, diffutils

You require both cvs (rcs) and subversion file even though someone could use one
or the other.  Is there a better way to do this?

You are also setting up the include file for cvsgraph, but not requiring cvsgraph.

GNU enscript is already available in Extras.  If you require and configure for
enscript you can have sytax highlighting.

The INSTALL files says that you need subversion python bindings - it appears
that those are in the main subversion package?

> %doc CHANGES README INSTALL TODO
Normally we don't include INSTALL files, except in this case it has additional
instruction on how to setup the mysql commit database (if someone wants to do
that at a later time), so it is useful.

The COMMITTERS file lists the contributors to the project, so it should be
included in %doc.

Are ./www and ./viewvc.org useful docs?

In viewvc.conf:
>     AddHandler python-program .py
>     PythonHandler handler

will require mod_python (which you do).  This is only one way of running viewvc.
 Is there any particular reason that you've chosen this method?  Is this the
best way?

In viewvc.conf:
> #    PythonDebug On
remove :)


> Requires:       httpd, mod_python, subversion >= 1.2, rcs, diffutils

If you require mod_python, you don't need httpd because it will be found by
dependency of mod_python on httpd.

I'll have to dig into the substitutions you're doing, but that will take me a
little longer.

Here are the rpmlint complaints for you package:
E: viewvc script-without-shebang
/usr/lib/python2.4/site-packages/viewvc/bin/mod_python/query.py
E: viewvc script-without-shebang
/usr/lib/python2.4/site-packages/viewvc/bin/mod_python/handler.py
E: viewvc non-executable-script
/usr/lib/python2.4/site-packages/viewvc/lib/vclib/ccvs/blame.py 0644
E: viewvc non-executable-script
/usr/lib/python2.4/site-packages/viewvc/lib/py2html.py 0644
E: viewvc non-executable-script
/usr/lib/python2.4/site-packages/viewvc/lib/compat_difflib.py 0644
E: viewvc non-executable-script
/usr/lib/python2.4/site-packages/viewvc/lib/compat_ndiff.py 0644
E: viewvc non-executable-script
/usr/lib/python2.4/site-packages/viewvc/lib/query.py 0644
E: viewvc non-executable-script
/usr/lib/python2.4/site-packages/viewvc/lib/ezt.py 0644
E: viewvc htaccess-file
/usr/lib/python2.4/site-packages/viewvc/bin/mod_python/.htaccess
E: viewvc script-without-shebang
/usr/lib/python2.4/site-packages/viewvc/bin/mod_python/.htaccess
E: viewvc script-without-shebang
/usr/lib/python2.4/site-packages/viewvc/bin/mod_python/viewvc.py
E: viewvc non-executable-script
/usr/lib/python2.4/site-packages/viewvc/lib/blame.py 0644
W: viewvc setup-not-quiet
W: viewvc mixed-use-of-spaces-and-tabs (spaces: line 5, tab: line 41)
Comment 10 Bernard Johnson 2007-03-01 14:58:21 EST
(In reply to comment #8)
> Actually, now that I look at the -2 version of the spec file, it seems to me
> that I may have misunderstood the %{python_sitelib} usage on the Packaging
> Python page. This directory holds pure python stuff, right? It's not meant as a
> replacement for /usr/share/packagename (i.e. architecture independent files)? Or
> is it?

You will see packaging done in both places.  Certainly "module" type packages
will live in the sitelib (like the subversion bindings).  I believe that for
applications like this is is more a matter of taste.

I'm no python expert (or packing expert), but I just recently finished TMDA
package that lives in sitelib.

Some people believe that /usr/share is strictly for arch independent data files
(not program).

And just as an example:
MoinMoin (web wiki) : sitelib
Yum                 : %{datadir}

Your packages templates should definately live in %{datadir} though.

Comment 11 Bojan Smojver 2007-03-01 15:11:49 EST
Regarding, comment #8, I did a quick -3 of the spec file to illustrate what I
meant. It is here:

ftp://ftp.rexursive.com/pub/viewvc/viewvc-3.spec
Comment 12 Bojan Smojver 2007-03-01 15:24:06 EST
> You can remove the sitearch line if your package is a noarch.

Shall do.

> You require both cvs (rcs) and subversion file even though someone could use one
or the other.  Is there a better way to do this?

Yeah, I was also thinking about this. On one hand, if I don't require this and
someone configures one of the two options (CVS or SVN), it may or may not work,
depending on the current state of their system. So, I thought, better err on the
side of caution and make sure at least minimum requirements for both (since
Fedora ships both CVS and SVN) are satisfied, so that things just work. But, if
there is a better way to do this, I'd be happy to change...

> You are also setting up the include file for cvsgraph, but not requiring cvsgraph.

> GNU enscript is already available in Extras.  If you require and configure for
enscript you can have sytax highlighting.

Yeah, not consistent eh? It's easy to put both in as dependencies, I guess. Will
change.

> The INSTALL files says that you need subversion python bindings - it appears
that those are in the main subversion package?

Looks that way on FC6. I'll check devel to see if things have been split up some
more.

> The COMMITTERS file lists the contributors to the project, so it should be
included in %doc.

> Are ./www and ./viewvc.org useful docs?

Yes, to both. Just copied Dag's stuff here, without looking at it much. Will
include.

> will require mod_python (which you do).  This is only one way of running viewvc.
> Is there any particular reason that you've chosen this method?  Is this the
best way?

Well, I was guessing that Python programs run best within mod_python, like Perl
programs do under mod_perl. Not a Python programmer myself, so I have no
proof... I mean, CGI stuff is still there, so if someone wants to reconfigure to
run like that, they still can.

>> #    PythonDebug On
>remove :)

Shall do ;-)

> If you require mod_python, you don't need httpd because it will be found by
dependency of mod_python on httpd.

Shall fix.

> Here are the rpmlint complaints for you package:

I'll look into that.

> Your packages templates should definately live in %{datadir} though.

Yeah, that's how -3 spec file does it.

Thanks for your time! I'll work on improvements and report back when I have them.
Comment 13 Bojan Smojver 2007-03-01 15:30:02 EST
> Looks that way on FC6. I'll check devel to see if things have been split up some
more.

Devel still ships python bindings with subversion package, so we should be OK there.
Comment 14 Bojan Smojver 2007-03-01 18:15:00 EST
OK, there is -4 now, located here:

ftp://ftp.rexursive.com/pub/viewvc/viewvc.spec
ftp://ftp.rexursive.com/pub/viewvc/viewvc-1.0.3-4.FC6.src.rpm

Most of the above issues should be now addressed. Enscript stuff still doesn't
work dues to SELinux issues. I'm looking into that now...
Comment 15 Bojan Smojver 2007-03-01 21:49:51 EST
OK, I've just uploaded the SELinux enabled version:

ftp://ftp.rexursive.com/pub/viewvc/viewvc.spec
ftp://ftp.rexursive.com/pub/viewvc/viewvc-1.0.3-5.FC6.src.rpm
Comment 16 Bojan Smojver 2007-03-01 23:54:01 EST
BTW, output of rpmlint is:

--------------------------------
$ rpmlint viewvc-1.0.3-5.FC6.noarch.rpm 
W: viewvc incoherent-version-in-changelog 1.0.3-5 1.0.3-5.FC6
E: viewvc non-standard-uid /var/spool/viewvc apache
E: viewvc non-standard-gid /var/spool/viewvc apache
E: viewvc non-standard-dir-perm /var/spool/viewvc 0700
E: viewvc unknown-key GPG#b832d3e3
--------------------------------

Hmm, not sure how to avoid the above - that's how things are supposed to be (bar
unknown GPG key, of course).

And for selinux package:

--------------------------------
$ rpmlint viewvc-selinux-1.0.3-5.FC6.noarch.rpm 
W: viewvc-selinux no-documentation
E: viewvc-selinux unknown-key GPG#b832d3e3
E: viewvc-selinux forbidden-selinux-command-in-%post chcon
--------------------------------

Equally confusing - the chcon bit I pinched directly from the Fedora Wiki:

http://fedoraproject.org/wiki/PackagingDrafts/SELinux/#head-a81109d3dfc8d5798d761d332c3153af0668e065
Comment 17 Ville Skyttä 2007-03-02 10:37:04 EST
$ rpmlint -I forbidden-selinux-command-in-%post
forbidden-selinux-command-in-%post :
A command which requires intimate knowledge about a specific SELinux
policy type was found in the scriptlet. These types are subject to change
on a policy version upgrade. Use the restorecon command which queries the
currently loaded policy for the correct type instead.

See also bug 214605 (the reporter, Steve, might be able to provide more info how
to do the SELinux stuff better if needed).
Comment 18 Paul Howarth 2007-03-02 11:34:02 EST
(In reply to comment #12)
> Well, I was guessing that Python programs run best within mod_python, like Perl
> programs do under mod_perl.

This is not always the case; moin for example, runs significantly faster as a
FastCGI application (e.g. with mod_fcgid) than with mod_python.
Comment 19 Paul Howarth 2007-03-02 13:11:29 EST
Regarding the SELinux issue, I think the best option is to:

(a) use %{_localstatedir}/cache/viewvc instead of %{_localstatedir}/spool/viewvc
(this is just the way I usually do it though)
(b) file a bug against selinux-policy requesting that the context type of
/var/cache/viewvc(/.*)? be set to httpd_cache_t
(c) remove all SElinux-related content from your own package

Dan is generally quite responsive to such requests.

Whilst waiting for the selinux-policy update to appear (updates appear quite
frequently, so it's unlikely to be long), you could just put a README.SELinux
file in that suggested the context changes to do manually if needed.
Comment 20 Bojan Smojver 2007-03-02 17:16:56 EST
Regarding comment #19, /var/cache has a different purpose than what we need
here, according to FHS 2.3. So, I have chosen /var/spool, which seems to fit a
bit better, since we cannot use /tmp or /var/tmp (i.e. we could get our
directory removed from underneath us).

Regarding comment #17, I'll use restorecon. Thanks.

Regarding comemnt #18, I really have no data regarding this in relation to viewvc.

I filed a bug against selinux-policy (bug #230794), so that selinux support can
be removed in future versions of the package, or if the change happens before we
get this out the door, then before.
Comment 22 Bernard Johnson 2007-03-02 18:54:11 EST
(In reply to comment #12)
> Yeah, I was also thinking about this. On one hand, if I don't require this and
> someone configures one of the two options (CVS or SVN), it may or may not work,
> depending on the current state of their system. So, I thought, better err on the
> side of caution and make sure at least minimum requirements for both (since
> Fedora ships both CVS and SVN) are satisfied, so that things just work. But, if
> there is a better way to do this, I'd be happy to change...

Well, looking at the packages required by each of those that you require
(subversion, rcs).  They have similarities and not too many differences.  IMHO,
it is not unreasonable to list both as requires rather than create meta packages
with the sole purpose of pulling the right dependencies.

# repoquery --resolve --requires subversion | sort
apr-0:1.2.7-10.i386  (<-- already required by httpd)
apr-util-0:1.2.8-1.fc6.i386 (<-- already required by httpd)
bash-0:3.1-16.1.i386 (<-- already required by httpd)
coreutils-0:5.97-12.3.fc6.i386 (<-- already required by httpd)
db4-0:4.3.29-9.fc6.i386 (<-- already required by httpd)
e2fsprogs-libs-0:1.39-7.fc6.i386
expat-0:1.95.8-8.2.1.i386 (<-- already required by httpd)
glibc-0:2.5-10.fc6.i686 (<-- already required by almost everything)
krb5-libs-0:1.5-13.i386 (<-- needed if cvs is installed)
neon-0:0.25.5-5.1.i386 (<-- already required by rpm)
openldap-0:2.3.27-4.i386 (<-- already required by httpd)
openssl-0:0.9.8b-8.3.fc6.i686 (<-- already required by httpd)
perl-4:5.8.8-10.i386 (<-- needed if cvs is installed)
perl-URI-0:1.35-3.noarch
python-0:2.4.4-1.fc6.i386  (<-- required for viewvc)
subversion-0:1.4.2-2.fc6.i386
zlib-0:1.2.3-3.i386 (<-- already required by httpd)

# repoquery --resolve --requires rcs | sort
glibc-0:2.5-10.fc6.i686

But more likely, if you're using viewvc with cvs repositories you also have cvs
installed (although this is not a hard requirement)

# repoquery --resolve --requires cvs | sort
bash-0:3.1-16.1.i386
cvs-0:1.11.22-6.fc6.i386
glibc-0:2.5-10.fc6.i686
info-0:4.8-14.fc6.i386
krb5-libs-0:1.5-13.i386
pam-0:0.99.6.2-3.16.fc6.i386
perl-4:5.8.8-10.i386
vim-minimal-2:7.0.191-2.fc6.i386
zlib-0:1.2.3-3.i386
Comment 23 Bernard Johnson 2007-03-02 19:07:27 EST
(In reply to comment #18)
> (In reply to comment #12)
> > Well, I was guessing that Python programs run best within mod_python, like Perl
> > programs do under mod_perl.
> 
> This is not always the case; moin for example, runs significantly faster as a
> FastCGI application (e.g. with mod_fcgid) than with mod_python.

I guess some basic benchmarking might be appropriate.
Comment 24 Bernard Johnson 2007-03-02 19:21:41 EST
(In reply to comment #21)
> Another bump (-6) here:
> 
> ftp://ftp.rexursive.com/pub/viewvc/viewvc.spec
> ftp://ftp.rexursive.com/pub/viewvc/viewvc-1.0.3-6.FC6.src.rpm

Another thing I'd like to cover is the ability to push this into the EPEL repo.
 This provides some more problems to solve.  Right off the top of my head EL4
only has subversion 1.1, so viewvc won't be usable with a subversion repository,
however, there is no reason that it can't be used with a cvs repository.

We need to add some strategic conditionals to take this into consideration.
Also, until it is known that enscript and cvsgraph are pushed into the EPEL
repo, those should be conditional as well.
Comment 25 Bernard Johnson 2007-03-02 19:29:05 EST
This comment can be removed since you only have the relevent line anyway:
# sitelib for noarch packages, sitearch for others (remove the unneeded one)

W: viewvc setup-not-quiet
You should use -q to have a quiet extraction of the source tarball, as this
generate useless lines of log ( for buildbot, for example )

W: viewvc macro-in-%changelog python_sitelib
Macros are expanded in %changelog too, which can in unfortunate cases lead
to the package not building at all, or other subtle unexpected conditions that
affect the build.  Even when that doesn't happen, the expansion results in
possibly "rewriting history" on subsequent package revisions and generally
odd entries eg. in source rpms, which is rarely wanted.  Avoid use of macros
in %changelog altogether, or use two '%'s to escape them, like '%%foo'.

This is triggered by your changelog comment in 1.0.3-3.  Change
"%{python_sitelib}" to "%%{python_sitelib}".

W: viewvc mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 61)
The specfile mixes use of spaces and tabs for indentation, which is a
cosmetic annoyance.  Use either spaces or tabs for indentation, not both.
Comment 26 Bojan Smojver 2007-03-02 21:30:53 EST
Regarding comment #25, this is all now in:

ftp://ftp.rexursive.com/pub/viewvc/viewvc.spec
ftp://ftp.rexursive.com/pub/viewvc/viewvc-1.0.3-7.FC6.src.rpm

Regarding comment #24, feel free to send patches to that effect.

Regarding comment #23, do you have a setup handy where you can do this?
Comment 27 Bernard Johnson 2007-03-03 05:20:50 EST
(In reply to comment #26)
> Regarding comment #23, do you have a setup handy where you can do this?

I do not.  I wasn't thinking of anything overly complex.  Just some time-trials
with scripted wget for comparison.
Comment 28 Bernard Johnson 2007-03-03 05:23:02 EST
Created attachment 149171 [details]
conditionals for epel
Comment 29 Bojan Smojver 2007-03-03 05:35:07 EST
Thanks for the EPEL stuff, I'll incorporate in -8.

In relation to benchmarks, I'll have to set up mod_fcgi/viewvc combo here, so it
may take some time. I'll keep you posted.
Comment 30 Bernard Johnson 2007-03-03 06:04:27 EST
> In relation to benchmarks, I'll have to set up mod_fcgi/viewvc combo here,
> so it may take some time. I'll keep you posted.

No problem, we want it to be a quality package, so take as much time as you need.

Suggestions:
1) In your %post and %postun section, use full pathnames of the programs you run
(ie. semanage & restorecon)
2) Use -b with patch to create a backup file.  This is recommended by other
fedora packages.

Question:
You have:
# Set mode 755 on executable scripts
%{__grep} -rl '^#!' %{buildroot}%{python_sitelib}/viewvc | xargs %{__chmod} 0755

but then later:
# mod_python files do not have to be executable (make rpmlint happy)
%{__chmod} 0644 %{buildroot}%{python_sitelib}/viewvc/bin/mod_python/*.py

So, do any of the file need to be +x ??

Nice job on the selinux integration, I see you fixed the packaging draft too :)

I will try to do the formal review by Tuesday morning if you're ready by then. 
I still want to look into the setup/installation before then.

Oops, my patch tests for <= "5" but it should have been < "5".  You probably
want to fix that.  I didn't have an EL5 to build against to check if the
dependencies were right.
Comment 31 Ville Skyttä 2007-03-03 08:12:08 EST
Just for the record, regarding SELinux things: while using restorecon instead of
chcon could be seen as a slight improvement, the real problem which is presumed
knowledge of existence and semantics of a particular type
(httpd_sys_script_exec_t in this case) remains.  I'm sure rpmlint could still be
improved here [0], but it'd be better if someone more well versed with SELinux
than me would supply patches.  Steve, comments?  If anyone has improvement
ideas, feel free to submit bugs against rpmlint here or upstream.

[0] Should semanage when used with a type be flagged as an error/warning?  If
yes, the -I message needs work too as restorecon doesn't suffice alone.
Comment 32 Bernard Johnson 2007-03-03 08:22:40 EST
While we're at it, could someone (Steve?) reconcile the difference between what
is recommended on the draft packaging page
(http://fedoraproject.org/wiki/PackagingDrafts/SELinux/#head-a81109d3dfc8d5798d761d332c3153af0668e065)
and what rpmlint is (or should be) advising?

It seems like we are taking a circular path that always comes back to "the
policy package must always know about the needs of your package before your
package will work with selinux".  Am I misunderstanding something here, or is
that what we are being told?
Comment 33 Steve Grubb 2007-03-03 08:42:52 EST
Regarding comment #31, spec file should have no knowlege of types. Policy could
change without warning and break this package's install. Yes, semanage with a -t
arg should be flagged by rpmlint as well.
Comment 34 Steve Grubb 2007-03-03 08:55:34 EST
Regarding comment #32, I asked some people to review and help with the
guidelines.  The bottom line is that embedding knowlege of a specific type can
be bad for long term use of the package. There are always ways to ask selinux to
figure out the right type.

The problem mentioned is that currently we ship a monolithic policy. We are
working on a way to break up the policy so each package can supply its own
policy bits. However, this means changing a lot of infrastructure including rpm,
 anaconda, and even the kernel. We are moving to self service, but it will still
be a while before everything is in place. Meanwhile, Dan is always happy to take
policy patches and is pretty quick to push new packages to rawhide.

Hope this helps...
Comment 35 Ville Skyttä 2007-03-03 10:53:51 EST
Thanks for the commments, Steve.  rpmlint RFE filed in bug 230845, feel free to
chime in.
Comment 36 Bojan Smojver 2007-03-03 17:10:41 EST
Regarding comment #30, all of that + EPEL patch is now in -8:

ftp://ftp.rexursive.com/pub/viewvc/viewvc.spec
ftp://ftp.rexursive.com/pub/viewvc/viewvc-1.0.3-8.FC6.src.rpm

In relation to +x, if I don't do that to any file that has #! in th first line,
rpmlint won't like it. So, it should stay, I guess.

Regarding SELinux comments, looks like the only real fix here is to integrate
the whole thing with official policy. I'll leave it in the spec for now, but the
moment bug #230794 is resolved, I'll drop the lot.
Comment 37 Bojan Smojver 2007-03-03 22:47:08 EST
Created attachment 149198 [details]
Benchmarks under mod_python
Comment 38 Bojan Smojver 2007-03-03 22:59:18 EST
Probably a stupid question to ask, but did anyone here ever use viewvc under
mod_fcgid? If so, it would be good to know configuration details, as I'm getting
a bit of pain when trying to make this run:

--------------------------------------------------
Status: 301 Moved
Content-Type: text/html; charset=UTF-8
Location: /

This document is located <a href="/">here</a>.
[Sun Mar 04 14:38:55 2007] [warn] (104)Connection reset by peer: mod_fcgid: read
data from fastcgi server error.
[Sun Mar 04 14:38:55 2007] [error] [client 127.0.0.1] Premature end of script
headers: viewvc.cgi
[Sun Mar 04 14:38:58 2007] [notice] mod_fcgid: process
/usr/lib/python2.4/site-packages/viewvc/bin/cgi/viewvc.cgi(18965)
exit(communication error), terminated by calling exit(), return code: 0
--------------------------------------------------

There are no files called .fcg, .fcgi or .fpl, so I'm guessing the only other
option is to run a straight .cgi file, right?

I had this in my configuration:

fcgid.conf:
--------------------------------------------------
<IfModule !mod_fastcgi.c>
    AddHandler fcgid-script fcg fcgi fpl cgi
</IfModule>
--------------------------------------------------

viewvc.conf:
--------------------------------------------------
ScriptAlias /viewvc /usr/lib/python2.4/site-packages/viewvc/bin/cgi/viewvc.cgi
Alias /viewvc-static /usr/share/viewvc/templates/docroot

<Directory /usr/lib/python2.4/site-packages/viewvc/bin/mod_python>
    Options ExecCGI
</Directory>
--------------------------------------------------

And, of course, all mod_python stuff commented out.
Comment 39 Paul Howarth 2007-03-04 04:49:27 EST
CGI scripts need to be adapted to work in FastCGI mode; if viewvc doesn't
include documentation on running in FastCGI mode, it almost certainly doesn't
support it.

Regarding SELinux, an alternative (and IMHO much better from a package
maintainability point of view) approach is to use a loadable policy module
containing the policy for your app. This will use the interfaces provided by the
main policy package and is a little more robust, though still needs knowledge of
types from the main policy. The mod_fcgid package takes this approach, which is
documented, from a packaging perspective, here:

http://fedoraproject.org/wiki/PackagingDrafts/SELinux/PolicyModules
Comment 40 Bojan Smojver 2007-03-04 05:11:14 EST
OK, we can forget FastCGI then. I don't see anything documented anywhere in
viewvc regarding that.

Regarding SELinux, that was actually the first page I found on the topic and we
could sure do one of those. Hopefully, we won't have to, if Dan beats us to it :-)

Anyhow, I was initially looking for a "common" Apache area that would have
httpd_sys_script_rw_t in order to reuse it, but couldn't find anything on my FS.
If there is such a place, we can just point viewvc to it...
Comment 41 Bojan Smojver 2007-03-04 05:31:28 EST
Comment on attachment 149198 [details]
Benchmarks under mod_python

The benchmarking was done on this hardware:
http://www.rexursive.com/articles/linuxondell6400.html

OS was Fedora Core 6.
Comment 42 Bernard Johnson 2007-03-05 17:14:36 EST
(In reply to comment #37)
> Created an attachment (id=149198) [edit]
> Benchmarks under mod_python

Is this faster/safer/more stable/more secure/etc. than running as plain +ExecCGI
under apache?  In other words, what does mod_python give us that plain cgi doesn't.
Comment 43 Bojan Smojver 2007-03-05 18:32:33 EST
Created attachment 149306 [details]
Benchmarks under CGI
Comment 44 Bojan Smojver 2007-03-05 18:34:32 EST
Sorry, I thought the CGI thingy would be obvious, so I didn't post the
benchmarks before. Essentially, mod_python setup gives us much better performance.
Comment 45 Bernard Johnson 2007-03-05 19:17:53 EST
In viewvc.conf:
> # Use CvsGraph. See http://www.akhphd.au.dk/~bertho/cvsgraph/ for
> # documentation and download.
> #
> use_cvsgraph = 0

package required but not used.

(In reply to comment #36)
> In relation to +x, if I don't do that to any file that has #! in th first line,
> rpmlint won't like it. So, it should stay, I guess.

> # mod_python files do not have to be executable (make rpmlint happy)
> %{__chmod} 0644 %{buildroot}%{python_sitelib}/viewvc/bin/mod_python/*.py

Ah, now I see.  After some additional investigation... this appears to be a
minor bug in their install script.  It sets the +x although that is not needed
for mod_python, and because there is no shebang, then rpmlint gets angry.  Ok, I
got it now.

You might want to feed this back upstream.

You probably want to notate that in the spec file as well so others don't wonder
why.


License:
compat_difflib.py: No license, assumed BSD or PSF ?
compat_ndiff.py: Public Domain - ok
py2html.py: eGenix.com Public License - ?
PyFontify.py: No license, assumed BSD ?
 
I *think* these are all ok.


Did you intend to do any updates based on Paul's comment #39?

Comment 46 Bernard Johnson 2007-03-05 19:19:26 EST
(In reply to comment #44)
> Sorry, I thought the CGI thingy would be obvious, so I didn't post the
> benchmarks before. Essentially, mod_python setup gives us much better performance.

Well, hopefully now we don't have to worry about someone asking why it wasn't
done their favorite way.  Here are the results.  Case closed.
Comment 47 Bojan Smojver 2007-03-05 19:44:46 EST
> package required but not used.

Coming in -9. Sorry :-(

> You probably want to notate that in the spec file as well so others don't wonder
why.

Also coming in -9.

> You might want to feed this back upstream.

OK.

> Did you intend to do any updates based on Paul's comment #39?

Not really. Since we need a one-liner in the official context instead and nobody
seems in the rush, I don't want to make http_script_exec_t R/W var_spool_t by
our policy unless I really need to.
Comment 49 Bernard Johnson 2007-03-05 22:40:47 EST
Created attachment 149318 [details]
make enscript only enable when it's available
Comment 51 Bernard Johnson 2007-03-06 00:05:53 EST
Package Review
==============

Key:
 - = N/A
 x = check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Buildroot is correct
(%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Rpmlint output:
     E: viewvc non-standard-uid /var/spool/viewvc apache
     E: viewvc non-standard-gid /var/spool/viewvc apache
     E: viewvc non-standard-dir-perm /var/spool/viewvc 0700

     The application requires these to run as a web application and create
     temporary files.  These files are also subject to containing confidential
     information.

     W: viewvc-selinux no-documentation

     There is no documentation for this package.
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must matches the base package %{name}, in the format
%{name}.spec.
 [x] Package meets the  Packaging Guidelines.
 [x] Package is licensed with an open-source compatible license and meet other
legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
 [x] 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.
 [x] Spec file is written in American English.
 [x] Spec file for the package is legible.
 [x] Sources used to build the package matches the upstream source, as provided
in the spec URL.
     MD5SUM this package    : 3d44ad485d38bf9f61d8111661260b4a
     MD5SUM upstream package: 3d44ad485d38bf9f61d8111661260b4a
 [-] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on:
 [x] Package is not known to require ExcludeArch, OR:
     Arches excluded:
     Why:
 [x] All build dependencies are listed in BuildRequires, except for any that are
listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [-] Package is not relocatable.
 [x] Package must own all directories that it creates.
 [-] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [x] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: fedora-6/i386
 [-] Package should compile and build into binary rpms on all supported
architectures.
     Tested on:
 [x] Package functions as described.
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files are correct.
 [x] File based requires are sane.
 [x] Latest version is packaged.

=== Issues ===
1.

=== Final Notes ===
1. It is understood that inherent knowledge of selinux policy types is not
recommended, however, this will only be included until the base policy includes
the information for this package.
2. Packager must obsolete -selinux package when policy is included in base policy.
3. Please inquire about the selinux policy updates required for EL4/EL5 (ie.
does your current open bug cover these or do you need a separate bug?).

================
*** APPROVED ***
================
Comment 52 Bojan Smojver 2007-03-07 00:46:20 EST
New Package CVS Request
=======================
Package Name: viewvc
Short Description: Browser interface for CVS and SVN version control
repositories
Owners: bojan@rexursive.com, bjohnson@symetrix.com
Branches: FC-6
InitialCC: 
Comment 53 Dennis Gilmore 2007-03-08 20:45:33 EST
Branched
Comment 54 Bojan Smojver 2007-03-08 22:59:50 EST
Build successful for devel and FC-6.
Comment 55 Christian Iseli 2007-06-05 12:26:52 EDT
Make Summary parser happy.
Comment 56 Bojan Smojver 2007-06-05 17:12:58 EDT
Regarding comment #55, could you elaborate on that a bit. Not sure I understand
what's required.
Comment 57 Christian Iseli 2007-06-06 05:56:57 EDT
(In reply to comment #56)
> Regarding comment #55, could you elaborate on that a bit. Not sure I understand
> what's required.

Sure.  When I produce the
http://fedoraproject.org/wiki/PackageMaintainers/PackageStatus
wiki page, the Summary field of review tickets is parsed by a script.
Your summary was missing a space ("viewvc-" instead of "viewvc -") and so the
script was looking for a package named viewvc- which does not exist.
I thus added the missing space and the next time the script is run it will not
flag that package has having potential problems.

HTH

Cheers,
 Christian
Comment 58 Bojan Smojver 2007-06-06 17:16:29 EDT
Ah, OK. I thought your comment had something to do with the actual viewvc code,
so I went on a little hunt to find out what exactly was the Summary parser in
viewvc :-)
Comment 59 Christian Iseli 2007-06-07 05:26:52 EDT
Oh, I see.  I guess I was a bit terse at first... sorry about that.
Comment 60 Bojan Smojver 2007-12-17 20:44:57 EST
New Package CVS Request
=======================
Package Name: viewvc
Short Description: Browser interface for CVS and SVN version control
repositories
Owners: bojan@rexursive.com, bjohnson@symetrix.com
Branches: EL-5 EL-4
InitialCC:
Cvsextras Commits:
Comment 61 Kevin Fenzi 2007-12-18 21:16:14 EST
cvs done.
Comment 62 Bojan Smojver 2015-07-09 03:06:08 EDT
New Package CVS Request
=======================
Package Name: viewvc
Short Description: Browser interface for CVS and SVN version control
repositories
Owners: bojan@rexursive.com, bjohnson@symetrix.com
Branches: epel7
InitialCC:
Comment 63 Jon Ciesla 2015-07-09 13:51:56 EDT
bojan@rexursive.com, bjohnson@symetrix.com are not FAS accounts.  Use FAS
account names, not email addresses.
Comment 64 Bojan Smojver 2015-07-09 17:20:52 EDT
New Package CVS Request
=======================
Package Name: viewvc
Short Description: Browser interface for CVS and SVN version control
repositories
Owners: bojan, bjohnson
Branches: epel7
InitialCC
Comment 65 Bojan Smojver 2015-08-18 01:27:45 EDT
Ping... Can we get the EPEL7 branch, please.
Comment 66 Bojan Smojver 2015-08-18 01:29:38 EDT
New Package CVS Request
=======================
Package Name: viewvc
Short Description: Browser interface for CVS and SVN version control
repositories
Owners: bojan, bjohnson
Branches: epel7
InitialCC
Comment 67 Jon Ciesla 2015-08-20 09:19:29 EDT
Invalid request, review flag not set.
Comment 68 Bojan Smojver 2015-08-20 10:08:33 EDT
New Package CVS Request
=======================
Package Name: viewvc
Short Description: Browser interface for CVS and SVN version control
repositories
Owners: bojan, bjohnson
Branches: epel7
InitialCC
Comment 69 Jon Ciesla 2015-08-20 10:18:51 EDT
WARNING: Invalid branch initialcc requested
WARNING: fedora-review flag set by review submitter!
Comment 70 Bojan Smojver 2015-08-20 10:23:26 EDT
New Package CVS Request
=======================
Package Name: viewvc
Short Description: Browser interface for CVS and SVN version control
repositories
Owners: bojan, bjohnson
Branches: epel7
InitialCC:
Comment 71 Jon Ciesla 2015-08-20 10:24:36 EDT
Still set by submitter, not reviewer.
Comment 72 Bojan Smojver 2015-08-20 17:39:21 EDT
Package Change Request
======================
Package Name: viewvc
New Branches: epel7
Owners:  bojan bjohnson

OK, so let's try with a slightly different text. This is not a review request, just need a new branch in git.
Comment 73 Jon Ciesla 2015-08-21 09:39:13 EDT
Git done (by process-git-requests).

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