Bug 646789

Summary: Review Request: openturns - C++ reliability library
Product: [Fedora] Fedora Reporter: Michel Zou <xantares09>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED NOTABUG QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, notting, tomspur
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-05-01 15:21:55 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 201449    

Description Michel Zou 2010-10-26 08:58:31 UTC
Spec URL: https://api.opensuse.org/public/source/science:openturns/openturns/openturns.spec?rev=26ca42312f64f85630540eb70a468d64&
SRPM URL: http://download.opensuse.org/repositories/science:/openturns/Fedora_13/src/openturns-0.13.2-50.2.src.rpm
Description: OpenTURNS is a scientific C++ library including an internal data model and algorithms dedicated to the treatment of uncertainties.

- there is a python interface: 'python-openturns' that makes it usable from python
- there is a custom R package 'R-rot' that enables additional statistical capabilities through R, it also depends on 'R-sensitivity' I had to package too.

Regards

Comment 1 Thomas Spura 2010-10-26 11:54:53 UTC
- Are you going to use that spec file from above as a template or what?
  If you just want to get this into fedora, but not be the packager on your own, you can put this package to the wishlist, and maybe someone else will look at this:
https://fedoraproject.org/wiki/PackageMaintainers/WishList
- Are you already a packager?

If not see:
https://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Comment 2 Michel Zou 2010-10-27 07:28:24 UTC
Hi,

- It's not a template, it works.
- I'm not a packager yet, but I'm willing to do the job

Regards.

Comment 3 Michael Schwendt 2010-12-23 09:42:40 UTC
* Please enter your real name in your bugzilla.redhat.com account preferences. Currently, you are listed as "(none)" on the NEEDSPONSOR list, which isn't helpful.

* Also maintain your own %changelog entries in the RPM spec files. For example, openturns.spec only lists two entries where your name/email doesn't appear at all. The package review ought to start with an added changelog entry that marks the start of the packaging for Fedora.

* A brief look at the spec (not a full review):


> Summary:        Uncertainty treatment library
> Group:          Development/Libraries

Run-time library packages typically go into group "System Environment/Libraries".


> BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root

Note that several details related to BuildRoot are not necessary anymore in Fedora 13 and newer:
https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag


> Patch0:         typedefs.patch

It is common practice to add the package name and software version to Patch file names, e.g.  openturns-0.13.2-typedefs.patch

Not only does that make clear which package the separate patch files belong into and "when" they have been created, it also reduces the risk of conflicts (especially when working in environments where multiple releases may use the same SOURCES directory).


> Requires:       lapack
> 
> Requires:       R-rot = 1.4.4
> Requires:       R-sensitivity >= 1.3.0
[...and more...]

https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires


> Obsoletes:      %{name} < %{version}

Even if self-Obsoletes like this may be harmless, there ought to be a comment in the spec file explaining them. Also mention when this Obsoletes tag has been added and when/whether it may be removed again.


> %package examples
> Summary:        OpenTURNS examples
> Group:          Development/Libraries

Really?


> %package validation
> Summary:        OpenTURNS validation files
> Group:          Development/Libraries

Really?


> %build
> %configure --disable-python
> 
> %if 0%{?mdkversion}
> ...
> ./configure 
> ...

So, for Mandriva the configure script would be called twice? Once as %configure, then as ./configure. Doesn't look correct and adds to the fact that the attempt at writing multi-dist spec files often introduces errors.


> %check
> export LD_LIBRARY_PATH=%{buildroot}%{_libdir}/%{name}
> ...

Good effort.


> %files
> ...
> %config %{_sysconfdir}/%{name}/%{name}.conf
> ...
> %{_libdir}/%{name}/*.so.*

The directories

  %{_sysconfdir}/%{name}/
  %{_libdir}/%{name}/

are not included in the package.
https://fedoraproject.org/wiki/Packaging:UnownedDirectories

> %files devel
> ...
> %{_datadir}/%{name}/m4/

Here, %_datadir/name is not included. The -examples and -validation subpackages also depend on that directory.

> %doc AUTHORS COPYING README

In all four packages? Bad choice. Including these %doc files in just the main library package is sufficient.

Comment 4 Michel Zou 2011-03-07 15:25:01 UTC
Hi,

I fixde all these problems :
https://build.opensuse.org/package/view_file?file=openturns.spec&package=openturns&project=science%3Aopenturns

Regards.

Comment 5 Jason Tibbitts 2012-04-25 03:10:17 UTC
Could we get an updated package?  All I see in comment 4 is the spec file, and that spec file is going to need a good bit of editing before it's acceptable for Fedora.

Comment 6 Michel Zou 2012-04-25 06:27:44 UTC
Hi,

The latest version is still available here:
https://build.opensuse.org/package/view_file?file=openturns.spec&package=openturns&project=science%3Aopenturns



As this package is meant to build on rhel, suse, centos, mdv, and fedora
The are several non-fedora specific dependencies like this:

%if 0%{?rhel_version} || 0%{?centos_version}
BuildRequires:  blas-devel
%endif

I don't know if it is acceptable as it is, or if you'd want to remove that.



Anyway, I'm open to suggestions, and don't hesitate for further questions !

Regards,

J.

Comment 7 Jason Tibbitts 2012-04-25 14:24:23 UTC
First off, if you really want reviewers to look at your package, please provide:

A link that when clicked gives the spec file.  Just the spec file; no fancy stuff.
A link that when clicked gives the src.rpm.  No fancy downloaders or anything; just the src.rpm saved to disk.

We have a lot of packages to look at, and if you make it difficult to get the stuff then people will just pass your ticket by (as they have for some time).  There are automated tools which some people use to do reviews, and if you make them not work then you're missing out on potential reviewers.

Now, all of the non Fedora (or EPEL) specific stuff is indeed impermissible in Fedora; we strive for the simplest spec files possible.  In addition, %rhel_version and %centos_version are not used (nor even defined) in Fedora.  See http://fedoraproject.org/wiki/Packaging:DistTag#Conditionals

Have you read our Packaging guidelines and documentation on becoming a packager?  https://fedoraproject.org/wiki/Packaging:Guidelines
https://fedoraproject.org/wiki/PackageMaintainers/Join

Comment 8 Michel Zou 2012-04-25 15:29:17 UTC
Hi,

I uploaded the spec file here:
https://trac.assembla.com/c2TSUCxoGr3On6eJe5afGb/export/2/trunk/fedora/openturns.spec

And the source rpm here:
https://trac.assembla.com/c2TSUCxoGr3On6eJe5afGb/export/2/trunk/fedora/openturns-1.0.src.rpm

Yes, that documentation sounds familiar.

J.

Comment 9 Michael Schwendt 2012-04-26 19:02:24 UTC
To be fair, let me leave a notification that I likely won't return to this ticket. Sorry!

* Issues mentioned in comment 3 are still present, have not been commented on, and have not been commented on in the spec %changelog either. For example, explicit Requires deserve to be commented on in the spec file. To follow the guidelines and to document the inter-package dependencies, too.

* The dist-independent conditional spec file sections are a borderline case. Not only is the spec file hardly readable during review because of the way the conditional sections are nested, I also question that it is readable/maintainable to the package maintainer(s).

* Random comments:

> Patch0:         %{name}-%{version}-bug_471.patch

It's really _much_ more clever to not use such macros in patch file names, because the patch may still apply cleanly when %version changes slightly. You wouldn't want to be forced to rename committed patch files in such a case.

> %package -n lib%{name}0

Dangerous, probably over-ambitious. I assume you're aiming at multiple parallel-installable lib%{name}N packages. But that mean you would have more work changing the package name when the library SONAME version increases (especially with explicit dependencies in the lib%{name}0 package). A superior way would be to use

  %package libs

to create the %{name}-libs subpackage which could move forward freely and independent of any compatibility lib%{name}N packages built from a separate src.rpm.

> %post -p /sbin/ldconfig
> %postun -p /sbin/ldconfig

These two can't be true, because the base package contains just %doc files, and not even any of the subpackages stores shared libs in run-time linker's search path.