[Bug 673589] Review Request: UpTools - C++ library for hpc, networking, db, memory, etc.

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=673589

--- Comment #26 from Sergio Belkin <sebelk at gmail.com> 2011-02-25 07:48:39 EST ---
(In reply to comment #25)

***The new and corrected files***
Spec URL: http://dl.dropbox.com/u/14217893/UpTools.spec
Spec URL: http://dl.dropbox.com/u/14217893/UpTools-8.5.4-8.fc16.src.rpm
***Below, the comments to Mamoru comments***

> Some notes:
> 
> * Macros
>   https://fedoraproject.org/wiki/Packaging/RPMMacros
>   - Please use macros properly. For example. /usr/bin should be
>     replaced by %{_bindir}
>     ! By the way, "/usr/bin/" part before iconv is just redundant.

Thanks Mamoru!
Firstly:

You're right. Just I thought that the complete path it was better. Fixed (i.e.
removed  "/usr/bin/")

> 
> * Compilation flags
>   - Would you explain why "-Wno-deprecated" is needed (i.e.
>     why do you want to suppress warnings?)

Because we were using hash_map and hash_set, now it is using through %configure
option unordered_map and unordered_set headers instead, AFAIK hash_* are
deprecated since gcc 4.3 but in tarball we preferred to be conservative.

> 
> * Parallel make
>     https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make
>   - Support parallel make if possible. If impossible, please write
>     some comments on the spec file about it.

Oops. Fixed.

> 
> * Timestamps
>   https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
>   - When installing files with "install" or "cp" commands, please
>     add "-p" option to keep timestamps on installed files:

Fixed too.

> 
> * %defattr
>   - Now we prefer to use %defattr(-,root,root,-)

Oops. Fixed!

> 
> * Dependency
>   - UpSsl.h under %_includedir/UpTools contains:
> -----------------------------------------------------------------
>     50  #ifndef USE_YASSL
>     51          #include <openssl/ssl.h>
>     52          #include <openssl/x509.h>
>     53          #include <openssl/rsa.h>
>     54          #include <openssl/engine.h>
> -----------------------------------------------------------------
>     looks like "Requires: openssl-devel%{?_isa}" is also needed
>     on -devel subpackage
>     ! By the way, I prefer to write one Requires per one line
>       because
>       - It is easier to read
>       - It makes diff output smaller and more readable when Requires
>         items changed.

About of "Requires per one line" I agree completely with you, in fact, before
it was one per line, but I thought that it was needed making so, and about
openssl-devel as Requires, I was taking into account that, so yum would resolve
the missing package:

rpm -qR mysql-devel | grep openssl
openssl-devel(x86-32)  

and

repoquery -R mysql-devel
libmysqlclient.so.16
libmysqlclient_r.so.16
mysql(x86-32) = 5.1.55-1.fc14
openssl-devel(x86-32)

Anyway, I've added it :)

> 
> * Messages on scriptlets
>   - Generally showing messages (especially non-error messages)
>     when executing scriptlets (%post and so on) is forbidden.
>     Instruction for creating example executables or so should be
>     written and be installed as a file, and should not appear
>     during scriptlets are executed.

Yup. Removed :)

> 
> * Miscs
>   - Why do you want to list each example files under doc/tests
>     explicitly? (i.e. please use glob)

Just the will to work in excess :)
Using glob now!

> 
>   - Also are there any reason you don't want to use %name-devel directory
>     under %_defaultdocdir?

Thanks! It was an error. Fixed....

> Allowing to use such directory and using
>     %doc in the spec file is much simpler and easier to read.

Yes is it. I've rearranged that properly. 

Thanks Mamoru, your advices helped me a lot.
Please could you give me your review and sponsorship.

Thanks in advance!

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.