Bug 249034

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/

Following comments from Debarshi ray, new files are located at

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

Further corrections have been added, as annotated in the .spec file. See
http://ascend.cheme.cmu.edu/ftp/jpye/

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.

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.

For the record, here is the email discussion thread:
https://www.redhat.com/archives/fedora-maintainers/2007-July/msg00400.html

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.

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?

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.

For the '-doc' subpackage use 'Documentation' as the value of the 'Group' tag.
See
http://fedoraproject.org/wiki/Packaging/Guidelines#head-9bbfa57478f0460c6160947a6bf795249488182b

<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>

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

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.

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/

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.

I *did* update the changelog stanza. I choose not to bump %release. Does it
really matter that I leave some comments there?

OK I see now, 'Mon 29 Jul' is wrong. Will upload again.

New stuff at 
http://ascend.cheme.cmu.edu/ftp/jpye/

+---------------+
| 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.

* 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.

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

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.

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

(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.

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

+------------------------+
| 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

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?

* 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.

John. Please wait and only request CVS access once your package has been
approved (fedora-review flag is +).

(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.

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.

+----------+
| 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.

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

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]+

Follow http://fedoraproject.org/wiki/WikiEditing to get edit access on the Wiki.

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

(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?

JohnPye.

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...

Once you have imported sundials into CVS and built it successfully, remember to
close this bug as "NEXTRELEASE".

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.

Hi Kevin,
Sorry, I forgot about that step.

New Package CVS Request
=======================
Package Name: sundials
Short Description: Suite of nonlinear solvers
Owners: john
Branches: FC-6 F-7
InitialCC: john

closing as NEXTRELEASE.

Package Change Request
======================
Package Name: sundials
New Branches: epel7
Owners: nonamedotc


I am the ownner of this package (from the recent past).

Git done (by process-git-requests).