Bug 429450 - Review Request: gstream - Simplified stream output/input for Allegro
Review Request: gstream - Simplified stream output/input for Allegro
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-20 08:04 EST by Hans de Goede
Modified: 2008-01-25 06:09 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-25 06:09:43 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
wolfy: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2008-01-20 08:04:23 EST
Spec URL: http://people.atrpms.net/~hdegoede/gstream.spec
SRPM URL: http://people.atrpms.net/~hdegoede/gstream-1.6-1.fc9.src.rpm
Description:
gstream is a C++ add-on library for Allegro. Its main purpose is to provide a
simplified syntax for Allegro's keyboard and text functions for input and
output, so that you can treat a graphical mode as a console.
Comment 1 manuel wolfshant 2008-01-20 20:55:49 EST
I've got two issues here
a) mock build fails with:

Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.26053
+ umask 022
+ cd /builddir/build/BUILD
+ cd gstream16
+ LANG=C
+ export LANG
+ unset DISPLAY
+ make -j3 -f Makefile.unx MAKEDOC=/usr/bin/allegro-makedoc 'OFLAGS=-O2 -g -pipe
-Wall -Wp,-D_FORTIFY_SOURCE=2 -fexception
s -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -fPIC'
Missing the makedoc utility from Allegro! Please copy the makedoc executable
from allegro/docs to this directory.
+ rm test.o
rm: cannot remove `test.o': No such file or directory
error: Bad exit status from /var/tmp/rpm-tmp.26053 (%build)

This happens despite having allegro-devel installed.

b) I've noticed that you pass some custom flags to gcc when building
libgstrm.so.0. Due to the failed build, I did not reach this point so I could
not see what flags are actually used, so I figured I'd better ask, just in case:
shouldn't RPM_OPT_FLAGS be used here, too ? 
Comment 2 Hans de Goede 2008-01-21 05:37:17 EST
(In reply to comment #1)

Thanks for taking a look at this!

> I've got two issues here
> a) mock build fails with:
> 
> Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.26053
> + umask 022
> + cd /builddir/build/BUILD
> + cd gstream16
> + LANG=C
> + export LANG
> + unset DISPLAY
> + make -j3 -f Makefile.unx MAKEDOC=/usr/bin/allegro-makedoc 'OFLAGS=-O2 -g -pipe
> -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexception
> s -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -fPIC'
> Missing the makedoc utility from Allegro! Please copy the makedoc executable
> from allegro/docs to this directory.
> + rm test.o
> rm: cannot remove `test.o': No such file or directory
> error: Bad exit status from /var/tmp/rpm-tmp.26053 (%build)
> 
> This happens despite having allegro-devel installed.
> 

Erm, oops, my bad, allegro-devel was "missing" makedoc (not install by make
install, but appearantly needed by some apps). Since I'm also the allegro
maintainer I fixed this, but I never committed let alone build my changes (other
then a local build).

I'm currently working on fixing allegro <-> pulseaudio interaction, when I'm
done with that I'll push a new allegro to devel, F-8 and F-7 which does include
makedoc (which I've name allegro-makedoc as makedoc is a bit generic).

I'll add a note here once the new allegro is done.

> b) I've noticed that you pass some custom flags to gcc when building
> libgstrm.so.0. Due to the failed build, I did not reach this point so I could
> not see what flags are actually used, so I figured I'd better ask, just in case:
> shouldn't RPM_OPT_FLAGS be used here, too ? 

Yes, you're right I'll fix this after a full review is done, as I would like to
keep the number of iterations (== work for both of us) to a minimum.
Comment 3 manuel wolfshant 2008-01-21 05:59:52 EST
>I'm currently working on fixing allegro <-> pulseaudio interaction, 
>when I'm done with that I'll push a new allegro [...]
> which does include makedoc (which I've name allegro-makedoc as makedoc is a
bit generic).
>I'll add a note here once the new allegro is done.

Excellent approach. I'll be here.


>Yes, you're right I'll fix this after a full review is done, as I would like
> to keep the number of iterations (== work for both of us) to a minimum.

Which is exactly why I've asked now and not after solving the makedoc issue :)
Comment 5 manuel wolfshant 2008-01-22 09:05:51 EST
Something is fishy here:
Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.91582
+ umask 022
+ cd /builddir/build/BUILD
+ cd gstream16
+ LANG=C
+ export LANG
+ unset DISPLAY
+ make -j3 -f Makefile.unx MAKEDOC=/usr/bin/allegro-makedoc 'OFLAGS=-O2 -g -pipe
-Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-
protector --param=ssp-buffer-size=4 -m64 -mtune=generic -fPIC'
c++    -Wall -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
 -fPIC -c gsintfac.cc
c++    -Wall -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
 -fPIC -c gsvirtul.cc
c++    -Wall -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
 -fPIC -c gsmanip.cc
c++    -Wall -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
 -fPIC -c gswrirea.cc
c++    -Wall -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
 -fPIC -c gsfunc.cc
c++    -Wall -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
 -fPIC -c test.cc
/usr/bin/allegro-makedoc gstream._tx
makeinfo --no-split gstream.txi
make: makeinfo: Command not found
make: *** [gstream.info] Error 127
make: *** Waiting for unfinished jobs....
test.cc: In function 'int main()':
test.cc:36: warning: 'int text_mode(int)' is deprecated (declared at
/usr/include/allegro/alcompat.h:155)
test.cc:36: warning: 'int text_mode(int)' is deprecated (declared at
/usr/include/allegro/alcompat.h:155)
gswrirea.cc: In member function 'void gbuf::output_chars(const char*)':
gswrirea.cc:33: warning: 'void textout(BITMAP*, const FONT*, const char*, int,
int, int)' is deprecated (declared at /usr/include/al
legro/alcompat.h:157)
gswrirea.cc:33: warning: 'void textout(BITMAP*, const FONT*, const char*, int,
int, int)' is deprecated (declared at /usr/include/al
legro/alcompat.h:157)
gswrirea.cc: In member function 'void Schar::draw()':
gswrirea.cc:257: warning: 'void textout(BITMAP*, const FONT*, const char*, int,
int, int)' is deprecated (declared at /usr/include/a
llegro/alcompat.h:157)
gswrirea.cc:257: warning: 'void textout(BITMAP*, const FONT*, const char*, int,
int, int)' is deprecated (declared at /usr/include/a
llegro/alcompat.h:157)
error: Bad exit status from /var/tmp/rpm-tmp.91582 (%build)

I could not locate where should makeinfo come from.
Comment 6 Hans de Goede 2008-01-22 09:48:25 EST
makeinfo is part of texinfo, so I'm missing a BuildRequires: texinfo, sorry
about that.
Comment 7 manuel wolfshant 2008-01-22 14:42:56 EST
Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on: devel/x86_64
 [x] Rpmlint output:
source RPM: none
binary RPM: none
 [x] Package is not relocatable.
 [x] Buildroot is correct
(%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Package is licensed with an open-source compatible license and meets other
legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type:Giftware
 [x] 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 is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package matches the upstream source, as provided
in the spec URL.
  SHA1SUM of package: 6d5548495da8d693ab5f39c648421541b9059279 gstrm16.zip
 [x] Package is not known to require ExcludeArch
 [!] All build dependencies are listed in BuildRequires, except for any that are
listed in the exceptions section of Packaging Guidelines.
See note 1
 [x] The spec file handles locales properly.
 [x] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
 [!] Package consistently uses macros.
See Note 2
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [x] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [x] Development .so files in -devel subpackage, if present.
 [x] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
 [x] Package does not own files or directories owned by other packages.
=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: F7 and devel / x86_64
 [x] Package should compile and build into binary rpms on all supported
architectures.
     Tested on: F7 and devel / x86_64
 [x] Package functions as described.
Allows building of maze, bz #429451
 [x] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files is correct.
 [x] File based requires are sane.


=== Issues ===
1. Missing BR: texinfo
2. Maybe
  make %{?_smp_mflags} -f Makefile.unx MAKEDOC=/usr/bin/allegro-makedoc \
should be replaced with
  make %{?_smp_mflags} -f Makefile.unx MAKEDOC=%{_bin}/allegro-makedoc \
Not a big deal, but for consistency sake
3. Fedora compiler flags are not used in the sequence
# makefile makes a .a file, make a .so ourselves
gcc -shared -o libgstrm.so.0 -Wl,-soname,libgstrm.so.0 *.o

=== Final Notes ===
1. MUSTFIX: Missing BR for texinfo
2. MUSTFIX: proper compiler flags must be used in the command listed as issue no. 3


================
*** APPROVED *** with the condition of fixing at least the 2 problems mentioned
under Final Notes (+ issue 2 from Issues if possible)
================

Comment 8 Hans de Goede 2008-01-22 15:55:39 EST
Thanks for the review! I'll fix all 3 mentioned issues before import.

New Package CVS Request
=======================
Package Name:      gstream
Short Description: Simplified stream output/input for Allegro
Owners:            jwrdegoede
Branches:          F-7 F-8 devel
InitialCC:         <empty>
Cvsextras Commits: Yes

Comment 9 Kevin Fenzi 2008-01-22 18:44:27 EST
cvs done.
Comment 10 Fedora Update System 2008-01-24 16:46:01 EST
allegro-4.2.2-7.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 11 Fedora Update System 2008-01-24 16:53:12 EST
allegro-4.2.2-7.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 12 Hans de Goede 2008-01-25 06:09:43 EST
Imported and build for F-7 F-8 and devel, updates for F-7 and F-8 are on their
way, closing.

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