Bug 647510 - Review Request: premake - A cross-platform build configuration tool
Summary: Review Request: premake - A cross-platform build configuration tool
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Dan Horák
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-10-28 15:38 UTC by Joachim de Groot
Modified: 2014-08-06 19:43 UTC (History)
5 users (show)

Fixed In Version: premake-4.3-2.fc14
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-12-09 21:57:31 UTC
dan: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Joachim de Groot 2010-10-28 15:38:07 UTC
Spec URL: http://fedora.jaydg.nl/premake.spec
SRPM URL: http://fedora.jaydg.nl/premake-4.2.1-1.fc13.src.rpm

Description: 
Premake is a build configuration tool that can generate project files for:
 - GNU make
 - Code::Blocks
 - CodeLite
 - MonoDevelop
 - SharpDevelop
 - Apple XCode 
 - Microsoft Visual Studio

Comment 1 Mohamed El Morabity 2010-10-28 17:33:07 UTC
I will review your package.

Comment 2 Joachim de Groot 2010-10-28 17:58:27 UTC
Thanks! 

Some notes:

This is my first package and I need a sponsor. 

Upstream bundles the Lua sources, I've added a patch to use the system's Lua libraries instead.

Comment 3 Mohamed El Morabity 2010-10-28 18:37:50 UTC
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

Comment 4 Joachim de Groot 2010-10-28 22:18:25 UTC
Thank you for your feedback!

(In reply to comment #3)
> 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.

Unfortunately, it took me a while to figure out how to get it there. 

I have added all your changes and additionally removed the 'A' from the summary (read about that in the review of another package).

The new files are 
Spec URL: http://fedora.jaydg.nl/premake.spec
SRPM URL: http://fedora.jaydg.nl/premake-4.2.1-2.fc13.src.rpm


> 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
I was looking for an option to install that does not require naming the executable again as target. That's why I came up with mkdir.

Comment 5 Joachim de Groot 2010-10-29 06:56:45 UTC
I've noticed that the debuginfo package was empty and fixed that. 

Spec URL: http://fedora.jaydg.nl/premake.spec
SRPM URL: http://fedora.jaydg.nl/premake-4.2.1-3.fc13.src.rpm

Output of rpmlint:
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

Comment 6 Dan Horák 2010-11-21 18:05:35 UTC
formal review is here, see the notes explaining OK* and BAD statuses below:

BAD	source files match upstream:
    upstream: 1cd4bded80ecbe695ec309352241a00e6ce01c20  premake-4.2.1-src.zip
    srpm:     a8253af9c17f6e90811b896a2e941492cab4e861  premake-4.2.1-src.zip

OK	package meets naming and versioning guidelines.
OK	specfile is properly named, is cleanly written and uses macros consistently.
OK	dist tag is present.
BAD	license field matches the actual license.
OK	license is open source-compatible. License text included in package.
BAD	latest version is being packaged.
BAD	BuildRequires are proper.
OK	compiler flags are appropriate.
BAD	%clean is present.
BAD	package builds in mock (Rawhide/x86_64).
OK	debuginfo package looks complete.
BAD	rpmlint is silent.
OK	final provides and requires look sane.
N/A*	%check is present and all tests pass.
OK	no shared libraries are added to the regular linker search paths.
OK	owns the directories it creates.
OK	doesn't own any directories it shouldn't.
OK	no duplicates in %files.
OK*	file permissions are appropriate.
OK	no scriptlets present.
OK	code, not content.
OK	documentation is small, so no -docs subpackage is necessary.
OK	%docs are not necessary for the proper functioning of the package.
OK	no headers.
OK	no pkgconfig files.
OK	no libtool .la droppings.
OK	not a GUI app.

- the freshly downloaded source archive doesn't contain the MacOS/X prebuild binary
- the resulting license should be GPL+, see point 4 in https://fedoraproject.org/wiki/Licensing/FAQ#How_do_I_figure_out_what_version_of_the_GPL.2FLGPL_my_package_is_under.3F
- version 4.3 was released few days ago (includes a switch from GPL to BSD)
- readline-devel must be added as BR
- you shouldn't clean %{_builddir}/%{buildsubdir} - http://fedoraproject.org/wiki/PackagingGuidelines#.25clean
- rpmlint complains a bit:
premake.x86_64: W: no-version-in-last-changelog
    => the format of changelog is bad (must include version-release pair, also add an empty line between entries) - http://fedoraproject.org/wiki/PackagingGuidelines#Changelogs
premake.x86_64: W: no-manual-page-for-binary premake4
    => nice to have, but not a blocker
- a test suite is included in the sources, please check whether it can be run during the build
- I'd use an explicit "-m 755" when installing the binary, you should also remove the "%{_builddir}/%{buildsubdir}" part and use "." instead

Comment 7 Dan Horák 2010-11-21 21:05:17 UTC
It may be also useful to install %{_bindir}/premake as a symlink to the premake4 binary.

Comment 8 Joachim de Groot 2010-11-27 15:51:50 UTC
Thank you for the review! I have updated the spec file:

Spec URL: http://fedora.jaydg.nl/premake.spec
SRPM URL: http://fedora.jaydg.nl/premake-4.3-1.fc14.src.rpm

Changes:
  - packed latest version
  - changes license to BSD
  - added readline-devel to BuildRequires
  - added a man page
  - do not clean %{_builddir}/%{buildsubdir}
  - added version-release pair to changelog

rpmlint is silent now: 
  2 packages and 1 specfiles checked; 0 errors, 0 warnings.

The provided test suite could be run during the build, but unfortunately one test fails on x86_64, so I chose not to add it. I'll report the bug upstream and run the tests when it has been fixed:

  os.findlib_FindSystemLib: /home/joachim/tmp/premake-4.3/tests/base/test_os.lua:18: expected true but was false
  600 tests passed, 1 failed

I'm not sure about the symlink, as upstream calls the current version premake4 throughout the documentation and chosen this name as the scripts for version 4 are incompatible to scripts for the previous versions.

Comment 9 Dan Horák 2010-11-28 10:50:13 UTC
(In reply to comment #8)
> Changes:
>   - packed latest version
>   - changes license to BSD
>   - added readline-devel to BuildRequires
>   - added a man page
>   - do not clean %{_builddir}/%{buildsubdir}
>   - added version-release pair to changelog

all issues mentioned in the review are fixed, but a new one appeared
- you shouldn't compress the man page during install, it's done automagically by rpm, also use wild-carded form "premake4.1*" in %files as the algorithm can change

For the records - sha1sum of the source archive
8f37a3599121580f18b578811162b9b49a2e122f  premake-4.3-src.zip

Also thanks for preparing the man page, did you sent it to upstream for inclusion in the new release of premake?
 
> The provided test suite could be run during the build, but unfortunately one
> test fails on x86_64, so I chose not to add it. I'll report the bug upstream
> and run the tests when it has been fixed:
> 
>   os.findlib_FindSystemLib:
> /home/joachim/tmp/premake-4.3/tests/base/test_os.lua:18: expected true but was
> false
>   600 tests passed, 1 failed

thanks for the information, hopefully they will solve the issue and the test-suite could be enabled in the future
 
> I'm not sure about the symlink, as upstream calls the current version premake4
> throughout the documentation and chosen this name as the scripts for version 4
> are incompatible to scripts for the previous versions.

yes, you are right with incompatibilities, let's keep only the binary

Comment 10 Joachim de Groot 2010-11-28 11:46:33 UTC
Thanks for your review.

(In reply to comment #9)
> all issues mentioned in the review are fixed, but a new one appeared
> - you shouldn't compress the man page during install, it's done automagically
> by rpm, also use wild-carded form "premake4.1*" in %files as the algorithm can
> change
I've uploaded a version that fixes the man page compression issue:

Spec URL: http://fedora.jaydg.nl/premake.spec
SRPM URL: http://fedora.jaydg.nl/premake-4.3-2.fc14.src.rpm


> Also thanks for preparing the man page, did you sent it to upstream for
> inclusion in the new release of premake?

Did so just now: http://sourceforge.net/tracker/?func=detail&aid=3121233&group_id=71616&atid=531880


> thanks for the information, hopefully they will solve the issue and the
> test-suite could be enabled in the future
I've reported that bug: http://sourceforge.net/tracker/?func=detail&aid=3121217&group_id=71616&atid=531878


I just realized that you are the codeblocks maintainer - the ability to create Codeblocks projects is the reason I got to use premake in the first place.

Comment 11 Dan Horák 2010-11-28 13:05:20 UTC
All issues are fixed and this package is APPROVED.

I will now sponsor you for the packagers group and please continue with next steps on the "maintainer join" wiki page. If you will have any questions then don't hesitate to ask either me directly or on the devel@lists.fp.o mailing list or #fedora-devel IRC channel.

Comment 12 Joachim de Groot 2010-11-28 14:55:17 UTC
New Package SCM Request
=======================
Package Name: premake
Short Description: Cross-platform build configuration tool
Owners: jaydg
Branches: f13 f14

Comment 13 Jason Tibbitts 2010-11-29 16:58:34 UTC
Git done (by process-git-requests).

Comment 14 Fedora Update System 2010-11-29 17:47:12 UTC
premake-4.3-2.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/premake-4.3-2.fc13

Comment 15 Fedora Update System 2010-11-29 17:48:39 UTC
premake-4.3-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/premake-4.3-2.fc14

Comment 16 Fedora Update System 2010-11-30 22:27:56 UTC
premake-4.3-2.fc14 has been pushed to the Fedora 14 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update premake'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/premake-4.3-2.fc14

Comment 17 Fedora Update System 2010-12-09 21:57:26 UTC
premake-4.3-2.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 18 Fedora Update System 2010-12-09 22:03:47 UTC
premake-4.3-2.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 19 Björn 'besser82' Esser 2014-08-06 18:54:03 UTC
Package Change Request
======================
Package Name: premake
New Branches: el5 el6 epel7
Owners: besser82 jaydg

Comment 20 Gwyn Ciesla 2014-08-06 19:43:50 UTC
Git done (by process-git-requests).


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