Bug 1067041 - Review Request: autodocksuite - AutoDock is a suite of docking tools to study protein-ligand interaction
Summary: Review Request: autodocksuite - AutoDock is a suite of docking tools to study...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Cole Robinson
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-02-19 14:43 UTC by Mukundan Ragavan
Modified: 2014-06-30 12:14 UTC (History)
2 users (show)

Fixed In Version: autodocksuite-4.2.5.1-5.fc19
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-03-07 06:25:57 UTC
Type: ---
Embargoed:
crobinso: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Mukundan Ragavan 2014-02-19 14:43:34 UTC
Spec URL: http://nonamedotc.fedorapeople.org/pkgreview/autodocksuite/autodocksuite.spec
SRPM URL: http://nonamedotc.fedorapeople.org/pkgreview/autodocksuite/autodocksuite-4.2.5.1-2.fc20.src.rpm

Description:
AutoDock is a suite of automated docking tools. It is designed to predict \
how small molecules, such as substrates or drug candidates, bind to a \
receptor of known 3D structure. AutoDock 4 actually consists of two main \
programs: autodock performs the docking of the ligand to a set of grids \
describing the target protein; autogrid pre-calculates these grids. In \
addition to using them for docking, the atomic affinity grids can be \
visualized. This can help, for example, to guide organic synthetic chemists \
design better binders.

Fedora Account System Username: nonamedotc

* mock builds fine

koji scratch build here - http://koji.fedoraproject.org/koji/taskinfo?taskID=6547436


Question I have - Is the versioning fine for the package?

My own "review" - 
* There are a bunch of spelling error warnings in rpmlint (all of them are fine, as far as I can tell)

* incorrect-fsf-address errors: I will inform upstream

Comment 2 Cole Robinson 2014-02-19 17:06:56 UTC
I'll take this

Comment 3 Cole Robinson 2014-02-19 17:22:10 UTC
The version 4.2.5.1 is fine, it comes from upstream and RPM can handle it for comparisons.

Besides the noted rpmlint spelling errors and fsf address, there's also this minor one:

autodocksuite.src:24: W: mixed-use-of-spaces-and-tabs (spaces: line 24, tab: line 1)

In my .vimrc, I have: set listchars=tab:>.  which shows hard tabs as visible, but that's totally up to you. I'd recommend being consistent in the spec at least.

Couple other points:

- I don't think triggering ldconfig is required, the package isn't installing any shared libraries.

- autodoc/COPYING is duplicated between the packages. Just stick it in the base package, since -doc requires the base package. I'd also stick the README in the -doc package but it's up to you.

- In %build you have:

export CFLAGS="%{optflags}" CXXFLAGS="%{optflags}"

Does that make a difference? The %configure macro should do that for you.


FYI I'm offline till Monday, sorry for bad timing. I'll follow up ASAP when I'm back

Comment 4 Mukundan Ragavan 2014-02-19 17:48:57 UTC
Hi Cole,

Thanks for your comments.

(In reply to Cole Robinson from comment #3)
> 
> Besides the noted rpmlint spelling errors and fsf address, there's also this
> minor one:
> 
> autodocksuite.src:24: W: mixed-use-of-spaces-and-tabs (spaces: line 24, tab:
> line 1)
> 
> In my .vimrc, I have: set listchars=tab:>.  which shows hard tabs as
> visible, but that's totally up to you. I'd recommend being consistent in the
> spec at least.

Fixed it. Actually, vimrc is a good suggestion. I will add it.

> 
> Couple other points:
> 
> - I don't think triggering ldconfig is required, the package isn't
> installing any shared libraries.

Done!

> 
> - autodoc/COPYING is duplicated between the packages. Just stick it in the
> base package, since -doc requires the base package. I'd also stick the
> README in the -doc package but it's up to you.

Done! README in -doc would be more appropriate.

> 
> - In %build you have:
> 
> export CFLAGS="%{optflags}" CXXFLAGS="%{optflags}"
> 
> Does that make a difference? The %configure macro should do that for you.
> 

Fixed. That was from a older spec file. Sorry. :)

New SPEC URL: http://nonamedotc.fedorapeople.org/pkgreview/autodocksuite/autodocksuite.spec
New SRPM URL: http://nonamedotc.fedorapeople.org/pkgreview/autodocksuite/autodocksuite-4.2.5.1-4.fc20.src.rpm

My fedora-review on updated files (sorry, I did not do the [x] marks this time) - http://nonamedotc.fedorapeople.org/pkgreview/autodocksuite/review.txt

The older review.txt is here - http://nonamedotc.fedorapeople.org/pkgreview/autodocksuite/ver2/review.txt

Comment 5 Christopher Meng 2014-02-20 03:56:51 UTC
1. doc package doesn't have the strong reason to

Requires:	%{name}%{?_isa} = %{version}-%{release}

So please remove this line.

2. cd autodock
%configure
make %{?_smp_mflags}

cd %{_builddir}/src/autogrid
%configure
make %{?_smp_mflags}

My optimization:

pushd autodock
%configure
make %{?_smp_mflags}
popd

pushd autogrid
%configure
make %{?_smp_mflags}
popd(this popd is optional)

3. %install
cd autodock
make install DESTDIR=%{buildroot}

cd %{_builddir}/src/autogrid
make install DESTDIR=%{buildroot}

Mine:

make -C autodock install DESTDIR=%{buildroot}
make -C autogrid install DESTDIR=%{buildroot}

Comment 6 Mukundan Ragavan 2014-02-20 14:11:44 UTC
Thank you Christopher!

(In reply to Christopher Meng from comment #5)
> 1. doc package doesn't have the strong reason to
> 
> Requires:	%{name}%{?_isa} = %{version}-%{release}
> 
> So please remove this line.
>

Done! 

> 2. cd autodock
> %configure
> make %{?_smp_mflags}
> 
> cd %{_builddir}/src/autogrid
> %configure
> make %{?_smp_mflags}
> 
> My optimization:
> 
> pushd autodock
> %configure
> make %{?_smp_mflags}
> popd
> 
> pushd autogrid
> %configure
> make %{?_smp_mflags}
> popd(this popd is optional)


Ok, I did not even know about pushd and popd. So, thanks a lot! Done.

> 
> 3. %install
> cd autodock
> make install DESTDIR=%{buildroot}
> 
> cd %{_builddir}/src/autogrid
> make install DESTDIR=%{buildroot}
> 
> Mine:
> 
> make -C autodock install DESTDIR=%{buildroot}
> make -C autogrid install DESTDIR=%{buildroot}

Done.


New SPEC URL: http://nonamedotc.fedorapeople.org/pkgreview/autodocksuite/autodocksuite.spec
New SRPM URL: http://nonamedotc.fedorapeople.org/pkgreview/autodocksuite/autodocksuite-4.2.5.1-5.fc20.src.rpm

Comment 7 Christopher Meng 2014-02-21 06:18:09 UTC
My next question, do we really need a separate doc package? Reason?

Comment 8 Mukundan Ragavan 2014-02-21 15:08:59 UTC
The doc file contains a user guide that is also available online (in the same form - pdf) and I just wanted to make it "optional".

Also, rpmlint complains if I package it in the main package.

Comment 9 Christopher Meng 2014-02-22 00:32:58 UTC
Rpmlint sometimes can't reflect the needs from users. 

But your opinion is right. 

Fine.

Comment 10 Mukundan Ragavan 2014-02-24 18:54:27 UTC
koji scratch build with the latest SRPM - http://koji.fedoraproject.org/koji/taskinfo?taskID=6566124

(i686 and arm done at the time of adding this comment)

Comment 11 Cole Robinson 2014-02-25 16:43:29 UTC
Files posted in Comment #6 look good to me

Comment 12 Mukundan Ragavan 2014-02-25 16:46:51 UTC
Thanks Cole!

New Package SCM Request
=======================
Package Name: autodocksuite
Short Description: AutoDock is a suite of docking tools to study protein-ligand interaction
Owners: nonamedotc
Branches: f19 f20
InitialCC:

Comment 13 Gwyn Ciesla 2014-02-25 18:52:46 UTC
Git done (by process-git-requests).

Comment 14 Fedora Update System 2014-02-25 22:04:35 UTC
autodocksuite-4.2.5.1-5.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/autodocksuite-4.2.5.1-5.fc19

Comment 15 Fedora Update System 2014-02-25 22:05:23 UTC
autodocksuite-4.2.5.1-5.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/autodocksuite-4.2.5.1-5.fc20

Comment 16 Fedora Update System 2014-02-26 13:55:36 UTC
autodocksuite-4.2.5.1-5.fc19 has been pushed to the Fedora 19 testing repository.

Comment 17 Fedora Update System 2014-03-07 06:25:57 UTC
autodocksuite-4.2.5.1-5.fc20 has been pushed to the Fedora 20 stable repository.

Comment 18 Fedora Update System 2014-03-07 06:30:21 UTC
autodocksuite-4.2.5.1-5.fc19 has been pushed to the Fedora 19 stable repository.

Comment 19 Mukundan Ragavan 2014-06-27 21:11:13 UTC
Package Change Request
======================
Package Name: autodocksuite
New Branches: epel7
Owners: nonamedotc

Comment 20 Gwyn Ciesla 2014-06-30 12:14:25 UTC
Git done (by process-git-requests).


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