Bug 249034 - Review Request: sundials - nonlinear differential/algebraic solvers from LLNL
Review Request: sundials - nonlinear differential/algebraic solvers from LLNL
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Debarshi Ray
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-20 09:46 EDT by John Pye
Modified: 2014-12-03 07:19 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-07 08:31:32 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
debarshir: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description John Pye 2007-07-20 09:46:08 EDT
Spec URL: http://jpye.fedorapeople.org/sundials/sundials.spec
SRPM URL: http://jpye.fedorapeople.org/sundials/sundials-2.3.0-0.fc6.src.rpm

Description: 

SUNDIALS is a SUite of Non-linear DIfferential/ALgebraic equation Solvers
for use in mathematical software. It includes KINSOL for solving nonlinear
algebraic systems of equations, CVODE for solving initial value problems for
systems of ordinary differential equations, CVODES for solving ODE IVPs with
sensitivity analysis, and IDA for solving differential/algebraic equation
systems.

SUNDIALS was implemented with the goal of providing robust time integrators
and nonlinear solvers that can easily be incorporated into existing simulation
codes. The primary design goals were to require minimal information from the
user, allow users to easily supply their own data structures underneath the
solvers, and allow for easy incorporation of user-supplied linear solvers and
preconditioners.
Comment 1 John Pye 2007-07-29 08:09:31 EDT
Following comments from Jason Tibbetts, I have uploaded a new version of the
.spec file and .src.rpm. They are both at:

http://jpye.fedorapeople.org/sundials/
Comment 2 John Pye 2007-07-29 23:05:15 EDT
Following comments from Debarshi ray, new files are located at

http://ascend.cheme.cmu.edu/ftp/jpye/
Comment 3 John Pye 2007-07-30 01:53:37 EDT
Further corrections have been added, as annotated in the .spec file. See
http://ascend.cheme.cmu.edu/ftp/jpye/
Comment 4 Debarshi Ray 2007-07-30 01:58:35 EDT
Some of the flags that you are passing manually to './configure' in the %build
stanza are automatically taken care of by the %configure macro. You could remove
the unnecessary ones and use the rpmbuild defaults.
Comment 5 John Pye 2007-07-30 02:43:27 EDT
I have checked this on Fedora Core 5, as emailed, and I found that there weren't
any excess flags. Can you please tell me which ones are superfluous? Perhaps the
%configure macro has changed in newer versions of RPM.
Comment 6 John Pye 2007-07-30 02:46:25 EDT
For the record, here is the email discussion thread:
https://www.redhat.com/archives/fedora-maintainers/2007-July/msg00400.html
Comment 7 manuel wolfshant 2007-07-30 05:17:25 EDT
I've taken a look at the spec and had the src.rpm built in mock.
There is a small problem with the Source tag, the URL listed over there
(http://ascend.cheme.cmu.edu/ftp/sundials-2.3.0.tar.gz) is not reachable. And it
is not the link provided by upstream. The fact that one has to actually agree to
the BSD license and provide a username+email address before using
http://www.llnl.gov/casc/sundials/download/download.html for downloading the
source is a bit of nuisance, but I would still use this URL as Source0 rather
then a copy placed somewhere else.

Minor problems which you might want to fix before uploading to cvs
- autoconf is already a BR of automake
- rpmlint on the source rpm gives:Source RPM:
W: sundials macro-in-%changelog configure
W: sundials mixed-use-of-spaces-and-tabs (spaces: line 11, tab: line 1)
These are easy to fix, just replace '%configure' with '%%configure' in changelog
and stick with either tabs or spaces in the spec.

 rpmlint sundials-devel-2.3.0-2.fc8.x86_64.rpm gives
W: sundials-devel no-documentation
which can be ignored, since we have a separate -doc rpm.

Looking at the mock build log, the flags used during compile seem OK to me.
Comment 8 John Pye 2007-07-30 06:23:43 EDT
Regarding the Source0 URL, my view is that a URL that does not work with a
simple 'wget' does not count as a download URL. So, I have added a comment to
that effect, and have also corrected the URL to my mirror copy of the source
tarball. Any person looking at the .spec file will be able to use the original
download location, while automated tools such as 'spectool' will succeed through
the use of my mirrored copy.

I have removed 'autoconf' as a build-time dependency and corrected teh
macro-in-%changelog problem, and changed all tags to spaces.

http://ascend.cheme.cmu.edu/ftp/jpye/

Any other issues?


Comment 9 Debarshi Ray 2007-07-30 08:19:17 EDT
You need not pass the "CC=gcc CXX=g++" flags to %configure. They are set by
default. 

Why are these commented lines kept in the spec file:
#aclocal
#autoconf
#autoheader
?

I will do a formal review once I am back on my own machine tonight.
Comment 10 Debarshi Ray 2007-07-30 08:22:36 EDT
For the '-doc' subpackage use 'Documentation' as the value of the 'Group' tag.
See
http://fedoraproject.org/wiki/Packaging/Guidelines#head-9bbfa57478f0460c6160947a6bf795249488182b
Comment 11 Debarshi Ray 2007-07-30 08:28:32 EDT
<pedantic>
Instead of  passing '--enable-static=no' '%configure', you could use
'--disable-static' as mentioned at
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7
</pedantic>
Comment 12 Debarshi Ray 2007-07-30 08:58:32 EDT
http://www.llnl.gov/casc/sundials/download/code/sundials-2.3.0.tar.gz can be
used as the Source0 value.

rishi@fencepost:~$ wget
http://www.llnl.gov/casc/sundials/download/code/sundials-2.3.0.tar.gz
--08:59:13--  http://www.llnl.gov/casc/sundials/download/code/sundials-2.3.0.tar.gz
           => `sundials-2.3.0.tar.gz'
Resolving www.llnl.gov... 198.128.246.160
Connecting to www.llnl.gov|198.128.246.160|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 6,469,146 (6.2M) [application/x-tar]

27% [=========>                           ] 1,767,968    335.33K/s    ETA 00:14
Comment 13 Debarshi Ray 2007-07-30 09:03:49 EDT
The 'mkdir -p $RPM_BUILD_ROOT' in the '%install' stanza is not needed. Is there
any specific reason you used it? Otherwise, consider removing it.
Comment 14 John Pye 2007-07-30 09:14:20 EDT
Hi Debarshi,

Have updated:
- download location
- Group for -doc pkg
- removed mkdir -p $RPM_BUILD_ROOT
- CC and CXX args

Have ignored <pedantry/> ;-)

New stuff at
http://ascend.cheme.cmu.edu/ftp/jpye/
Comment 15 Debarshi Ray 2007-07-30 09:22:51 EDT
Update the '%changelog' stanza, and optionally give the '%release' a bump.

My question regarding:
%build
# Building this RPM on some platforms requires the following
#aclocal
#autoconf
#autoheader
...still stands.
Comment 16 John Pye 2007-07-30 09:30:04 EDT
I *did* update the changelog stanza. I choose not to bump %release. Does it
really matter that I leave some comments there?
Comment 17 John Pye 2007-07-30 09:33:41 EDT
OK I see now, 'Mon 29 Jul' is wrong. Will upload again.
Comment 18 John Pye 2007-07-30 09:40:41 EDT
New stuff at 
http://ascend.cheme.cmu.edu/ftp/jpye/
Comment 19 Debarshi Ray 2007-07-30 23:18:51 EDT
+---------------+
| FORMAL REVIEW  |
+---------------+

- MUST: rpmlint must be run on every package. The output should be posted in the
review.

Fine. Warning can be ignored since separate -doc subpackage is being provided.

[rishi@ginger i386]$ rpmlint sundials-2.3.0-2.fc8.i386.rpm 
[rishi@ginger i386]$ rpmlint sundials-debuginfo-2.3.0-2.fc8.i386.rpm 
[rishi@ginger i386]$ rpmlint sundials-devel-2.3.0-2.fc8.i386.rpm 
W: sundials-devel no-documentation
[rishi@ginger i386]$ rpmlint sundials-doc-2.3.0-2.fc8.i386.rpm 
[rishi@ginger i386]$ 

[rishi@ginger SRPMS]$ rpmlint sundials-2.3.0-2.fc8.src.rpm 
[rishi@ginger SRPMS]$

- MUST: The package must be named according to the Package Naming Guidelines.

Fine.

- MUST: The spec file name must match the base package %{name}, in the format
%{name}.spec unless your package has an exemption on Package Naming Guidelines.

Fine.

- MUST: The package must meet the Packaging Guidelines.

You need not install the INSTALL_NOTES as documentation. See:
https://fedoraproject.org/wiki/Packaging/Guidelines#head-9bbfa57478f0460c6160947a6bf795249488182b

- MUST: The package must be licensed with an open-source compatible license and
meet other legal requirements as defined in the legal section of Packaging
Guidelines.

Fine.

- MUST: The License field in the package spec file must match the actual license.

Fine.

- MUST: If (and only if) the source package includes the text of the license(s)
in its own file, then that file, containing the text of the license(s) for the
package must be included in %doc.

Fine.

- MUST: The spec file must be written in American English.

<pedantic>
A few sentences in the '%changelog' stanza lack full stops.
</pedantic>

- MUST: The spec file for the package MUST be legible. If the reviewer is unable
to read the spec file, it will be impossible to perform a review. Fedora is not
the place for entries into the Obfuscated Code Contest (http://www.ioccc.org/).

Fine.

- MUST: The sources used to build the package must match the upstream source, as
provided in the spec URL. Reviewers should use md5sum for this task. If no
upstream URL can be specified for this package, please see the  Source URL
Guidelines for how to deal with this.

Fine.

- MUST: The package must successfully compile and build into binary rpms on at
least one supported architecture.

Fine. Koji info: http://koji.fedoraproject.org/koji/taskinfo?taskID=82674

- MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch. Each architecture listed in ExcludeArch needs to have a bug filed
in bugzilla, describing the reason that the package does not compile/build/work
on that architecture. The bug number should then be placed in a comment, next to
the corresponding ExcludeArch line. New packages will not have bugzilla entries
during the review process, so they should put this description in the comment
until the package is approved, then file the bugzilla entry, and replace the
long explanation with the bug number. (Extras Only) The bug should be marked as
blocking one (or more) of the following bugs to simplify tracking such issues:
FE-ExcludeArch-x86, FE-ExcludeArch-x64, FE-ExcludeArch-ppc, FE-ExcludeArch-ppc64 

Not applicable. Koji info: http://koji.fedoraproject.org/koji/taskinfo?taskID=82674

- MUST: All build dependencies must be listed in BuildRequires, except for any
that are listed in the exceptions section of Packaging Guidelines; inclusion of
those as BuildRequires is optional. Apply common sense.

Why is 'BuildRequires: automake' necessary? I don't find any use of 'automake'
during the build process.

- MUST: The spec file MUST handle locales properly. This is done by using the
%find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.

Fine.

- MUST: Every binary RPM package which stores shared library files (not just
symlinks) in any of the dynamic linker's default paths, must call ldconfig in
%post and %postun. If the package has multiple subpackages with libraries, each
subpackage should also have a %post/%postun section that calls /sbin/ldconfig.

You need to invoke /sbin/ldconfig for the -devel subpackage too, since it also
includes shared libraries.

- MUST: If the package is designed to be relocatable, the packager must state
this fact in the request for review, along with the rationalization for
relocation of that specific package. Without this, use of Prefix: /usr is
considered a blocker.

Is the '#  --with-mpi-root=/usr/share/openmpi' line in '%build' stanza
necessary? If you _must_ keep it, then replace /usr/share with %{_datadir}.

John, just have look at the above comments. I will go through the remaining
issues (http://fedoraproject.org/wiki/Packaging/ReviewGuidelines), and let you
know further.

Mamoru, feel free to let me know if I have slipped up somewhere. Since this is
my first review, your help might be useful.
Comment 20 John Pye 2007-07-31 00:27:40 EDT
* INSTALL_NOTES removed.
* Corrected punct in changelog.
* removed automake dep
* added %post and %postun for -devel pkg (not sure about this, as the shared
libraries contained in the -devel package are only symlinks and contain no new
SONAMEs. But have done as requested).
* it is definitely desirable that MPI support be added once build issues with
that are resolved. So this is a valid reminder. Regarding %{_datadir} I'm not
sure I totally see the logic there, but have changed it anyway. No stress.
* bumped release to 3.

New files at http://ascend.cheme.cmu.edu/ftp/jpye/
Note new filename for .src.rpm.

Comment 21 Mamoru TASAKA 2007-07-31 03:45:31 EDT
Well,

(In reply to comment #19)
> Mamoru, feel free to let me know if I have slipped up somewhere. Since this is
> my first review, your help might be useful.
Okay, I will check your review later (I have to check other person's
package first..), but:


(In reply to comment #20)
> * added %post and %postun for -devel pkg (not sure about this, as the shared
> libraries contained in the -devel package are only symlinks and contain no new
> SONAMEs. But have done as requested).
Acutally this is not needed.
I will check if there is something else to fix anyway
Comment 22 Mamoru TASAKA 2007-07-31 14:38:39 EDT
Well,
* /sbin/ldconfig call
  - calling /sbin/ldconfig is not needed for -devel package

* static archive
  - split static archives from -devel package, move them to -static
    subpackage and make -static subpackage require -devel subpackge
    (check: Packaging Static Libraries of
     http://fedoraproject.org/wiki/Packaging/Guidelines )

* dependency for main/subpackage
  - Usually the dependency for main/subpackage must be version-release
    specific
    i.e. -devel package must have: 
    "Requires: %{name} = %{version}-%{release}"

* defattr
  - We recommend %defattr(-,root,root,-)
  - Also this must be added to all subpackages.

(In reply to comment #19)
> Mamoru, feel free to let me know if I have slipped up somewhere. 
> Since this is
> my first review, your help might be useful.
Well, when you want to review in this style, please also write the
summary of where to fix so that your review can be read easily.

Comment 23 John Pye 2007-08-01 02:41:52 EDT
Hi Tasaka,

I have made changes according to your suggestions. FWIW on FC5, I get an *error*
with rpmlint about having a dependency on the -devel package -- is that OK?
Perhaps rpmlint should be backported to FC5?

Anyway, I have removed the ldconfig, split out the -static package, added the
release ID to the sub-package dependencies and added the defattr thing as you said.

Please let me know if/what more is required. And thanks for your help.
http://ascend.cheme.cmu.edu/ftp/jpye/

Cheers
JP
Comment 24 Mamoru TASAKA 2007-08-01 11:59:30 EDT
(In reply to comment #23)
> Hi Tasaka,
> 
> I have made changes according to your suggestions. FWIW on FC5,
> I get an *error*
> with rpmlint about having a dependency on the -devel package 
On rawhide, there is no error (just note that FC5 is no longer
supported)

-4 seems good. I leave the judgment to Debarshi.
Comment 25 John Pye 2007-08-03 07:09:43 EDT
Hi Debarshi,

I think that you need to mark this as 'fedora-review +' and possibly 'fedora-cvs
+' so that I can proceed, assuming you're happy with the package now.

Cheers
JP
Comment 26 Debarshi Ray 2007-08-03 16:58:50 EDT
+------------------------+
| FORMAL REVIEW (contd.) |
+------------------------+

* Due to a change in the Licensing Guidelines
(http://fedoraproject.org/wiki/Packaging/LicensingGuidelines) the value of the
License tag, should be filled with the appropriate license Short License
identifier(s) from the "Good License" tables on the Fedora Licensing
(http://fedoraproject.org/wiki/Licensing) page.

"BSD-style" is not one of the accepted values for the License tag.

* Your usage of macros is inconsistent. eg., you have used %{name}-%{version} in
Source0 while using hard-coded strings in Patch0; you have used both %{_libdir}
and %_libdir to refer to directories; etc..

A possible solution would be to use:
Patch0: %{name}-ltmain.patch
...and decide on whether to use the braces ('{' '}') or not while referring to
directories.

Consistency is necessary for QA and usability. See
http://fedoraproject.org/wiki/Packaging/Guidelines#macros
Comment 27 Debarshi Ray 2007-08-03 17:19:33 EDT
I am curious about the reason for this comment:
# Parallel build seems to require some fixes from upstream

I have twice scratch built this package on Koji without any issues on all the
four architectures (i386, x86_64, ppc, ppc64) without any problems. So what are
the fixes that you have commented about?
Comment 28 John Pye 2007-08-04 01:48:06 EDT
* Regarding the licensing guidelines, email sent to "Spot" Callaway.
* Your point about macros is rather pedantic. I don't see the value in going
through another cycle to correct this.
* The comment about parallel builds indicates that I tried to include support
for nvec_parallel but failed due to an build error. From what I could tell, it
*seemed* to need some patching of the upstream code. For the moment I will not
be providing parallel support but would hope to do so in future. Note that the
Debian package of SUNDIALS does not include parallel either.

Requesting CVS access.
Comment 29 Kevin Fenzi 2007-08-04 02:03:57 EDT
John. Please wait and only request CVS access once your package has been
approved (fedora-review flag is +).
Comment 30 Debarshi Ray 2007-08-04 03:50:04 EDT
(In reply to comment #28)

> * Your point about macros is rather pedantic. I don't see the value in going
> through another cycle to correct this.

It may seem pedantic when we see it in the perspective of one package. Take into
consideration a 4000 packages, and suddenly such things become important. Any
way "consisten use of macros" is a "must" item in
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines and I would like you to
do something about it.

In any case we have to wait for the Licensing issue to be resolved.

> * The comment about parallel builds indicates that I tried to include support
> for nvec_parallel but failed due to an build error. From what I could tell, it
> *seemed* to need some patching of the upstream code. For the moment I will not
> be providing parallel support but would hope to do so in future. Note that the
> Debian package of SUNDIALS does not include parallel either.

Ok.

Comment 31 John Pye 2007-08-04 04:50:06 EDT
I have uploaded another version that fixes the macro issue.
I have changed the License to BSD after looking over the additional conditions
in the SUNDIALS license and seeing that they really weren't restricting the use
of the code in any way.
http://ascend.cheme.cmu.edu/ftp/jpye/

Please take a look and if you are happy give me 'review +'. I suspect it might
not be necessary to wait for Spot's reply, as (note) the 'License' field is in
no way legally binding.
Comment 32 Debarshi Ray 2007-08-04 12:42:36 EDT
+----------+
| APPROVED |
+----------+

John, you may like to take a look at
http://fedoraproject.org/wiki/PackageMaintainers/FEver to receive automatic
reminders when there is a new upstream release.
Comment 33 John Pye 2007-08-04 23:02:41 EDT
Note: response from Spot:

Actually, the extra items on the license are not new conditions, nor are
they exceptions.

In vast simplifications, they say:

1. LLNL did this for the DOE.
2. The US government disclaims liability (which the BSD license above it
already did)
3. the US goverment doesn't endorse or recommend this, nor can you use
its trademarks to imply this. (again, the BSD license above it already
covered it).

So, just saying "BSD" is correct here, since there really are no
additional restrictions or exceptions.

~spot
Comment 34 John Pye 2007-08-04 23:12:44 EDT
Note: for use of 'FEver' the following settings seem to be ok and will be put in
place once I get edit privileges on the Fedora Wiki:

URL: http://www.llnl.gov/CASC/sundials/download/download.html
Filename (regex): sundials-([0-9.]+)\.tar\.[gzb2]+
Comment 35 Debarshi Ray 2007-08-04 23:58:11 EDT
Follow http://fedoraproject.org/wiki/WikiEditing to get edit access on the Wiki.
Comment 36 John Pye 2007-08-05 00:14:39 EDT
I have done that. Apparently I am supposed to ask someone who has edit
privileges to give me permission. You are on that list.
Comment 37 Debarshi Ray 2007-08-05 01:45:10 EDT
(In reply to comment #36)

> I have done that. Apparently I am supposed to ask someone who has edit
> privileges to give me permission. You are on that list.

Oops! I did not know that I can give you permission. Well now that I know, what
is your Wiki user name? JohnPye?
Comment 38 John Pye 2007-08-05 02:17:59 EDT
JohnPye.
Comment 39 Debarshi Ray 2007-08-05 05:55:15 EDT
I have added you. Please create a Wiki page introducing yourself at
http://fedoraproject.org/wiki/JohnPye .

If there are any issues, let me know. Happy editing...
Comment 40 Debarshi Ray 2007-08-05 05:56:36 EDT
Once you have imported sundials into CVS and built it successfully, remember to
close this bug as "NEXTRELEASE".
Comment 41 Kevin Fenzi 2007-08-05 14:40:45 EDT
John: Can you post a cvs template so we know what branches and such you want? 
See: http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure
for templates/examples. Reset fedora-cvs to ? when you are ready. 
Thanks.
Comment 42 John Pye 2007-08-05 22:43:33 EDT
Hi Kevin,
Sorry, I forgot about that step.

New Package CVS Request
=======================
Package Name: sundials
Short Description: Suite of nonlinear solvers
Owners: john@curioussymbols.com
Branches: FC-6 F-7
InitialCC: john@curioussymbols.com
Comment 43 John Pye 2007-08-07 08:31:32 EDT
closing as NEXTRELEASE.
Comment 44 Mukundan Ragavan 2014-12-02 20:45:56 EST
Package Change Request
======================
Package Name: sundials
New Branches: epel7
Owners: nonamedotc


I am the ownner of this package (from the recent past).
Comment 45 Gwyn Ciesla 2014-12-03 07:19:42 EST
Git done (by process-git-requests).

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