Bug 452636 - Review Request: mod_proxy_html - Module to rewrite content as it passes through an apache proxy.
Review Request: mod_proxy_html - Module to rewrite content as it passes throu...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Jochen Schmitt
Fedora Extras Quality Assurance
http://www.djhsolutions.net/pprindevi...
:
: 453231 547930 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-24 03:41 EDT by Philip Prindeville
Modified: 2016-08-14 12:22 EDT (History)
10 users (show)

See Also:
Fixed In Version: 3.1.2-6.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-12-17 23:24:31 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
jochen: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Replacement .spec file (2.05 KB, text/plain)
2008-07-01 22:07 EDT, Philip Prindeville
no flags Details
Replacement .src.rpm file (23.57 KB, application/octet-stream)
2008-07-01 22:09 EDT, Philip Prindeville
no flags Details
Replacement .spec file (2.12 KB, text/plain)
2008-07-03 16:31 EDT, Philip Prindeville
no flags Details
Replacement .spec file (2.13 KB, application/octet-stream)
2008-07-11 02:07 EDT, Philip Prindeville
no flags Details
Replacement .src.rpm file (25.39 KB, application/octet-stream)
2008-07-11 02:08 EDT, Philip Prindeville
no flags Details
Replacement .src.rpm file (25.66 KB, application/octet-stream)
2008-07-13 17:22 EDT, Philip Prindeville
no flags Details
Replacement .spec file (2.48 KB, text/plain)
2008-07-13 17:23 EDT, Philip Prindeville
no flags Details
Replacement .src.rpm file (25.91 KB, application/octet-stream)
2008-08-12 12:20 EDT, Philip Prindeville
no flags Details
Replacement .spec file (2.61 KB, text/plain)
2008-08-12 12:21 EDT, Philip Prindeville
no flags Details
Replacement .spec file (2.51 KB, text/plain)
2008-09-02 19:25 EDT, Philip Prindeville
no flags Details
Replacement .src.rpm file (23.60 KB, application/octet-stream)
2008-09-02 19:27 EDT, Philip Prindeville
no flags Details
Replacement .spec file (2.66 KB, text/plain)
2008-09-02 19:34 EDT, Philip Prindeville
no flags Details
Replacement .src.rpm file (23.79 KB, application/octet-stream)
2008-09-02 19:35 EDT, Philip Prindeville
no flags Details
Replacement .spec file (2.78 KB, text/plain)
2008-09-19 11:30 EDT, Philip Prindeville
no flags Details
Replacement .src.rpm file (24.02 KB, application/octet-stream)
2008-09-19 11:31 EDT, Philip Prindeville
no flags Details
Replacement .spec file (2.92 KB, text/plain)
2008-09-19 11:58 EDT, Philip Prindeville
no flags Details
Replacement .src.rpm file (24.18 KB, application/octet-stream)
2008-09-19 12:02 EDT, Philip Prindeville
no flags Details
Replacement .spec file (2.99 KB, application/octet-stream)
2009-12-10 14:07 EST, Philip Prindeville
no flags Details
Replacement .src file (27.43 KB, application/x-rpm)
2009-12-10 14:08 EST, Philip Prindeville
no flags Details

  None (edit)
Description Philip Prindeville 2008-06-24 03:41:05 EDT
Spec URL: http://www.djhsolutions.com/pprindeville/httpd-mod_proxy_html.spec
SRPM URL: http://www.djhsolutions.com/pprindeville/httpd-mod_proxy_html-3.0.0-1.fc7.src.rpm
Description: Not sure if this is my first official request and I need a sponsor or not...  I've submitted things via surrogates before.  :-)
Comment 1 Ray Van Dolson 2008-06-28 12:49:10 EDT
Looks like I missed this and submitted one of my own a few days after you:

  Bug #453231

Yours was first so I am happy to defer, but take a glance at mine and let me
know what you want to do.  If you want, you can even use my spec and RPM as I
think it's a little closer to being "Fedora'ized" than yours is. :-)
Comment 2 Jason Tibbitts 2008-06-28 13:54:05 EDT
If you folks want to work together to produce a package for review, that's great
and I'll try to help.  But I agree that Philip's spec needs more work than
Ray's.  Things like Fedora not using Vendor or Packager, the package name not
matching what's used in Fedora (try "yum list mod_\*") and, honestly,
"Excludeos: windows"?
Comment 3 Philip Prindeville 2008-06-28 14:13:34 EDT
I have no problem merging them.  That's fine.

Some observations:

* we don't need -Wl,"-lxml2" in the invocation of apxs.  We can just use -lxml2 
directly;

* similarly, it would be preferable to install via "apxs -i -S 
LIBEXECDIR=$RPM_BUILD_ROOT/%{modulesdir} -n %{modname} %{modname}.la" as I've 
done; the .libs directory is an implementation detail of apxs using libtool 
that we shouldn't rely on;

Do we really need to supply the path to apxs?  It should be in the default 
search path.  If it's not, it might be because someone wants to try out a new 
version of it (in which case we should use that anyway).

Other than that, I'm fine with the rest of the changes.
Comment 4 Ray Van Dolson 2008-06-28 19:09:57 EDT
(In reply to comment #3)
> I have no problem merging them.  That's fine.
> 
> Some observations:
> 
> * we don't need -Wl,"-lxml2" in the invocation of apxs.  We can just use -lxml2 
> directly;

I'm definitely not an expert on apxs, does  -lxml2 get added automatically
somewhere later in the process?  I originally did not use it and resulted in a
.so that wasn't linked against libxml2.so.  Had to use LoadFile in Apache to get
things working.
 
> * similarly, it would be preferable to install via "apxs -i -S 
> LIBEXECDIR=$RPM_BUILD_ROOT/%{modulesdir} -n %{modname} %{modname}.la" as I've 
> done; the .libs directory is an implementation detail of apxs using libtool 
> that we shouldn't rely on;

That makes sense to me!

> Do we really need to supply the path to apxs?  It should be in the default 
> search path.  If it's not, it might be because someone wants to try out a new 
> version of it (in which case we should use that anyway).

I don't think we do, I just typically prefer to be more specific than less.

> Other than that, I'm fine with the rest of the changes.
> 

I'll merge in your suggestions to my spec file.  I don't have a burden to be the
maintainer particularly if you are interested, just let me know.
Comment 5 Ray Van Dolson 2008-06-28 19:43:08 EDT
Alright, I see what you mean about passing -lxml2 directly.  A much more obvious
solution. :-)

See my bug (#453231) for the latest spec and srpm.

Do you want to close this one out as a duplicate of mine or vice versa (the
request would need to be renamed to conform with other Fedora Apache modules)?
Comment 6 Philip Prindeville 2008-06-29 03:29:39 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > I have no problem merging them.  That's fine.
> > 
> > Some observations:
> > 
> > * we don't need -Wl,"-lxml2" in the invocation of apxs.  We can just use -
lxml2 
> > directly;
> I'm definitely not an expert on apxs, does  -lxml2 get added automatically
> somewhere later in the process?  I originally did not use it and resulted in a
> .so that wasn't linked against libxml2.so.  Had to use LoadFile in Apache to 
get
> things working.

I was just going by the example config file, which says that you need the 
LoadFile anyway...  So the fact that the module wasn't linked (statically) 
against libxml2.a was a good thing.  Plus it means that you can update versions 
of libxml2.so independently, which you lose if the module links statically 
against libxml2.a instead.

> > * similarly, it would be preferable to install via "apxs -i -S 
> > LIBEXECDIR=$RPM_BUILD_ROOT/%{modulesdir} -n %{modname} %{modname}.la" as 
I've 
> > done; the .libs directory is an implementation detail of apxs using libtool 
> > that we shouldn't rely on;
> That makes sense to me!
> > Do we really need to supply the path to apxs?  It should be in the default 
> > search path.  If it's not, it might be because someone wants to try out a 
new 
> > version of it (in which case we should use that anyway).
> I don't think we do, I just typically prefer to be more specific than less.

Well, for the person developing a new version of apxs, they might appreciate 
being able to test their changes against all the Apache modules without having 
to modify each and every one of the .spec files to do so.

> > Other than that, I'm fine with the rest of the changes.
> > 
> I'll merge in your suggestions to my spec file.  I don't have a burden to be 
the
> maintainer particularly if you are interested, just let me know.

Either way is fine with me.


Comment 7 Philip Prindeville 2008-06-29 04:01:51 EDT
(In reply to comment #6)
> (In reply to comment #4)
> > I'm definitely not an expert on apxs, does  -lxml2 get added automatically
> > somewhere later in the process?  I originally did not use it and resulted 
in a
> > .so that wasn't linked against libxml2.so.  Had to use LoadFile in Apache 
to 
> get
> > things working.
> I was just going by the example config file, which says that you need the 
> LoadFile anyway...  So the fact that the module wasn't linked (statically) 
> against libxml2.a was a good thing.  Plus it means that you can update 
versions 
> of libxml2.so independently, which you lose if the module links statically 
> against libxml2.a instead.

Actually, scratch that.

I just did a quick test of with and without -lxml2 in the initial compile of 
apxs.  In either case, libxml2.a is not statically linked.

The only difference seems to be that if you specify -lxml2, then that library 
is added to the dynamic load list for the shared object... and if you don't, 
then it must be explicitly loaded with the LoadFile called out in the config 
file.

Since the config file calls it out explicitly, and since it's normally loaded 
up manually anyway on other platforms, I'm inclined to stick with the slightly 
more complicated way of loading libxml2.so up explictly... just to be 
compatible with other distros.
Comment 8 Ray Van Dolson 2008-06-30 14:22:41 EDT
I'd like to get the opinion of the reviewer on that.  My feeling is that linking
libxml2.so against the module itself might be more Fedora-ish than using
LoadFile.   And as this RPM is for Fedora specifically....

Are you set up with a FAS account and all that good stuff?
Comment 9 Ray Van Dolson 2008-06-30 14:23:38 EDT
*** Bug 453231 has been marked as a duplicate of this bug. ***
Comment 10 Ray Van Dolson 2008-06-30 14:25:15 EDT
I have closed my bug as a duplicate of this one.

Spec: http://rayvd.fedorapeople.org/mod_proxy_html/mod_proxy_html.spec
SRPM: http://rayvd.fedorapeople.org/mod_proxy_html/mod_proxy_html-3.0.1-5.src.rpm

Phil, feel free to use my spec and SRPM there.  Would you be willing to add me
as a co-maintainer on this once it is approved?
Comment 11 Philip Prindeville 2008-06-30 14:32:34 EDT
(In reply to comment #8)
> I'd like to get the opinion of the reviewer on that.  My feeling is that 
linking
> libxml2.so against the module itself might be more Fedora-ish than using
> LoadFile.   And as this RPM is for Fedora specifically....
> Are you set up with a FAS account and all that good stuff?

No, no account yet.

I'll wait for an opinion of a reviewer.

Yeah, loading it might be more Fedoraish, but that's not necessarily a good 
thing.  Sometimes just having configs that move easily from one platform to 
another without changes is a bigger win.

(In reply to comment #10)
> Phil, feel free to use my spec and SRPM there.  Would you be willing to add me
> as a co-maintainer on this once it is approved?

Sure, that would be fine.
Comment 12 Philip Prindeville 2008-07-01 22:05:58 EDT
Asked on #apache-modules, and the consensus was that having explicit control of
where a shared-object library got loaded from was a good thing, especially if
you might want to try out a newer version of that library, etc.

I've added a "--with xml" option to build that adds it to the explicit list of
dependencies for the shared-object, but this is off by default and should
probably not be enabled in the distro release.
Comment 13 Philip Prindeville 2008-07-01 22:07:36 EDT
Created attachment 310748 [details]
Replacement .spec file
Comment 14 Philip Prindeville 2008-07-01 22:09:10 EDT
Created attachment 310749 [details]
Replacement .src.rpm file
Comment 15 Philip Prindeville 2008-07-03 16:31:06 EDT
Created attachment 310960 [details]
Replacement .spec file
Comment 16 Philip Prindeville 2008-07-11 02:07:45 EDT
Created attachment 311550 [details]
Replacement .spec file

Cleaned up 64-bit architecture issue.  Made it pass rpmlint.
Comment 17 Philip Prindeville 2008-07-11 02:08:33 EDT
Created attachment 311551 [details]
Replacement .src.rpm file

Built under FC8 on i386 processor.
Comment 18 Philip Prindeville 2008-07-11 02:09:44 EDT
Ran rpmlint on file.  It generates one warning, which is actually erroneous.

$ rpmlint mod_proxy_html.spec
mod_proxy_html.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 45, tab: line 40)
0 packages and 1 specfiles checked; 0 errors, 1 warnings.
$ 
Comment 19 Philip Prindeville 2008-07-13 17:22:44 EDT
Created attachment 311672 [details]
Replacement .src.rpm file
Comment 20 Philip Prindeville 2008-07-13 17:23:54 EDT
Created attachment 311673 [details]
Replacement .spec file

Fixed gcc warning about missing braces/ambiguous "else".
Comment 21 Philip Prindeville 2008-07-13 17:25:18 EDT
New rpmlint run:

$ rpmlint mod_proxy_html.spec ../RPMS/i386/mod_proxy_html-3.0.1-5.fc8.i386.rpm
mod_proxy_html.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 47, tab: line 42)
1 packages and 1 specfiles checked; 0 errors, 1 warnings.
$ 
Comment 22 Joe Orton 2008-08-12 07:13:57 EDT
non-formal review:

1) remove the Vendor, Packager, Provides and Excludeos tags
2) remove Requires: httpd, libxml2 - only Requires necessary should be:

Requires: httpd-mmn = %(cat %{_includedir}/httpd/.mmn || echo missing)

3) use an approved BuildRoot tag, see wiki packaging guidelines
4) Source: http://apache.webthing.com/mod_proxy_html/mod_proxy_html.tgz
is bad - do upstream not provide versioned URLs?
5) using %{_sbindir}/apxs throughout is probably a good idea

6) License: GPL should follow the licensing conventions on the wiki
http://fedoraproject.org/wiki/Licensing

7) the package should be called simply mod_proxy_html, no httpd- prefix necessary
Comment 23 Philip Prindeville 2008-08-12 12:01:52 EDT
(In reply to comment #22)
> non-formal review:
> 
> 1) remove the Vendor, Packager, Provides and Excludeos tags
> 2) remove Requires: httpd, libxml2 - only Requires necessary should be:
> 
> Requires: httpd-mmn = %(cat %{_includedir}/httpd/.mmn || echo missing)
> 
> 3) use an approved BuildRoot tag, see wiki packaging guidelines
> 4) Source: http://apache.webthing.com/mod_proxy_html/mod_proxy_html.tgz
> is bad - do upstream not provide versioned URLs?
> 5) using %{_sbindir}/apxs throughout is probably a good idea
> 
> 6) License: GPL should follow the licensing conventions on the wiki
> http://fedoraproject.org/wiki/Licensing
> 
> 7) the package should be called simply mod_proxy_html, no httpd- prefix
> necessary

The most recent version of the .rpm and .spec files are attached directly to this bug report.

I'll make the remaining fixes that you suggest that haven't been made (most already have).
Comment 24 Philip Prindeville 2008-08-12 12:20:14 EDT
Created attachment 314111 [details]
Replacement .src.rpm file
Comment 25 Philip Prindeville 2008-08-12 12:21:26 EDT
Created attachment 314112 [details]
Replacement .spec file
Comment 26 Philip Prindeville 2008-08-12 12:23:59 EDT
(In reply to comment #22)
> non-formal review:
> 
> 4) Source: http://apache.webthing.com/mod_proxy_html/mod_proxy_html.tgz
> is bad - do upstream not provide versioned URLs?

Unfortunately, they do not.

> 5) using %{_sbindir}/apxs throughout is probably a good idea

Ok.  I thought that allowing for the potential of testing an alternate version of apxs might be a good thing.

All other comments have been addressed.
Comment 27 Joe Orton 2008-08-13 07:23:21 EDT
(In reply to comment #26)
> (In reply to comment #22)
> > non-formal review:
> > 
> > 4) Source: http://apache.webthing.com/mod_proxy_html/mod_proxy_html.tgz
> > is bad - do upstream not provide versioned URLs?
> 
> Unfortunately, they do not.

upstream should be educated then ;)

You'll need to work around that and version the tarball manually, I think this is covered in the wiki somewhere.

> > 5) using %{_sbindir}/apxs throughout is probably a good idea
> 
> Ok.  I thought that allowing for the potential of testing an alternate version
> of apxs might be a good thing.

I'm not sure it really makes a difference, but it reduces predictability if apxs is picked from $PATH.

> All other comments have been addressed.

more nit-picking^W^Wreview:

1) this stuff is unnecessary obfuscation:

%define base proxy_html
%define modname mod_%{base}

Name: %{modname}

the spec file is for building mod_proxy_html, not an abstract httpd module; so use "Name: mod_proxy_html" and hard-code proxy_html as necessary and mod_proxy_html or %{name} otherwise.

2) the with-xml options should go; the module should be linked against -lxml2 and the conf file purged of LoadFile unconditionally.  The upstream method of providing a deliberately broken module is totally crazy and not something that should be supported (even as an option) in Fedora.

otherwise looks fine.
Comment 28 Philip Prindeville 2008-08-14 02:35:50 EDT
(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #22)
> > > non-formal review:
> > > 
> > > 4) Source: http://apache.webthing.com/mod_proxy_html/mod_proxy_html.tgz
> > > is bad - do upstream not provide versioned URLs?
> > 
> > Unfortunately, they do not.
> 
> upstream should be educated then ;)

That might be beyond the scope of this bug fix.  ;-)

In any case, I've tried contacting the owners but not gotten any traction with them.

> You'll need to work around that and version the tarball manually, I think this
> is covered in the wiki somewhere.

I looked at:

http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Non-Numeric_Version_in_Release

but didn't find it there.  Nor is it in:

http://fedoraproject.org/wiki/Packaging/SourceURL

Can you point me at the right guidelines?

Also, rpmlint doesn't seem to have issue with Source0: not containing %{version}.  Is that a shortcoming of rpmlint?

> > > 5) using %{_sbindir}/apxs throughout is probably a good idea
> > 
> > Ok.  I thought that allowing for the potential of testing an alternate version
> > of apxs might be a good thing.
> 
> I'm not sure it really makes a difference, but it reduces predictability if
> apxs is picked from $PATH.
> 
> > All other comments have been addressed.
> 
> more nit-picking^W^Wreview:
> 
> 1) this stuff is unnecessary obfuscation:
> 
> %define base proxy_html
> %define modname mod_%{base}
> 
> Name: %{modname}
> 
> the spec file is for building mod_proxy_html, not an abstract httpd module; so
> use "Name: mod_proxy_html" and hard-code proxy_html as necessary and
> mod_proxy_html or %{name} otherwise.
> 
> 2) the with-xml options should go; the module should be linked against -lxml2
> and the conf file purged of LoadFile unconditionally.  The upstream method of
> providing a deliberately broken module is totally crazy and not something that
> should be supported (even as an option) in Fedora.

Not sure I follow you.  Why does Apache support LoadFile then?  Or is it a Fedora policy to discourage using it?  (It's not documented in:

http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Addon_Packages_.28httpd.2C_pam.2C_and_SDL.29

so where should I be looking?)

> otherwise looks fine.
Comment 29 Ray Van Dolson 2008-08-14 12:03:40 EDT
I think either way would end up functionally the same; however, if we link against libxml2, we then get an implicit requires against libxml2 although I suppose you could just manually add an explicit Requires as well.

I'm not a technical expert on the subject at all however, so I'd be interested in hearing why LoadFile would be a better alternative than linking?
Comment 30 Patrice Dumas 2008-08-14 12:18:04 EDT
(In reply to comment #29)
> I think either way would end up functionally the same; however, if we link
> against libxml2, we then get an implicit requires against libxml2 although I
> suppose you could just manually add an explicit Requires as well.
> 
> I'm not a technical expert on the subject at all however, so I'd be interested
> in hearing why LoadFile would be a better alternative than linking?

Using a loadfile is certainly not the way to go in fedora. Linking against libxml2 doesn't only add an automatic requires, this is a mere side effect
of adding the soname dependency on the right libxml2 version in the binary
(something along libxml2.so.2). The binary will refuse to run if the libxml2
shared object with the right version isn't found.

There are ways to use another library at runtime (certainly by setting
LD_LIBRARY_PATH), using LoadFile (which certainly does a dlopen) is not
right. 

Ther are cases where using LoadFile or similar are right (like plugins
for example), but here it doesn't seems to be the case at all.
Comment 31 Philip Prindeville 2008-09-02 19:25:17 EDT
Created attachment 315602 [details]
Replacement .spec file
Comment 32 Philip Prindeville 2008-09-02 19:27:11 EDT
Created attachment 315603 [details]
Replacement .src.rpm file
Comment 33 Philip Prindeville 2008-09-02 19:29:43 EDT
(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #22)
> > > non-formal review:
> > > 
> > > 4) Source: http://apache.webthing.com/mod_proxy_html/mod_proxy_html.tgz
> > > is bad - do upstream not provide versioned URLs?
> > 
> > Unfortunately, they do not.
> 
> upstream should be educated then ;)
> 
> You'll need to work around that and version the tarball manually, I think this
> is covered in the wiki somewhere.

Upstream has cooperated in adding numbered versions.

All other issues have been addressed.

Results from rpmlint:

$ rpmlint mod_proxy_html.spec ../RPMS/i386/mod_proxy_html-debuginfo-3.0.1-7.fc8.i386.rpm 
1 packages and 1 specfiles checked; 0 errors, 0 warnings.
$
Comment 34 Philip Prindeville 2008-09-02 19:34:09 EDT
Created attachment 315605 [details]
Replacement .spec file

Forgot to update ChangeLog.
Comment 35 Philip Prindeville 2008-09-02 19:35:02 EDT
Created attachment 315606 [details]
Replacement .src.rpm file

Forgot to edit ChangeLog.
Comment 36 Philip Prindeville 2008-09-06 15:12:31 EDT
Quick question: should the LoadModule that enables this functionality be uncommented from the config file by default by installation?  Or is that something that should be enabled explicitly by the end-user uncommenting that line?
Comment 37 Philip Prindeville 2008-09-19 11:30:18 EDT
Created attachment 317204 [details]
Replacement .spec file
Comment 38 Philip Prindeville 2008-09-19 11:31:08 EDT
Created attachment 317205 [details]
Replacement .src.rpm file
Comment 39 Joe Orton 2008-09-19 11:42:35 EDT
You lost "BuildRequires: libxml2-devel httpd-devel" somewhere along the way - if that was a result of my comments about losing the Requires, my apologies.

The LoadModule for the module should be uncommented by default; i.e. the module should be loaded by default, when installed.  If someone doesn't want the module loaded, they can remove the package :)

Other than that, this looks great, thanks for persevering!

I'm not an Official Sponsor so I can't do the formal review unfortunately.
Comment 40 Philip Prindeville 2008-09-19 11:58:54 EDT
Created attachment 317207 [details]
Replacement .spec file

Put back BuildRequires: as before.
Comment 41 Philip Prindeville 2008-09-19 12:02:20 EDT
Created attachment 317209 [details]
Replacement .src.rpm file
Comment 42 Erik S. LaBianca 2009-07-17 15:07:05 EDT
Just as a point of interest, I downloaded the .src.rpm from comment #1 and was able to build and use it perfectly on CentOS 5.
Comment 43 Philip Prindeville 2009-07-17 15:51:36 EDT
Posting to fedora-devel today to ask for a sponsor to be assigned.

This fix has been good to go for 10 months now.
Comment 45 Itamar Reis Peixoto 2009-07-17 20:20:00 EDT
(In reply to comment #44)

oh my god! , I am sorry, posted in wrong bug, please ignore my previous post 

:-(

my mistake
Comment 46 Jon Ciesla 2009-07-22 08:06:51 EDT
Philip, I can sponsor and do the formal review.  I'll start on that today.  While I'm on that, can you point me to a few practice reviews you've done?
Comment 47 Jon Ciesla 2009-07-22 09:09:02 EDT
Formal review:

#  MUST: rpmlint must be run on every package. The output should be posted in the review.[1]

Clean.

# MUST: The package must be named according to the Package Naming Guidelines .

OK.  Might want to change Source: to Source0:

# MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. [2] .

OK.

# MUST: The package must meet the Packaging Guidelines .

OK.

# MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .

OK.

# MUST: The License field in the package spec file must match the actual license. [3]

OK.

# 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 must be included in %doc.[4]

OK.

# MUST: The spec file must be written in American English. [5]

OK.

# MUST: The spec file for the package MUST be legible. [6]

OK.

# MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.

OK.

# MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [7]

OK.

# MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. [8]

N/A

# MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.

OK.

# MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.[9]

OK.

# MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. [10]

N/A

# MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. [11]

N/A

# MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. [12]

OK.

# MUST: A Fedora package must not list a file more than once in the spec file's %files listings. [13]

OK.

# MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. [14]

OK.

# MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [15]

OK.

# MUST: Each package must consistently use macros. [16]

OK.

# MUST: The package must contain code, or permissable content. [17]

OK.

# MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). [18]

N/A

# MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. [18]

OK.

# MUST: Header files must be in a -devel package. [19]

N/A

# MUST: Static libraries must be in a -static package. [20]

N/A

# MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). [21]

N/A

# MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. [19]

Apache module, N/A

# MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} [22]

N/A

# MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[20]

OK.

# MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. [23]

N/A

# MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. [24]

OK.

# MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [25]

OK.

# MUST: All filenames in rpm packages must be valid UTF-8. [26]

OK.


Looks great and can be approved once I'm satisfied enough with practice reviews to sponsor.  That said, it looks like you've learned a lot in this review, so I'm thinking this shouldn't take long at all.
Comment 48 Jon Ciesla 2009-07-23 07:55:36 EDT
FYI, Philip, I attempted to reply to your email, but:

   ----- The following addresses had permanent fatal errors -----
<philipp@redfish-solutions.com>
    (reason: 554 mail.redfish-solutions.com ESMTP not accepting messages)

   ----- Transcript of session follows -----
... while talking to mail.redfish-solutions.com.:
<<< 554 mail.redfish-solutions.com ESMTP not accepting messages
554 5.0.0 Service unavailable
Comment 49 Philip Prindeville 2009-07-23 14:26:45 EDT
(In reply to comment #48)
> FYI, Philip, I attempted to reply to your email, but:
> 
>    ----- The following addresses had permanent fatal errors -----
> <philipp@redfish-solutions.com>
>     (reason: 554 mail.redfish-solutions.com ESMTP not accepting messages)
> 
>    ----- Transcript of session follows -----
> ... while talking to mail.redfish-solutions.com.:
> <<< 554 mail.redfish-solutions.com ESMTP not accepting messages
> 554 5.0.0 Service unavailable  

I added whitelisting for 76.204.165.225.
Comment 50 Jochen Schmitt 2009-12-10 13:57:53 EST
OK, I have take a short look on your package and have to recorgnise, that the packaged release is obsolete, because this is an old review.

So it may be nice, if you can create a package for the current release 3.1.

If this package may ok, I'm willing to sponsor you.

Best Regards:

Jochen Schmitt
Comment 51 Philip Prindeville 2009-12-10 14:07:11 EST
Created attachment 377563 [details]
Replacement .spec file

Bump to 3.1.2.
Comment 52 Philip Prindeville 2009-12-10 14:08:18 EST
Created attachment 377565 [details]
Replacement .src file
Comment 53 Jochen Schmitt 2009-12-10 14:29:57 EST
Good:
+ Could download upstream sources via spectool -g
+ Packaged tar ball matches with upstr 
(md5sum: d6497b48d2bffc4150472472004618c3)
+ Local build works fine
+ Rpmlint is silent on source package
+ Rpmlint is silent on binary package
+ Rpmlint is silent on debuginfo package
+ Scratch build on koji works fine

*** APPROVED ***

Please do the following step:
1.) Create a FAS account
2.) Fullfill the CLA
3.) Request membership in the packages group
4.) Send me your FAS-id for sponsoring.

A additional advice for future reviews:

Please try to get a webspace and upload the SPEC file and the source rpms to it. and post the URLs in the bug report. Of course you can take the space on fedorapeople.org which you will get after you has a valid FAS account.

Best Regards

Jochen Schmitt
Comment 54 Philip Prindeville 2009-12-10 14:46:53 EST
Jochen: id is "philipp".  I just sent in the packagers group request.

Thanks.
Comment 55 Jochen Schmitt 2009-12-10 14:57:16 EST
I have approve your membership request. so you can now create a CVSAdmin request for your package.
Comment 57 Philip Prindeville 2009-12-15 20:46:33 EST
*** Bug 547930 has been marked as a duplicate of this bug. ***
Comment 58 Philip Prindeville 2009-12-15 20:57:03 EST
New Package CVS Request
=======================
Package Name: mod_proxy_html
Short Description: reverse-proxy output filter for HTML
Owners: philipp
Branches: F-11 F-12
InitialCC: s4504kr limb
Comment 59 Ray Van Dolson 2009-12-15 21:08:58 EST
Please branch for EL-4 and EL-5 as well (if willing).
Comment 60 Philip Prindeville 2009-12-15 21:18:05 EST
New Package CVS Request
=======================
Package Name: mod_proxy_html
Short Description: reverse-proxy output filter for HTML
Owners: philipp
Branches: F-11 F-12 EL-4 EL-5
InitialCC: s4504kr limb
Comment 61 Kevin Fenzi 2009-12-16 00:30:47 EST
cvs done.
Comment 62 Fedora Update System 2009-12-16 17:53:13 EST
mod_proxy_html-3.1.2-5.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/mod_proxy_html-3.1.2-5.fc11
Comment 63 Fedora Update System 2009-12-16 17:54:51 EST
mod_proxy_html-3.1.2-5.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/mod_proxy_html-3.1.2-5.fc12
Comment 64 Fedora Update System 2009-12-16 19:00:13 EST
mod_proxy_html-3.1.2-6.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/mod_proxy_html-3.1.2-6.fc11
Comment 65 Fedora Update System 2009-12-16 19:00:18 EST
mod_proxy_html-3.1.2-6.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/mod_proxy_html-3.1.2-6.fc12
Comment 66 Fedora Update System 2009-12-17 23:21:29 EST
mod_proxy_html-3.1.2-6.fc12 has been pushed to the Fedora 12 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 mod_proxy_html'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2009-13279
Comment 67 Fedora Update System 2009-12-17 23:24:23 EST
mod_proxy_html-3.1.2-5.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 68 Fedora Update System 2009-12-17 23:37:03 EST
mod_proxy_html-3.1.2-5.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 69 Fedora Update System 2010-01-07 16:45:46 EST
mod_proxy_html-3.1.2-6.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 70 Fedora Update System 2010-01-07 16:54:50 EST
mod_proxy_html-3.1.2-6.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

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