Bug 417711 - Review Request: flam3 - Programs to generate and render cosmic recursive fractal flames
Summary: Review Request: flam3 - Programs to generate and render cosmic recursive frac...
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
Depends On:
Blocks: 431290
TreeView+ depends on / blocked
Reported: 2007-12-10 06:28 UTC by Ian Weller
Modified: 2008-03-20 02:44 UTC (History)
3 users (show)

Fixed In Version: 2.7.8-4.fc7
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2008-02-07 20:57:25 UTC
Type: ---
mtasaka: fedora-review+
kevin: fedora-cvs+

Attachments (Terms of Use)
Fix spurious permissions. (167 bytes, patch)
2007-12-17 21:42 UTC, Mat Booth
no flags Details | Diff
rpmbuild result when patching out #include "config.h" from private.h (12.12 KB, text/plain)
2008-01-30 22:27 UTC, Ian Weller
no flags Details
patch to remove config.h properly (789 bytes, patch)
2008-01-31 07:26 UTC, Mamoru TASAKA
no flags Details | Diff

Description Ian Weller 2007-12-10 06:28:35 UTC
Spec URL: http://ianweller.org/rpm/flam3.spec
SRPM URL: http://ianweller.org/rpm/flam3-2.7.6-1.fc8.src.rpm
Description: Flam3, or Fractal Flames, are algorithmically generated images and animations. This is free software to render fractal flames as described on http://flam3.com. Flam3-animate makes animations, and flam3-render makes still images. Flam3-genome creates and manipulates genomes (parameter sets). A C library is also installed.

Please note that this is my first package, and I'm looking for a sponsor.

Comment 1 Mat Booth 2007-12-12 22:14:35 UTC
(Unofficial: I can neither approve your package or sponsor you, but there are
some things you need to look at before a real reviewer gets here...)

I'm only able to test building the package for i386, but besides some warnings,
it seems to work ok.

You should probably drop kernel from the requires. I could be wrong but it looks
like it might prevent users of kernel-xen from installing your package. Besides,
it ought to be a reasonable assumption to say all users will have a kernel
installed. ;-)

Also, rpmlint says:

flam3.i386: E: explicit-lib-dependency libjpeg
flam3.i386: E: explicit-lib-dependency libpng
flam3.i386: E: explicit-lib-dependency libxml2

which indicates that rpm will find these dependencies on its own and that you
should not include them in your requires. This being the case, it's probably
also safe to remove zlib from the requires since it's a dependency of libpng.

flam3.i386: W: devel-file-in-non-devel-package /usr/lib/pkgconfig/flam3.pc
flam3.i386: W: devel-file-in-non-devel-package /usr/include/flam3.h
flam3.i386: W: devel-file-in-non-devel-package /usr/lib/libflam3.a

This indicates that you need to move these files into subpackages. Header files
(and usually pkgconfig files) must be *-devel packages and static libs must be
in *-static packages. (Take a look at zlib or similar
http://cvs.fedoraproject.org/viewcvs/rpms/zlib/devel/ if you need an example.)
Packages containing pkgconfig files must also list pkgconfig as a dependency.

flam3.i386: W: no-documentation

There is nothing in your %doc section in the SPEC. The source package includes
the text of the licence in its own file "COPYING" so it must be included in this
section and will resolve the error.

flam3-debuginfo.i386: W: spurious-executable-perm /usr/src/debug/flam3-2.7.6/png.c

This file shouldn't be executable.

After making these changes, don't forget to increment the release number.

This is my first real review. I hope I've been helpful.

Comment 2 Ian Weller 2007-12-16 01:10:14 UTC
This has been very helpful as far as I can see. :)

I'll get right on all of this once I get my power back. The midwest (where I happen to live) had a nasty ice 
storm, and we haven't had power since Tuesday. Hopefully I'll have another release done soon.

I finished the package and posted this bug just before I saw most of these points on the wiki.... laziness 
and sleepiness sometimes gets the best of me.

I'm assuming this all has to be done with patches, because it needs the original source code, right?

Comment 3 Ian Weller 2007-12-16 21:48:25 UTC
I have produced version 2.7.6 release 2.
Spec URL: http://ianweller.org/rpm/flam3.spec
SRPM URL: http://ianweller.org/rpm/flam3-2.7.6-2.fc8.src.rpm

All of the problems that were stated in comment #1 have been fixed *except* the
spurious-executable-perm problem. I searched all over the internet looking for a
fix for that, and the only fixes were to ignore it. If I absolutely must fix it,
somebody please let me know how. ;)

I did run rpmlint and the only problem it came back with was the ignored
spurious-executable-perm problem.

I'm not sure if the subpackages need to have requires linking to the original
flam3 package. I ran some rpm queries and it doesn't do that automatically...
however I don't think I need flam3 to have flam3-devel in this case. Let me know
if you think the opposite. :)

Comment 4 Mat Booth 2007-12-17 21:42:50 UTC
Created attachment 289822 [details]
Fix spurious permissions.

Comment 5 Mat Booth 2007-12-17 21:47:12 UTC
C source files probably shouldn't ever be executable, so it ought really to be
fixed. See above (I was too trigger happy with the attachment) for a patch to
your specfile that shows you how, it's easy enough. :-)

The devel package probably ought to require the static lib package too, since
you wouldn't be able to use the header file alone.

Comment 6 Ian Weller 2007-12-17 22:13:42 UTC
ok, but does the devel package need to require the main package?

Comment 7 Ian Weller 2007-12-17 23:53:17 UTC
release 2.7.7-1.
Spec URL: http://ianweller.org/rpm/flam3.spec
SRPM URL: http://ianweller.org/rpm/flam3-2.7.7-1.fc8.src.rpm

i made the devel and static subpackages require the main package. also i fixed
everything per comments #4 and #5.

Comment 8 Mat Booth 2007-12-18 00:29:58 UTC
Yes, you are right, it should require the main package.

Rereading the review guidelines also tells me that you should make dependencies
between your sub-packages fully versioned too, which makes sense:

Requires: %{name} = %{version}-%{release}


After having a look around at some other packages, like readline (
) I see they also have the static package require the devel package:

Requires: %{name}-devel = %{version}-%{release}

But I reckon that would create a cyclic dependency here.

The reason readline-devel doesn't require readline-static is presumably because
it includes a shared library which makes the header files not useless. I still
don't think there's much point in installing a header file alone in this case
though, but on the other hand, I guess there's no real point in installing just
the static lib without the header so I'm a bit hesitant about what to suggest.
Personally, I'd whack the header in with the static library (or vice-verse) but
that violates a "must" rule.

I can't seem to find an existing package that has a static lib without also
having a shared lib, so maybe the solution would be to build a shared library
too and copy readline's arrangement.

However I really don't want to be advising you wrongly. Perhaps input from a
more experienced reviewer would be appropriate.

Comment 9 Mamoru TASAKA 2007-12-19 14:23:11 UTC
For static archive, please follow the section
"Packaging Static Libraries" of

Comment 10 Ian Weller 2007-12-20 02:21:01 UTC
ok, all of that is fixed. release 2.7.7-2.
Spec URL: http://ianweller.org/rpm/flam3.spec
SRPM URL: http://ianweller.org/rpm/flam3-2.7.7-2.fc8.src.rpm

Comment 11 Mamoru TASAKA 2007-12-21 15:27:06 UTC
For 2.7.7-2:

* Redundant Requires
  - "Requires: glibc" is really unneeded.

* flam3.src:25: W: unversioned-explicit-provides flam3-static
  (from rpmlint)
  - Please change to
Provides:       flam3-static = %{version}-%{release}

* SourceURL
  - I recommend to use %version macro because with this
    you probably won't have to change the SourceURL when new
    version is released.

* Timestamps
  - To keep timestamps on installed files, please use
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
    While sometimes this does not work, this method usually
    works for recent Makefiles.

* Directory ownership issue
[tasaka1@localhost ~]$ rpm -qf /usr/share/flam3/flam3-palettes.xml 
[tasaka1@localhost ~]$ rpm -qf /usr/share/flam3                    
file /usr/share/flam3 is not owned by any package
  - This means that
    * installing flam3 rpm creates %_datadir/flam3 to install
      xml file, however the created %_datadir/flam3 directory is
      not owned by any package.

  ! Note
    When you write
    this includes the directory %_datadir/flam3 and all 
    files/directories/etc under %_datadir/flam3, while
%dir %{_datadir}/flam3/
    includes the directory %{_datadir}/flam3 only.

* Some issues in -devel subpackage
  - Please check %{_libdir}/pkgconfig/flam3.pc. The content
    must be fixed.
Libs: -L${libdir} -lflam3 @WIN32_LIBS@

  - The following line in flam3.pc
Requires: libpng12 >= 1.0
    means that flam3-devel should have "Requires: libpng-devel"

   - %_includedir/flam3.h contains:
    24  #include <stdio.h>
    25  #include <libxml/parser.h>
    26  #include "isaac.h"
     * The line 25 means that flam3-devel should have
     * And the line 26 is strange because isaac.h is not installed.

* Duplicate documents
  - You don't have to install documents in -devel subpackage
    which are already added to main package.

Comment 12 Ian Weller 2007-12-29 06:55:08 UTC
Mamoru, I'm not exactly sure what you mean by the content in flam3.pc needing to
be fixed.  As far as I can see, I see absolutely nothing that could be wrong (or
right) with the pkgconfig file... what should it say?  Are the lines that you took
from it and pasted here needing to be fixed?  Or are you just referring to the
content under it in your comment?

Also, I decided that I would install isaac.h (and isaacs.h which is required by
it) in the same directory where flam3.h would go.  Will this cause problems?  The
command I'm using to install it is
  install -p -m 644 isaac.h $RPM_BUILD_ROOT/usr/include/isaac.h
and is under the make install command.

Comment 13 Mamoru TASAKA 2007-12-29 10:57:20 UTC
(In reply to comment #12)
> Are the lines that you took
> from it and pasted here needing to be fixed?  
For example,
$ pkg-config --cflags flam3
@WIN32_CFLAGS@ -I@INCLUDEDIR@ -I/usr/include/libpng12  
This result is wrong.

> Also, I decided that I would install isaac.h (and isaacs.h which is required by
> it) in the same directory where flam3.h would go.  Will this cause problems?  The
> command I'm using to install it is
>   install -p -m 644 isaac.h $RPM_BUILD_ROOT/usr/include/isaac.h
> and is under the make install command.

IMO all header files (which should be installed) should be moved to
under %_includedir/%name.

Comment 14 Mamoru TASAKA 2008-01-13 13:52:37 UTC

Comment 15 Ian Weller 2008-01-13 21:51:33 UTC

sorry, i've been distracted with school and whatnot.

however, i'm still not understanding what exactly i need to do to get pkgconfig
right. i've searched for docs everywhere, and google-fu'd to the best of my
ability, and i still don't have anything new. in other words, help would be
appreciated so i can this small problem fixed. :)

Comment 16 Mamoru TASAKA 2008-01-17 14:15:25 UTC
(In reply to comment #15)
> in other words, help would be
> appreciated so i can this small problem fixed. :)

Please don't say "small"...

* Thu Jan 17 2008 Mamorut Tasaka <mtasaka.u-tokyo.ac.jp> 2.7.7-3
- Fix pkgconfig .pc file
- Install missing headers
- Move header files into %%{_includedir}/%%{name}
- Fix cflags to meet Fedora guidelines
- More Requires to -devel subpackage for static archive

* Actually some fixes were needed to fix not only for macros
  expansion problem but also linkage issue
* Also some header files are missing and I forcely installed them with
  changing the installation directory.

Would you check how I changed your srpm?

Comment 17 Ian Weller 2008-01-17 22:47:18 UTC
> * Thu Jan 17 2008 Mamorut Tasaka <mtasaka.u-tokyo.ac.jp> 2.7.7-3

> Would you check how I changed your srpm?
I definitely would if I knew where you put it ;) if you did upload it as an
attachment it's not in the list...

Comment 19 Ian Weller 2008-01-18 04:04:35 UTC
works for me. got the patch and looked at it.

i guess at this point we just have to wait until somebody else finds a problem
or approves it?

Comment 20 Mamoru TASAKA 2008-01-18 13:49:58 UTC
No, usually I can approve this, however as this is NEEDSPONSOR
ticket, more one procedure is needed.

NOTE: Before being sponsored:

This package will be accepted with another few work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on:
(NOTE: please don't choose "Merge Review")

Review guidelines are described mainly on:

As the last flam3 srpm is written by me, I will want your another 
review request and check it if you can.

Comment 21 Mamoru TASAKA 2008-01-27 07:58:13 UTC

Comment 22 Ian Weller 2008-01-27 21:50:33 UTC
if i do a package that requires flam3 and submit that for review, is there
anything i need to do special?

Comment 23 Ian Weller 2008-01-28 03:10:00 UTC
ok never mind on comment #22's question, the package i was thinking of doing i
can't even get to build. i'll leave it to somebody else. however i'm working on
another package now, and while that purrs in mock i'll go help out some other

Comment 24 Ian Weller 2008-01-28 03:45:01 UTC
Spec URL: http://ianweller.org/rpm/flam3.spec
SRPM URL: http://ianweller.org/rpm/flam3-2.7.6-1.fc8.src.rpm
* Sun Jan 27 2008 Ian Weller <ianweller> 2.7.8-1
- Updated to version 2.7.8
- Made sure that libflam3.la wasn't included, complying with review guidelines
  "Packages must NOT contain any .la libtool archives, these should be removed
in the spec."


Also I have completed a second package, bug #430441, xautolock. I went ahead and
added the FE-NEEDSPONSOR blocker on that.

Comment 25 Ian Weller 2008-01-28 03:51:48 UTC
Sorry, I gave the wrong URLs for the spec and SRPM.
Spec URL: http://ianweller.org/rpm/flam3/flam3.spec
SRPM URL: http://ianweller.org/rpm/flam3/flam3-2.7.8-1.fc8.src.rpm
are correct.

Comment 26 Mamoru TASAKA 2008-01-28 14:25:13 UTC
Rebuild failed on both ppc and ppc64...


Comment 27 Ian Weller 2008-01-28 16:45:52 UTC
how exactly am i going to be able to test this when i create a new package? i
don't have a ppc computer (or ppc64) with linux.

will mock do it for me?

Comment 28 Ian Weller 2008-01-29 03:39:53 UTC
i took a hard look at the log files and tried what i thought was possible, and
it wasn't. then i took a look at the code with my general understanding of C,
and couldn't find any way to fix it, and was generally confused about how it
worked on i386 but not ppc.

so i asked the developers. the forum topic is available at
http://community.electricsheep.org/node/173 . i won't be able to provide any
other information until either that forum topic is answered or someone answers
me here.

Comment 29 Jason Tibbitts 2008-01-29 04:35:25 UTC
Well, this code uses GCC atomic bitops; for whatever reason, configure decides
that ppc has them but when it does to compile the file they don't actually work.

I have access to a PPC machine, so I did a configure run and then removed the
definition of HAVE_GCC_ATOMIC_OPS.  The build succeeded.  (Just to check, I put
it back and the build fails.)  So that seems to be the root of the issue.  I'm
not sure what the proper solution is; perhaps a call to sed wrapped in %ifarch
after the call to %configure would be OK.  But at least it's something you can
report upstream.

Comment 30 Jason Tibbitts 2008-01-29 04:44:51 UTC
To be more explicit, I removed the definition of HAVE_GCC_ATOMIC_OPS from the
config.h file generated by configure.

Comment 31 Ian Weller 2008-01-29 05:02:53 UTC
Jason: could you submit a patch?

Comment 32 Ian Weller 2008-01-29 05:26:22 UTC
Sorry, Jason, I was able to figure it out myself. I went ahead and removed you
from the CC list.

Spec URL: http://ianweller.org/rpm/flam3/flam3.spec
SRPM URL: http://ianweller.org/rpm/flam3/flam3-2.7.8-2.fc8.src.rpm

* Mon Jan 28 2008 Ian Weller <ianweller> 2.7.8-2
- Added more missing headers -- they might be used by some program somewhere:
  private.h config.h img.h
- Fix atomic ops error on ppc and ppc64 with Patch1

As far as I know from Jason's data it should work. I also found a line very
similar to it describing something for 64 bit, so I commented that out too with
Patch1. We'll see how that builds.

Comment 33 Ian Weller 2008-01-29 05:31:17 UTC
Also I reported Jason's information upstream.

Comment 34 Mamoru TASAKA 2008-01-29 10:42:10 UTC
- Don't include autotool-generated header files such as "config.h"
  This easily causes name-space conflict.

- And are newly included two header files really needed?

Comment 35 Mamoru TASAKA 2008-01-29 10:52:02 UTC
- Well, I didn't notice this before but %_datadir/flam3 is not owned
  by any pacakge, which must be fixed.

Comment 36 Ian Weller 2008-01-29 23:05:02 UTC
(In reply to comment #34)
> - Don't include autotool-generated header files such as "config.h"
>   This easily causes name-space conflict.
> - And are newly included two header files really needed?

I'm in the process of building a package called qosmic which requires
flam3-devel files. I found that it needs the private.h file, which requires
config.h. I included img.h for good measure, since it might be necessary at some

I fixed comment #35's information, and I'll upload a new build after you let me
know what to do about the header files.

Comment 37 Jason Tibbitts 2008-01-29 23:12:44 UTC
I think that including config.h is pretty much a non-starter.  If something
needs it it's probably badly broken.

There are various other reviews where this has been an issue; the first one I
pulled up is https://bugzilla.redhat.com/show_bug.cgi?id=245917#c4

Comment 38 Mamoru TASAKA 2008-01-30 02:21:26 UTC
You should try not to include config.h
You can see more explanation by Hans:

Comment 39 Ian Weller 2008-01-30 05:08:35 UTC
OK so I understand why config.h is a bad thing. But I'm slightly confused
because I don't know exactly what I need to do to fix this, because qosmic
requires private.h, which requires config.h... >.<

What exactly should I do about this? I tried seeing if private.h would work
without config.h so i patched it out and the build for flam3 failed.

Comment 40 Mamoru TASAKA 2008-01-30 16:51:10 UTC
Would you explain (or attach the log, your trial patch) how your
patch failed? IMO private.h doesn't seem to need config.h.

Comment 41 Ian Weller 2008-01-30 22:27:46 UTC
Created attachment 293505 [details]
rpmbuild result when patching out #include "config.h" from private.h

Here's what happens when I enable "patch2" in my spec file. That patch reads:
--- flam3-2.7.8/private.h.orig	2008-01-29 22:45:08.000000000 -0600
+++ flam3-2.7.8/private.h	2008-01-29 22:45:16.000000000 -0600
@@ -35,7 +35,7 @@
 #define basename(x) strdup(x)
-#include "config.h"
+/* #include "config.h" */
 #define EPS (1e-10)
 #define CMAP_SIZE 256

This patch is applied as a regular patch, before configuration and building, so
I'm not sure if that's causing the problem, and instead should remove it with
sed after making.

Comment 42 Mamoru TASAKA 2008-01-31 07:26:37 UTC
Created attachment 293553 [details]
patch to remove config.h properly

This is because flam.c (and others) don't try to include config.h
but actually they should.
The attatched patch should work for this issue.

Comment 43 Ian Weller 2008-01-31 21:16:57 UTC
* Tue Jan 29 2008 Ian Weller <ianweller> 2.7.8-3
- Removed config.h properly
- We now own datadir/flam3

SRPM: http://ianweller.org/rpm/flam3/flam3-2.7.8-3.fc8.src.rpm
spec: http://ianweller.org/rpm/flam3/flam3.spec


on another note, i've pretty much got qosmic ready to put up as a package, which
is utterly dependent on flam3. should i wait until flam3 is approved and whatnot
before I submit a review request for qosmic or should I do it after I double
check my packaging?

Comment 44 Mamoru TASAKA 2008-02-01 11:52:40 UTC

- This package (flam3) itself is now okay, except for the following
  * The line
%patch2 -p1 -b flam3-genome.c -b flam3.c -b private.h
    is somewhat confusing. Just writing
%patch1 -p1 -b .remove_config_h
    or so is enough.

- Your another review request (bug 430441) needs some fix, however
  I will accept it for initial draft.

    This package (flam3) is APPROVED by me

Please follow the procedure written on:
from "Get a Fedora Account".
At a point a mail should be sent to sponsor members which notifies
that you need a sponsor. At the stage, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 7/8, you also have
to look at
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Comment 45 Mamoru TASAKA 2008-02-01 12:08:32 UTC
I meant:

%patch2 -p1 -b .remove_config_h

Comment 46 Ian Weller 2008-02-02 00:45:24 UTC
i've registered with the FAS. my username is ianweller.

thanks! i'll also fix that line when i upload the package.

Comment 47 Mamoru TASAKA 2008-02-02 02:08:05 UTC
Now I should be sponsoring you. Please follow "Join" wiki again.

Comment 48 Ian Weller 2008-02-02 02:38:58 UTC
New Package CVS Request
Package Name: flam3
Short Description: Programs to generate and render cosmic recursive fractal flames
Owners: ianweller
Branches: F-7 F-8
InitialCC: ianweller
Cvsextras Commits: yes

Comment 49 Ian Weller 2008-02-02 02:46:10 UTC
Mamoru: is it also ok if i have wiki editing privileges?

Comment 50 Mamoru TASAKA 2008-02-02 03:13:19 UTC
(In reply to comment #49)
> Mamoru: is it also ok if i have wiki editing privileges?

It should be okay (after you do some procedure:
http://fedoraproject.org/wiki/WikiEditing )

Comment 51 Ian Weller 2008-02-02 05:01:15 UTC
I've done that. I'm at the final step: "Contact Someone from the EditGroup --
Anyone listed on the EditGroup page may add you to the EditGroup. This is the
group that has write access to most parts of the wiki. If you have already been
talking to a contributor, ask them if they can add you." You are in the EditGroup.

Comment 52 Ian Weller 2008-02-02 05:28:25 UTC
* Fri Feb 01 2008 Ian Weller <ianweller> 2.7.8-4
- Made patch commands less confusing

Spec URL: http://ianweller.fedorapeople.org/SRPMS/flam3/2.7.8-4/flam3.spec

Comment 53 Jason Tibbitts 2008-02-02 15:05:45 UTC
You should provide your wikiname so that folks will know what to add to the

Comment 54 Ian Weller 2008-02-02 16:33:45 UTC
wikiname is IanWeller.

Comment 55 Kevin Fenzi 2008-02-02 18:47:32 UTC
cvs done.

Comment 56 Fedora Update System 2008-02-02 20:07:40 UTC
flam3-2.7.8-4.fc7 has been submitted as an update for Fedora 7

Comment 57 Fedora Update System 2008-02-02 20:10:08 UTC
flam3-2.7.8-4.fc8 has been submitted as an update for Fedora 8

Comment 58 Ian Weller 2008-02-02 20:13:30 UTC
koji build OK
added to bodhi

Comment 59 Fedora Update System 2008-02-07 20:57:23 UTC
flam3-2.7.8-4.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 60 Fedora Update System 2008-02-07 20:57:29 UTC
flam3-2.7.8-4.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 61 Ian Weller 2008-03-19 22:43:35 UTC
Package Change Request
Package Name: flam3
New Branches: EL-4 EL-5

Comment 62 Kevin Fenzi 2008-03-20 02:44:47 UTC
cvs done.

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