Bug 417711

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

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?

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

Created attachment 289822 [details]
Fix spurious permissions.

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.

ok, but does the devel package need to require the main package?

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.

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 (
http://cvs.fedoraproject.org/viewcvs/rpms/readline/devel/readline.spec?rev=1.36&view=auto
) 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.

For static archive, please follow the section
"Packaging Static Libraries" of
http://fedoraproject.org/wiki/Packaging/Guidelines

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

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 
flam3-2.7.7-2.fc9
[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
------------------------------------------------------
%files
%{_datadir}/flam3/
------------------------------------------------------
    this includes the directory %_datadir/flam3 and all 
    files/directories/etc under %_datadir/flam3, while
-------------------------------------------------------
%files
%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@
Cflags: -I@INCLUDEDIR@ @WIN32_CFLAGS@
-------------------------------------------------------

  - 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
       "libxml2-devel"
     * 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.

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.

(In reply to comment #12)
> Are the lines that you took
> from it and pasted here needing to be fixed?  
Absolutely.
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.

ping?

pong.

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

(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

Well,
* 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?

> * Thu Jan 17 2008 Mamorut Tasaka <mtasaka.u-tokyo.ac.jp> 2.7.7-3
thanks.

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

Oh, no.. Sorry.

http://mtasaka.fedorapeople.org/Review_request/flam3/flam3.spec
http://mtasaka.fedorapeople.org/Review_request/flam3/flam3-2.7.7-3.fc8.src.rpm

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?

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 :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

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:
http://fedoraproject.org/PackageReviewStatus/NEW.html
(NOTE: please don't choose "Merge Review")


Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

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

ping?

if i do a package that requires flam3 and submit that for review, is there
anything i need to do special?

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

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.

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.

Rebuild failed on both ppc and ppc64...

http://koji.fedoraproject.org/koji/taskinfo?taskID=378037

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?

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.

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.

To be more explicit, I removed the definition of HAVE_GCC_ATOMIC_OPS from the
config.h file generated by configure.

Jason: could you submit a patch?

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.

Also I reported Jason's information upstream.

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

- Well, I didn't notice this before but %_datadir/flam3 is not owned
  by any pacakge, which must be fixed.

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

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.

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

You should try not to include config.h
You can see more explanation by Hans:
https://bugzilla.redhat.com/show_bug.cgi?id=208034#c43

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.

Would you explain (or attach the log, your trial patch) how your
patch failed? IMO private.h doesn't seem to need config.h.

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)
 #endif
 
-#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.

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.

* 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?

Okay.

- 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:
http://fedoraproject.org/wiki/PackageMaintainers/Join
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
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

I meant:

%patch2 -p1 -b .remove_config_h

i've registered with the FAS. my username is ianweller.

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

Now I should be sponsoring you. Please follow "Join" wiki again.

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

Mamoru: is it also ok if i have wiki editing privileges?

(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 )

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.

* 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
SRPM URL:
http://ianweller.fedorapeople.org/SRPMS/flam3/2.7.8-4/flam3-2.7.8-4.fc8.src.rpm

You should provide your wikiname so that folks will know what to add to the
EditGroup.

wikiname is IanWeller.

cvs done.

flam3-2.7.8-4.fc7 has been submitted as an update for Fedora 7

flam3-2.7.8-4.fc8 has been submitted as an update for Fedora 8

koji build OK
added to bodhi

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.

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.

Package Change Request
======================
Package Name: flam3
New Branches: EL-4 EL-5

cvs done.