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
I will review your package.
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.
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
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.
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.
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
It may be also useful to install %{_bindir}/premake as a symlink to the premake4 binary.
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.
(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
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.
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.o mailing list or #fedora-devel IRC channel.
New Package SCM Request ======================= Package Name: premake Short Description: Cross-platform build configuration tool Owners: jaydg Branches: f13 f14
Git done (by process-git-requests).
premake-4.3-2.fc13 has been submitted as an update for Fedora 13. https://admin.fedoraproject.org/updates/premake-4.3-2.fc13
premake-4.3-2.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/premake-4.3-2.fc14
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
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.
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.
Package Change Request ====================== Package Name: premake New Branches: el5 el6 epel7 Owners: besser82 jaydg