[Bug 489751] Review Request: btanks - Funny battle game with tanks

[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=489751


Alexey Torkhov <atorkhov at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |needinfo?(lemenkov at gmail.co
                   |                            |m)




--- Comment #3 from Alexey Torkhov <atorkhov at gmail.com>  2009-03-17 07:12:04 EDT ---
Spec URL: http://atorkhov.fedorapeople.org/btanks.spec
SRPM URL: http://atorkhov.fedorapeople.org/btanks-0.8.7686-2.fc10.src.rpm

* Tue Mar 17 2009 Alexey Torkhov <atorkhov at gmail.com> - 0.8.7686-3
- Add patch backups
- Simplify scripts a bit
- Move libs to -libs subpackage to ensure better work in multilib environment
- Cleanier remove-smpeg patch

(In reply to comment #2)
> > %patch0 -p1
> 
> Add postfix. E.g. you should invoke %patch in the following way:
> 
> %patch0 -p1 -b .some_descriptive_postfix

I'm wondering why those backups are needed? I never found it useful.
Anyway, fixed.

> > dos2unix -k *.txt ChangeLog *.url LICENSE EXCEPTION
> 
> Consider using sed -i 's|\r||g' instead (just to avoid dos2unix as a BR).

No, I don't want to do it. Dos2unix is tiny and useful. Otherwise, I had to do
touch to save timestamps.

> > iconv -f latin1 -t utf-8 EXCEPTION > EXCEPTION.new
> touch -r EXCEPTION EXCEPTION.new
> mv -f EXCEPTION.new EXCEPTION
> 
> This piece of spec (and similar ones) is not fail-safe. You should change it
> to something like this:
> 
> iconv -f latin1 -t utf-8 EXCEPTION > EXCEPTION.new && touch -r EXCEPTION
> EXCEPTION.new && mv -f EXCEPTION.new EXCEPTION

Those pieces are equivalent, as script are executed under -e shell option and
it will exit after first command that fails.

> Also, you should use some bash syntax sugar:
> 
> "mv -f EXCEPTION.new EXCEPTION" == "mv -f EXCEPTION{.new,}"
> "touch -r EXCEPTION EXCEPTION.new" == "touch -r EXCEPTION{,.new}"

Fixed.

> > %files
> ...
> %{_libdir}/*.so
> 
> This is not a blocker, actually, but I believe, that dlopened objects should
> be in a proper subdir in %_libdir (for example %{_libdir}/%{name} ). However,
> there are many exceptions (unixODBC, tcl/tk among them), who stores their
> *so-libraries in %_libdir.  

Actually, that is how it is made now - btanks and bted are linked against libs
under %{_libdir} (check ldd), but dlopen'ed libbt_objects plugin is in
%{_libdir}/%{name}.

-- 
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.