Bug 457038 - Review Request: primer3 - PCR primer design tool
Review Request: primer3 - PCR primer design tool
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nicolas Chauvet (kwizart)
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-29 08:10 EDT by Pierre-YvesChibon
Modified: 2008-10-30 08:55 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-10-30 08:51:27 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kwizart: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Pierre-YvesChibon 2008-07-29 08:10:07 EDT
Spec URL: http://pingou.fedorapeople.org/RPMs/primer3.spec
SRPM URL: http://pingou.fedorapeople.org/RPMs/primer3-1.1.4-1.fc9.src.rpm
Description: 
Primer3 is a widely used program for designing PCR primers (PCR = 
"Polymerase Chain Reaction"). PCR is an essential and ubiquitous 
tool in genetics and molecular biology. Primer3 can also design 
hybridization probes and sequencing primers.

PCR is used for many different goals. Consequently, primer3 has 
many different input parameters that you control and that tell 
primer3 exactly what characteristics make good primers for your goals.
Comment 1 Pierre-YvesChibon 2008-07-29 08:10:42 EDT
build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=745087
Comment 2 Nicolas Chauvet (kwizart) 2008-07-29 11:07:30 EDT
- 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 ?
Comment 3 Pierre-YvesChibon 2008-07-30 09:15:38 EDT
(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

Comment 4 Pierre-YvesChibon 2008-08-06 10:16:05 EDT
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@yahoo.fr> 1.1.4-2
- Keep the timestamp in the README.txt
- Change the CFLAG for the compilation
- Remove BR perl
Comment 5 Nicolas Chauvet (kwizart) 2008-08-07 03:15:08 EDT
- 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 ?
Comment 6 Pierre-YvesChibon 2008-08-11 07:38:18 EDT
(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
Comment 7 Pierre-YvesChibon 2008-08-12 11:19:14 EDT
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
Comment 8 Nicolas Chauvet (kwizart) 2008-08-12 12:04:27 EDT
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.
Comment 9 Pierre-YvesChibon 2008-10-06 04:32:00 EDT
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
Comment 10 Nicolas Chauvet (kwizart) 2008-10-06 10:09:43 EDT
--------------------
This package (primer3) is APPROVED by me
--------------------
Comment 11 Pierre-YvesChibon 2008-10-07 08:32:23 EDT
New Package CVS Request
=======================
Package Name: primer3
Short Description: PCR primer design tool
Owners: pingou
Branches: F-8 F-9
InitialCC:
Comment 12 Kevin Fenzi 2008-10-07 13:20:46 EDT
cvs done.
Comment 13 Fedora Update System 2008-10-16 15:50:18 EDT
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
Comment 14 Fedora Update System 2008-10-16 15:50:22 EDT
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
Comment 15 Fedora Update System 2008-10-30 08:51:24 EDT
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.
Comment 16 Fedora Update System 2008-10-30 08:55:35 EDT
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.

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