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=647510 Mohamed El Morabity <pikachu.2014@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEW AssignedTo|pikachu.2014@xxxxxxxxx |nobody@xxxxxxxxxxxxxxxxx Flag|fedora-review? | --- Comment #3 from Mohamed El Morabity <pikachu.2014@xxxxxxxxx> 2010-10-28 14:37:50 EDT --- I've just seen you added the  FE-NEEDSPONSOR  flag after I took your package. Since I'm not a sponsor, I won't be able to approve your package if necessary. Anyway I'll give you some comments. 1) The Sourceforge source URLs must follow the following rule: http://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net So, for premake, yout Source0 entry should look like: Source0: http://downloads.sourceforge.net/%{name}/%{name}-%{version}-src.zip 2) Your patch to use system lua instead of embedded looks really good. But you should add a comment about it in your .spec file which explains why it's here: http://fedoraproject.org/wiki/PackagingGuidelines#All_patches_should_have_an_upstream_bug_link_or_comment Maybe the patch name deserves to be more explicit also, e.g.  premake-4.2.1-system-lua.patch  3) Two (sad) issues about the build: * the build logs look quite silent, as you can see here: http://koji.fedoraproject.org/koji/getfile?taskID=2562351&name=build.log Not a good thing, among other things, compilation flags cannot be checked... Fortunately, a variable,  verbose Â, can be set do disable silent compilation: in your %build section: make verbose=true %{?_smp_mflags} * by the way, Fedora compilation flags are not used to compile your program. You must compile premake using the Fedora CFLAGS (defined in the %{optflags} macro). Since build/gmake.unix/Premake4.make, called by the Makefile, defines CFLAGS from required preprocessors flags $(CPPFLAGS), it must be modified. I suggest you using sed in %prep instead of making of a patch for this small change: sed -i "s/^\s*CFLAGS\s*+=.*/CFLAGS += \$(CPPFLAGS) %{optflags}/" build/gmake.unix/Premake4.make (this will update in Premake4.make all CFLAGS definitions to: CFLAGS += $(CPPFLAGS) <content of %{optflags}> 4) In %install, you don't need to use the absolute path to your build directory to install the executable. The following line is enough: %install rm -rf %{buildroot} install -Dp bin/release/premake4 %{buildroot}/%{_bindir}/premake4 Note that: * %{_bindir} macro should be used instead of /usr/bin * the -D option will create the destination folder if necessary, so a call to mkdir before is useless * the -p option will preserve timestamp -- 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review