Several problems:

1. missing a initscript
http://fedoraproject.org/wiki/Packaging:SysVInitScript

2.-devel subpackage is not needed in this package

3.
%doc %{_defaultdocdir}/%{name}-%{version}/%{name}.conf

%doc is not needed in %{_defaultdocdir}

4.

Group:     Development/Tools is not appropriate

5.Please do not gzip man page by yourself, rpmbuild will do it automatically.

6. most packagers/reviewers don't like use unnecessary macros, e.g. %{__install} %{__gzip}

Some reference for you:
1.http://fedoraproject.org/wiki/How_to_create_an_RPM_package
2.http://fedoraproject.org/wiki/PackagingGuidelines

Spec URL: http://aquini.tchesoft.com/RPMS/mongoose/mongoose.spec
SRPM URL: http://aquini.tchesoft.com/RPMS/mongoose/mongoose-2.8-2.fc12.src.rpm

MD5:
fa58e9038709d389b2fc7c6cffc9c068  mongoose-2.8-2.fc12.src.rpm
3ffb7344a8f8cb8056cfa2c8e220649c  mongoose.spec


Chen,
Thanks for reviewing the spec.

I did make some of the modifications you suggested, as follows:
[2] -devel package was stripped off the spec file;
[3] The misusing of %doc was fixed;
[4] Group changed to Applications/System -- IMHO, I do not think it could fit in anyone else in System Environment class;
[5] Now the man page compression is done by rpmbuild itself.

Unfortunately, there was some suggestions of yours I could not deliver as promptly as you wish: 
[1] I do not see a real necessity to ship a SysV initscrip, since mongoose is a regular user tool not intended to work as a daemon. The purpose of mongoose is to give flexibility to an ordinary user (not administrator) when he/she needs to set up quickly an web server to share files in a specific directory, or just do some tests. Please, let me know if you observation [1] is a real blocker to this package. 

[6] As you may notice, there is no install section on Makefile delivered by upstream, so I have the real need of being using %{__install} macros. IMHO, it  would be preferable using this macros than the commands itself, wouldn' be? Please, let me know if you observation [6] is also a real blocker to this package.  

Once again, thanks a lot for your attention on this matter.

The %setup macro takes arguments you can use to write simpler spec files.
Full documentation: http://www.rpm.org/max-rpm/s1-rpm-inside-macros.html

The %doc macro takes files in the build dir (inside the unpacked tarball)
and puts files in %{_docdir}/%{name}-%{version}.

install has a -D option you can use.

It's normal to list man pages this way:
 %{_mandir}/man1/%{name}.1*

spec file where these tips are implemented:

spec: http://terjeros.fedorapeople.org/mongoose/mongoose.spec
srpm: http://terjeros.fedorapeople.org/mongoose/mongoose-2.8-3.fc12.src.rpm

> do some tests. Please, let me know if you observation [1] is a real blocker to
> this package. 
> [6] As you may notice, there is no install section on Makefile delivered by
> upstream, so I have the real need of being using %{__install} macros. IMHO, it 
> would be preferable using this macros than the commands itself, wouldn' be?

1.New packagers should avoid of using unnecessarily macros, e.g. %{__rm} %{__install} %{__ln_s} etc, those macros are useless for fedora.

2. You can include examples in %doc


See https://fedoraproject.org/wiki/Packaging_tricks

3. As Terje RÃ said, you should list man pages in this way: %{_mandir}/man1/%{name}.1*, because fedora may switch manpages from gz to other compression format in some time.

Terje and Chen,
Thanks a lot for your attention, and for the valuable guidance and review. 

Here are the new Spec and SRPM based on several suggestions of yours:
Spec URL: http://aquini.tchesoft.com/RPMS/mongoose/mongoose.spec
SRPM URL: http://aquini.tchesoft.com/RPMS/mongoose/mongoose-2.8-3.fc12.src.rpm

MD5 sum:
099c517688eed8f2505856b90b4c334f  mongoose.spec
89d928ea31bf80677157ea0c7fe30a85  mongoose-2.8-3.fc12.src.rpm

Best regards.

1.Patch1 is not enough, you should also remove some gcc switches in CFLAGS: -W -Wall -std=c99 -pedantic -Os
2.
Don't add source to %doc( %{name}.c %{name}.h Makefile main.c)
3.
use install instead of %{__install}

rpm --eval %__install
/usr/bin/install

rpm --eval %__make
/usr/bin/make

rpm --eval %__rm
/usr/bin/rm

(In reply to comment #6)
> 1.Patch1 is not enough, you should also remove some gcc switches in CFLAGS: -W
> -Wall -std=c99 -pedantic -Os
make CFLAGS="$RPM_OPT_FLAGS" linux

> 3.
> use install instead of %{__install}
> 
> rpm --eval %__install
> /usr/bin/install
> 
> rpm --eval %__make
> /usr/bin/make
> 
> rpm --eval %__rm
> /usr/bin/rm    

No idea why you are fighting these macros. These macros are correct, there is nothing wrong in using them.

(In reply to comment #7)
> (In reply to comment #6)
> > 1.Patch1 is not enough, you should also remove some gcc switches in CFLAGS: -W
> > -Wall -std=c99 -pedantic -Os
> make CFLAGS="$RPM_OPT_FLAGS" linux
> > 3.
> > use install instead of %{__install}
> > 
> > rpm --eval %__install
> > /usr/bin/install
> > 
> > rpm --eval %__make
> > /usr/bin/make
> > 
> > rpm --eval %__rm
> > /usr/bin/rm    
> No idea why you are fighting these macros. These macros are correct, there is
> nothing wrong in using them.    

Several months, there's a disscussion in fedora-packaging maillist most packagers object using those unesscessary macros.

Actually for seasoned packagers, I think use those macros are acceptable if they can use them consistently throughout the spec.

But, for new packagers, they can hardly use those macros consistently, taken this spec for a example: The packager choose %{__install}, but still use rm, make, mkdir. 
This breaks the principle of pick one packaging style and use it consistently.

For me, I always suggest new packagers not to use those macros, few new packagers can remember those rpm macros which have shorten alternative commands.

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > 1.Patch1 is not enough, you should also remove some gcc switches in CFLAGS: -W
> > > -Wall -std=c99 -pedantic -Os
> > make CFLAGS="$RPM_OPT_FLAGS" linux
> > > 3.
> > > use install instead of %{__install}
> > > 
> > > rpm --eval %__install
> > > /usr/bin/install
> > > 
> > > rpm --eval %__make
> > > /usr/bin/make
> > > 
> > > rpm --eval %__rm
> > > /usr/bin/rm    
> > No idea why you are fighting these macros. These macros are correct, there is
> > nothing wrong in using them.    
> 
> Several months, there's a disscussion in fedora-packaging maillist most
> packagers object using those unesscessary macros.
Still, there is nothing wrong in using them nor did we ban them.
 
> Actually for seasoned packagers, I think use those macros are acceptable if
> they can use them consistently throughout the spec.
Actually, technically these macros are superior and cleaner than not using them.

The problems with these macros are elsewhere: Once they are being used, rpm can't easily get rid of them.


> But, for new packagers, they can hardly use those macros consistently, taken
> this spec for a example: The packager choose %{__install}, but still use rm,
> make, mkdir. 
> This breaks the principle of pick one packaging style and use it consistently.
> 
> For me, I always suggest new packagers not to use those macros, few new
> packagers can remember those rpm macros which have shorten alternative
> commands.    
Well, for the moment, I abstain to furtherly comment on this. Only so far, you are enforcing a non existing rule.

If there are no policy against macros let the packager decide?

Some more, more important comments:

 o should the python bindings be shipped?
 o does it make sense to ship and enable a default mongonse.conf
   in /etc/mongoose.conf by adding 
     -DCONFIG_FILE=\"%{_sysconfdir}/mongoose.conf\" to make and
   install mongoose.conf in correct location? 
 o ship .c and .h file in devel package for use in embedded mode?
 o mongoose seems to do some funky things with SSL. There are no req.
   on ssl libs, but package seems to have SSL support.

(In reply to comment #10)
> If there are no policy against macros let the packager decide?
Exactly.


>  o does it make sense to ship and enable a default mongonse.conf
>    in /etc/mongoose.conf by adding 
>      -DCONFIG_FILE=\"%{_sysconfdir}/mongoose.conf\" to make and
>    install mongoose.conf in correct location? 
IMO, yes. I was about to propose the same, but you were faster ;)

(In reply to comment #11)
> (In reply to comment #10)
> > If there are no policy against macros let the packager decide?
> Exactly.
I agree with this, but one packager should choose one packaging style. If he like using macros, then he'd better to also use %{__rm} %{__make} instead of rm or make.

There's also a disscussion on devel list.
http://lists.fedoraproject.org/pipermail/devel/2010-March/133466.html

Hello Terje, Ralf and Chen.

Firstly, thank you guys a lot for giving me such attention!

(In reply to comment #10)
> Some more, more important comments:
> 
>  o should the python bindings be shipped?
Could you point me some doc on that, please? I'm kind of python 'unaware' :(

>  o does it make sense to ship and enable a default mongonse.conf
>    in /etc/mongoose.conf by adding 
>      -DCONFIG_FILE=\"%{_sysconfdir}/mongoose.conf\" to make and
>    install mongoose.conf in correct location? 
OK, that will be done in the next Spec.

>  o ship .c and .h file in devel package for use in embedded mode?
Those were shipped in the first version, in a -devel rpm. Chen has pointed it as unnecessary and I stripped it off from Spec. I can reinclude the %package section and ship those files again.

>  o mongoose seems to do some funky things with SSL. There are no req.
>    on ssl libs, but package seems to have SSL support.
In SSL mode, mongoose tries to load libssl.so on demand -- pretty much like apache does with its modules. It was my fault not include the libssl-devel req. My question, now, is that: Should I include this -devel req in the ordinary rpm or should I ship it as another rpm (like  -ssl), just to fulfill this req, that users must install if they want SSL support?


(In reply to comment #12)
Chen, as it seems that you ended up agreeing with Terje and Ralf, I will adopt your last suggestion and just use macros from now on (until we haven't another guidance).Please do not take it as personal, I just really think that macros are useful and cleaner than simple cmds.


Guys, thanks again! In a couple days I will be posting these new improvements...

Please, let me aware of anything else meanwhile!

Best regards.

(In reply to comment #13)


> >  o ship .c and .h file in devel package for use in embedded mode?
> Those were shipped in the first version, in a -devel rpm. Chen has pointed it
> as unnecessary and I stripped it off from Spec. I can reinclude the %package
> section and ship those files again.

You can split out libmongoose and libmongoose-devel as subpackages or simply drop those files. Include source files in rpm is not a right way.

> >  o mongoose seems to do some funky things with SSL. There are no req.
> >    on ssl libs, but package seems to have SSL support.
> In SSL mode, mongoose tries to load libssl.so on demand -- pretty much like
> apache does with its modules. It was my fault not include the libssl-devel req.
> My question, now, is that: Should I include this -devel req in the ordinary rpm
> or should I ship it as another rpm (like  -ssl), just to fulfill this req, that
> users must install if they want SSL support?
If the shlib is dlopened, you should not add it as a dependency.

> (In reply to comment #12)
> Chen, as it seems that you ended up agreeing with Terje and Ralf, I will adopt
> your last suggestion and just use macros from now on (until we haven't another
> guidance).Please do not take it as personal, I just really think that macros
> are useful and cleaner than simple cmds.

Few sponsors like this style.
See http://lists.fedoraproject.org/pipermail/devel/2010-March/133466.html

Also few crital packages in fedora use those macros. 

http://cvs.fedoraproject.org/viewvc/rpms/kernel/devel/kernel.spec?view=markup
http://cvs.fedoraproject.org/viewvc/rpms/glibc/devel/glibc.spec?view=markup
http://cvs.fedoraproject.org/viewvc/rpms/gtk2/devel/gtk2.spec?revision=1.430&view=markup

I agree with Ralf that no guideline against using of those macros, but I still 
suggest you not to use them. Actually, those macros are not documented in fedora guideline.

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

Created attachment 415100 [details]
Email - upstream reply

Sergey Lyubka replies some questions raised over the mongoose.spec review.

Hello all,

Yesterday I talked to Sergey Lyubka, upstream mongoose developer, and for the sake of brevity I will quote his major thoughts bellow. The entire email is attached to this ticket also.

" Mongoose supports SSL but does not depend on the SSL library.
This is because SSL support is best-effort: Mongoose tries to load libssl.so
dynamically.
If it is present on the system, Mongoose will support SSL, otherwise not.

Regarding devel package, honestly I would not create it at all. "

Facing that, should I really ship the -devel package? Are the questions concerning SSL dependency clarified?

Please, give me some thoughts about it, people.


Thanks in advance for your attention on this matter.

(In reply to comment #16)

> " Mongoose supports SSL but does not depend on the SSL library.
> This is because SSL support is best-effort: Mongoose tries to load libssl.so
> dynamically.
> If it is present on the system, Mongoose will support SSL, otherwise not.

That's not the way to do it under Linux. The normal way to use shared libs is to directly link them in.

If, for some reasons, they need to be dlopen'ed, then the "versioned libraries" libraries need to be dlopen'ed, not the unversioned libraries.

I.e. dlopening libopenssl.so is wrong.
It should be libopenssl.so.<something> => This package needs to be patched.

BTW: I just noticed, in mongoose, the same consideration applies to libcrypto.so.*.

Ralf,

(In reply to comment #17)
> If, for some reasons, they need to be dlopen'ed, then the "versioned libraries"
> libraries need to be dlopen'ed, not the unversioned libraries.
> 
> I.e. dlopening libopenssl.so is wrong.
> It should be libopenssl.so.<something> => This package needs to be patched.
> 
> BTW: I just noticed, in mongoose, the same consideration applies to
> libcrypto.so.*.

I think it would be a good approach apply a patch to fix that two #defines SSL_LIB and CRYPTO_LIB in mongoose.c, wouldn't it? That way we fix this issue with dlopen'ed non-versioned libraries without asking for great modifications on the whole project. I can ask upstream for a patch, or I can provide that patch from myself.

If so, it will remain me one more question: Just after patching mongoose.c to fix the SSL requisites it would be advisable to include openssl as a req to mongoose rpm installation, wouldn't be?


Once again, thanks for your attention

Regards.

(In reply to comment #18)
 
> I think it would be a good approach apply a patch to fix that two #defines
> SSL_LIB and CRYPTO_LIB in mongoose.c, wouldn't it?
This is what I would do.

I'd add something similar to this to mongoose.c 
#ifndef SSL_LIB
#define SSL_LIB "libssl.so"
#endif
and use something similar to this
make CFLAGS="${RPM_OPT_FLAGS} -DSSL_LIB=\"libssl.so.<version>\""

Spec URL: http://aquini.tchesoft.com/RPMS/mongoose/mongoose.spec
SRPM URL: http://aquini.tchesoft.com/RPMS/mongoose/mongoose-2.8-4.fc12.src.rpm

MD5:
82d77f3cac55d6bcf706f50ab49c79a6  mongoose.spec
8f2ac61063f9cf9e3c24e7b9f531acf2  mongoose-2.8-4.fc12.src.rpm

Ok, here we go for another review round. ;)

Ralf: The issue of dlopen'ed non-versioned openssl shared libs was addressed exactly as you have suggested. Now SSL_LIB and CRYPTO_LIB can be defined to override mongoose's default values in build time. 


Terje: I do not include an openssl req in the package, hence mongoose does a very neat job when it calls dlopen() to the SSL_LIB and CRYPTO_LIB. So, if mongoose runs into a situation in it can't find those libs in the system, all of its SSL support will be disabled without breaking any other part of the software. (Please, let me know if that is OK)
 I also do not ship mongoose.conf as you suggested, because I realized if I done that mongoose wouldn't be self-sufficient (as is stated in its description) and it wouldn't fit its major goal of being perfect for all sorts of demos, quick tests, file sharing, and Web programming. In other words, any attempt of an ordinary user to start mongoose without pointing another conf file would crash due to insufficient permissions to bind privileged ports. Firstly I thought it would be better having the conf file in /etc instead of pwd(). However, it has just caused more annoyances than pleasure when using mongoose.


Chen: Now, there is no trace of any source file shipped at all.


Guys, I really look forward your feedback. Please, do no hesitate in review this package and send me any suggestion or improvements.

Thank you very much for your attention!

Best regards.

Hello Rafael, 

  Comments, inline.

>  Ralf Corsepius      2010-05-19 09:31:16 EDT 
>
>> " Mongoose supports SSL but does not depend on the SSL library.
>> This is because SSL support is best-effort: Mongoose tries to load libssl.so
>> dynamically.
>> If it is present on the system, Mongoose will support SSL, otherwise not.
>
>That's not the way to do it under Linux. The normal way to use shared libs is
>to directly link them in.
>
>If, for some reasons, they need to be dlopen'ed, then the "versioned libraries"
>libraries need to be dlopen'ed, not the unversioned libraries.
>
>I.e. dlopening libopenssl.so is wrong.
>It should be libopenssl.so.<something> => This package needs to be patched.
>
>BTW: I just noticed, in mongoose, the same consideration applies to
>libcrypto.so.*.    

IMO, your spec looks good, also followed all requests above.
However, since using the approach for versioning the libssl as described early, please also add RequireBuild field for openssl package, and make a script in order to check dynamically the correct version of ssl library. Using this can avoid break the package (since currently is static - 1.0.0) for future release on Fedora.

Thanks!

Cheers
Douglas

Hello Douglas,

Thanks a lot for taking time and providing me such valuable 
suggestion.

I did all modifications you have suggested as inserting "BuildRequires: 
openssl-devel", and also as inserting a small script before calling "make" at %build section.
Please, let me know if that approach is clean and neat enough in its trying to address the fix suggested.

I look forward your comments!

Cheers.

--
Spec URL: http://aquini.tchesoft.com/RPMS/mongoose/mongoose.spec
SRPM URL: http://aquini.tchesoft.com/RPMS/mongoose/mongoose-2.8-5.fc12.src.rpm

MD5:
01c8e71bb28beccfc200104db176019c  mongoose.spec
9ff618fdbb61ecbaf65c9831a41b9564  mongoose-2.8-5.fc12.src.rpm

Hello Rafael,

> I did all modifications you have suggested as inserting "BuildRequires: 
> openssl-devel", and also as inserting a small script before calling "make" at
> %build section.
> Please, let me know if that approach is clean and neat enough in its trying to
> address the fix suggested.
> 
> I look forward your comments!

According with the last spec/src.rpm ACK from my side. To my eyes your package looks good. 

Hopefully, soon someone with the right permissions will post the final review/approval to your package.

Cheers
Douglas

PING...

I'm still looking forward sponsorship. Besides this submission, in order to show my understanding of Package Review Guidelines, I've also done some informal reviews as follows:
https://bugzilla.redhat.com/show_bug.cgi?id=592770
https://bugzilla.redhat.com/show_bug.cgi?id=595011

Thanks in advance for your attention!

Best regards.

Good:
* Package is named mongoose which follows the upstream project name
* spec file naming follows package naming
* License in spec and sources is MIT which is open source
* No license text included in the tarball so no need to include one.
* Spec is legible and American English
* http://mongoose.googlecode.com/files/mongoose-2.8.tgz
* 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
* Patch files both have upstream issues opened and look good for our purposes
* Builds in koji  (See Needswork below for one change that I implemented when building in koji)
* rpmlint is clean:
  3 packages and 0 specfiles checked; 0 errors, 0 warnings.
* Ran mongoose on a directory of html files nad it served them up on port 8080

Needswork:
* We can't call rpm from within an rpm spec file since we aren't assured that
  the rpm that created the build environment is compatible with the rpm that
  would be run within the environment.  Here's a different command that would
  work:
  LIBV=$(find '%{_libdir}' | grep -E '/libssl.so.[0-9]?[0-9]?$' | sed -e 's!%{_libdir}/libssl.so.!!')
* Ask upstream to include a license file.

APPROVED

Remember to fix the LIBV line in the spec file before you import and build.

I'm sponsoring you and approving the package.  Feel free to ask me any
questions you have.  I'm abadger1999 on irc.freenode.net (almost always in
#fedora-admin) and toshio [at] fedoraproject.org

Toshio,

I fixed that approach to grab the correct versioned shared libraries as you have suggested, and I also opened an issue asking upstream to ship a license text file within mongoose's .tar file (http://code.google.com/p/mongoose/issues/detail?id=159). I also did successfull mock & koji build tests for dist-f12 & dist-f13 branches.

Thanks for taking time to review this package and sponsor me as a Fedora packager.

Best regards

--
Spec URL: http://aquini.tchesoft.com/RPMS/mongoose/mongoose.spec
SRPM URL: http://aquini.tchesoft.com/RPMS/mongoose/mongoose-2.8-6.fc12.src.rpm

MD5:
979fc85b1e6c5f812afca7a8441ef879  mongoose.spec
e0a4b6be5e68c785cc5da0811dfe2ca7  mongoose-2.8-6.fc12.src.rpm

New Package CVS Request
=======================
Package Name: mongoose
Short Description: An easy-to-use self-sufficient web server
Owners: aquini
Branches: F-12 F-13
InitialCC:

CVS done (by process-cvs-requests.py).

mongoose-2.8-6.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/mongoose-2.8-6.fc12

mongoose-2.8-6.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/mongoose-2.8-6.fc13

mongoose-2.8-6.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

mongoose-2.8-6.fc12 has been pushed to the Fedora 12 stable repository.  If problems still persist, please make note of it in this bug report.

Package Change Request
======================
Package Name: mongoose
New Branches: el5 el6
Owners: aquini

Git done (by process-git-requests).

mongoose-2.8-6.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/mongoose-2.8-6.el5

mongoose-2.8-6.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

mongoose-3.0-1.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/mongoose-3.0-1.fc14

mongoose-3.0-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/mongoose-3.0-1.fc15

mongoose-3.0-1.fc15 has been pushed to the Fedora 15 stable repository.  If problems still persist, please make note of it in this bug report.

mongoose-3.0-1.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.