Bug 549496

Summary: Review Request: bakefile - A cross-platform, cross-compiler native makefiles generator
Product: [Fedora] Fedora Reporter: Filipe Rosset <rosset.filipe>
Component: Package ReviewAssignee: Dan Horák <dan>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: dan, fedora-package-review, notting, panemade, terjeros
Target Milestone: ---Flags: dan: fedora-review+
tibbs: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
URL: http://www.bakefile.org
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-03-04 18:18:01 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Bug Depends On: 567713    
Bug Blocks:    

Description Filipe Rosset 2009-12-21 20:29:15 UTC
Spec URL: http://filiperosset.fedorapeople.org/packages/bakefile/bakefile.spec
SRPM URL: http://filiperosset.fedorapeople.org/packages/bakefile/bakefile-0.2.8-1.fc12.src.rpm
Description: Bakefile is cross-platform, cross-compiler native makefiles generator. It takes compiler-independent description of build tasks as input and generates native makefile (autoconf's Makefile.in, Visual C++ project, bcc makefile etc.).

Tested with rpmlint, mock and koji.
Koji results: https://koji.fedoraproject.org/koji/taskinfo?taskID=1884369

This is my second package.
First package: https://bugzilla.redhat.com/show_bug.cgi?id=549366

Comment 1 Terje Røsten 2009-12-21 20:54:13 UTC
Some random comments.

a) Is summary a bit strange? Drop the leading verb?

b) Useless line: %doc. Is there more info available in the package?

c) I am favor of more explicit %files section,  %{_libdir}/* 
   can pick up random stuff.

d) The source url is not correct, just drop %{name} macro here.

e) %{_mandir}/man1/bakefil*.gz change to %{_mandir}/man1/bakefil*
 (compression might change to e.g. xz)

f) commas in buildreq/req is just noise

Comment 2 Terje Røsten 2009-12-21 20:56:15 UTC
g) is the url http://www.bakefile.org/ or http://bakefile.sourceforge.net/ ?

Comment 3 Fabian Affolter 2009-12-21 21:20:35 UTC
Just some quick comment on your spec file.

- The Source URL has a typo in one name macros.
- Please check your BR.  Isn't python-devel requiring python?

Comment 4 Fabian Affolter 2009-12-21 22:59:08 UTC
*** Bug 549495 has been marked as a duplicate of this bug. ***

Comment 5 Filipe Rosset 2009-12-22 13:00:31 UTC
Spec URL: http://filiperosset.fedorapeople.org/packages/bakefile/bakefile.spec
SRPM URL: http://filiperosset.fedorapeople.org/packages/bakefile/bakefile-0.2.8-2.fc12.src.rpm

New files with the corrections suggested in comments 1, 2 and 3.

Comment 6 Dan Horák 2010-01-09 11:47:43 UTC
formal review is here, see the notes below:

BAD	source files match upstream:
	    bf8394d944fb34fdce8d5b82c891c180dc7af05e  bakefile-0.2.8.tar.gz
OK	package meets naming and versioning guidelines.
OK	specfile is properly named, is cleanly written and uses macros consistently.
OK	dist tag is present.
OK	license field matches the actual license.
OK	license is open source-compatible (MIT). License text included in package.
OK	latest version is being packaged.
OK	BuildRequires are proper.
OK	compiler flags are appropriate.
OK	%clean is present.
OK	package builds in mock (Rawhide/x86_64).
OK	debuginfo package looks complete.
OK	rpmlint is silent.
BAD	final provides and requires look sane.
BAD	%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.
BAD	no libtool .la droppings.
OK	not a GUI app.

- use http://download.sourceforge.net/%{name}/%{name}-%{version}.tar.gz as the value of Source tag, the URL used doesn't lead to the source archive
- you must have automake as Requires for proper ownership of /usr/share/aclocal directory where some files are placed
- a test-suite is included in the source archive, but it's not run
- _bkl_c.la file must not be included, use a "rm" command in %install or %exclude in %files
- includes copies of uuid and subprocess python modules, system ones provided by the python package or some add-on package must be used
- includes a copy of empy python module, this one must be packaged independently

Comment 7 Dan Horák 2010-02-23 13:55:37 UTC
ping?

Comment 8 Filipe Rosset 2010-02-23 18:13:26 UTC
comment #6
- includes a copy of empy python module, this one must be packaged
independently


I open new review request to python-empy on:
https://bugzilla.redhat.com/show_bug.cgi?id=567713

Comment 9 Filipe Rosset 2010-03-02 03:48:25 UTC
Spec URL: http://filiperosset.fedorapeople.org/packages/bakefile/bakefile.spec
SRPM URL: http://filiperosset.fedorapeople.org/packages/bakefile/bakefile-0.2.8-3.fc12.src.rpm

%changelog
* Mon Mar 02 2010 Filipe Rosset <rosset.filipe@gmail.com> - 0.2.8-3
- Fixed Source url
- Updated BuildRequires and Requires info
- Empy python module packaged independently
- Removed files of uuid and subprocess python modules
- Removed .la files

I need help to solve this issue:
- a test-suite is included in the source archive, but it's not run

Comment 10 Dan Horák 2010-03-02 09:04:07 UTC
(In reply to comment #9)
> I need help to solve this issue:
> - a test-suite is included in the source archive, but it's not run    

The source archive contains the "tests" subdirectory that looks like as an internal test-suite for bakefile. It is more than advisable to run such test-suite during the package build process (uses %check section in spec). But I've tried it myself now and can't get it to do anything (autoconf_inc.m4 doesn't exist). Please ask the author how the test-suite should be used and eventually include it in the build process.

But all the remaining issues are fixed and the package is APPROVED now. I will now sponsor you in FAS and you can then request CVS branches for both bakefile and python-empy (https://fedoraproject.org/wiki/CVS_admin_requests#New_Packages). If you have any questions regarding Fedora packaging and processes now or in the future, don't hesitate to ask me.

Comment 11 Filipe Rosset 2010-03-02 13:33:39 UTC
New Package CVS Request
=======================
Package Name: bakefile
Short Description: A cross-platform, cross-compiler native makefiles generator
Owners: filiperosset
Branches: F-11 F-12 EL-5
InitialCC: filiperosset

Comment 12 Jason Tibbitts 2010-03-02 22:50:57 UTC
CVS done (by process-cvs-requests.py).

I added an F-13 branch as well since that seems to have been missed.