[Bug 647510] Review Request: premake - A cross-platform build configuration tool

[bugzilla at redhat.com]

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 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW
         AssignedTo|pikachu.2014 at gmail.com      |nobody at fedoraproject.org
               Flag|fedora-review?              |

--- Comment #3 from Mohamed El Morabity <pikachu.2014 at gmail.com> 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.