Bug 457038

build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=745087

- starting review -

1/ It would be fine to keep timestamps while converting README.txt to UTF8
2/ This package doesn't use our RPM_OPT_FLAGS
gcc -c -g -Wall -D__USE_FIXED_PROTOTYPES__ -O2 -DDPAL_MAX_ALIGN=36
-DMAX_PRIMER_LENGTH=36 primer3_main.c
3/ It seems possible to build a Shared library :
OLIGOTM_DYN_LIB = liboligotm.so.1.2.0
 You can note that this library will probably need to be installed manually as
oligotm.h will probably need the releated liboligotm.a (or .so) to be bundled in
prism3-devel 
Do you know any dependent package to test primer3-devel ?
I guess not all headers will be needed...
4/ Make check is usally done in a separate job control (before %clean)
%check
pushd src
%{?_with_tests:make check}
popd
This will probably need to be conditionalized as some tests take time.
(tested succeded on F-8 x86_64 local rpmbuild - so it's good)
5/ Why BuildRequires:  perl ?
This is in the default buildRequirement, but I wonder why you have added it ?

(In reply to comment #2)
> 1/ It would be fine to keep timestamps while converting README.txt to UTF8
I am using now touch -r COPYING.txt README.txt after the conversion to UTF8.
Is it fine ?

> 2/ This package doesn't use our RPM_OPT_FLAGS
> gcc -c -g -Wall -D__USE_FIXED_PROTOTYPES__ -O2 -DDPAL_MAX_ALIGN=36
> -DMAX_PRIMER_LENGTH=36 primer3_main.c
Could you give me some insigth on how I can change this ? 

> 3/ It seems possible to build a Shared library :
> OLIGOTM_DYN_LIB = liboligotm.so.1.2.0
>  You can note that this library will probably need to be installed manually as
> oligotm.h will probably need the releated liboligotm.a (or .so) to be bundled in
> prism3-devel 
> Do you know any dependent package to test primer3-devel ?
Actually, I do not know any.

> I guess not all headers will be needed...
I would say so to, but since I am not sure I prefered to put them all rather
than miss one.

> 4/ Make check is usally done in a separate job control (before %clean)
> %check
> pushd src
> %{?_with_tests:make check}
> popd
added

> This will probably need to be conditionalized as some tests take time.
> (tested succeded on F-8 x86_64 local rpmbuild - so it's good)

> 5/ Why BuildRequires:  perl ?
> This is in the default buildRequirement, but I wonder why you have added it ?
It's the only dependency that I found on the website. I can removed it if it's
there by default

There are the new files
http://pingou.fedorapeople.org/RPMs/primer3.spec
http://pingou.fedorapeople.org/RPMs/primer3-1.1.4-2.fc9.src.rpm

%changelog
* Wed Aug 06 2008 pingou <pingoufc4> 1.1.4-2
- Keep the timestamp in the README.txt
- Change the CFLAG for the compilation
- Remove BR perl

- README.txt convertion can be simplified. 
- you need to have export INIT_CFLAGS="%{optflags}" in the %build section
- make test remains in the %build section.
- non-standard perms for binaries, should be 755

It still seems strange if you don't install the .a or .so when you install some .h for the -devel package. Is it possible or relevant to build a shared onject library instead ?

(In reply to comment #5)
> - README.txt convertion can be simplified.
I have always done this in that way. Does it really matter ? I would prefer be consistent over my spec file.
> - you need to have export INIT_CFLAGS="%{optflags}" in the %build section
Done
> - make test remains in the %build section.
Done
> - non-standard perms for binaries, should be 755
Done


> It still seems strange if you don't install the .a or .so when you install some
> .h for the -devel package. Is it possible or relevant to build a shared onject
> library instead ?

I have never done such sub-package could you point me to an example or so doc ?

SPEC
http://pingou.fedorapeople.org/RPMs/primer3.spec
SRPM
http://pingou.fedorapeople.org/RPMs/primer3-1.1.4-3.fc9.src.rpm

I corrected the make test to include the conditionnality
--with tests does the tests (make test)
defaults does not

SPEC
http://pingou.fedorapeople.org/RPMs/primer3.spec
SRPM
http://pingou.fedorapeople.org/RPMs/primer3-1.1.4-4.fc9.src.rpm

export INIT_CFLAGS="%{optflags} -fPIC -DPIC"
make %{?_smp_mflags} liboligotm.so.1.2.0
make %{?_smp_mflags}
make %{?_smp_mflags} primer3_core.dyn

This will build a shared version of the library along with a binary linked with the shared library. Maybe -fPIC isn't needed on i386 for some little library (you need to test), but on x86_64, it is mandatory.

Ok I have removed the -devel package and I did not do the shared library because I do not know any project that needs those headers files.

I am keeping this package as simple as possible.

SPEC
http://pingou.fedorapeople.org/RPMs/primer3.spec
SRPM
http://pingou.fedorapeople.org/RPMs/primer3-1.1.4-5.fc9.src.rpm

--------------------
This package (primer3) is APPROVED by me
--------------------

New Package CVS Request
=======================
Package Name: primer3
Short Description: PCR primer design tool
Owners: pingou
Branches: F-8 F-9
InitialCC:

cvs done.

primer3-1.1.4-5.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/primer3-1.1.4-5.fc9

primer3-1.1.4-5.fc8 has been submitted as an update for Fedora 8.
http://admin.fedoraproject.org/updates/primer3-1.1.4-5.fc8

primer3-1.1.4-5.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

primer3-1.1.4-5.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.