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@xxxxxxxxx> 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review