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