This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours

Bug 908088 (ascend)

Summary: Review Request: ascend - ASCEND modelling environment
Product: [Fedora] Fedora Reporter: Antonio Trande <anto.trande>
Component: Package ReviewAssignee: Rex Dieter <rdieter>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: anto.trande, package-review, rdieter
Target Milestone: ---Flags: rdieter: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: ascend-0.9.8-6.20140211svn4638.fc20 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2014-06-14 21:50:49 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Bug Depends On: 908089, 908090, 920518    
Bug Blocks:    

Description Antonio Trande 2013-02-05 15:36:13 EST
Spec URL: http://sagitter.fedorapeople.org/ascend/ascend.spec
SRPM URL: http://sagitter.fedorapeople.org/ascend/ascend-0.9.8-2.fc18.src.rpm

Description: ASCEND IV is both a large-scale object-oriented mathematical
modeling environment and a strongly typed mathematical modeling
language. Although ASCEND has primarily been developed by Chemical
Engineers, great care has been exercised to assure that it is
domain independent. ASCEND can support modeling activities in
fields from Architecture to (computational) Zoology.

Fedora Account System Username: sagitter

This package depends from 'sundials solver' (> 2.4.0) already available in Fedora but not to the latest version yet.
Comment 1 Antonio Trande 2013-03-02 18:32:10 EST
ipopt dependency changed to coin-or-Ipopt.
.c/.h files rearranged to be all included in devel package.  

Spec URL: http://sagitter.fedorapeople.org/ascend/ascend.spec
SRPM URL: http://sagitter.fedorapeople.org/ascend/ascend-0.9.8-3.fc18.src.rpm
Comment 2 Antonio Trande 2013-03-13 11:36:48 EDT
doc sub-package changed to noarch.

Spec URL: http://sagitter.fedorapeople.org/ascend/ascend.spec
SRPM URL: http://sagitter.fedorapeople.org/ascend/ascend-0.9.8-4.fc18.src.rpm
Comment 3 Antonio Trande 2013-05-12 13:47:24 EDT
'xgraph' Requires removed.

Spec URL: http://sagitter.fedorapeople.org/ascend/ascend.spec
SRPM URL: http://sagitter.fedorapeople.org/ascend/ascend-0.9.8-5.fc18.src.rpm
Comment 4 Antonio Trande 2013-12-02 13:04:23 EST
The development goes slowly and there are some things to fix.
Comment 5 Antonio Trande 2014-02-15 11:34:40 EST
Last changes:

- Update to svn4638
- 'data' sub-package is now arched

It seems that it's not possible define noarch the 'data' package.
I obtained 
BuildError: mismatch when analyzing ascend-data-0.9.8-1.20140211svn4638.fc21.noarch.rpm 
http://koji.fedoraproject.org/koji/taskinfo?taskID=6522713

- Built a 'data' sub-package
- Removed Tcl/Tk subpackage building because of missing dependencies
  (tktable) 
- Defined two macros for the packaging
- Defined the scriplets in %%post, %%postun, %%posttrans
- Patching compiler flags
- Added CUnit-devel BR
- IDA solver excluded

Spec URL: http://sagitter.fedorapeople.org/ascend/ascend.spec
SRPM URL: http://sagitter.fedorapeople.org/ascend/ascend-0.9.8-2.20140211svn4638.fc20.src.rpm
Koji build in rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=6522934
Comment 6 Rex Dieter 2014-02-18 14:25:30 EST
I'll take a look (thanks for offering to swap reviews!).
Comment 7 Rex Dieter 2014-02-18 14:44:03 EST
A few initial comments:

1.  SHOULD drop deprecated rpm constructs, including Group: tags  (unless you plan on supporting building on/for epel-5)


2.  SHOULD give script or detailed instructions on how to (re)generate Source0 archive


3. MUST use
BuildRequires: python2-devel
and associated macros %{python2_sitelib} %{python2_sitearch} macros per
https://fedoraproject.org/wiki/Packaging:Python


4.  MUST drop unwanted deps
Requires(post): desktop-file-utils shared-mime-info
Requires(postun): desktop-file-utils shared-mime-info
see also:
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#mimeinfo


5.  MUST fix icon scriptlets.  
stuff referencing %{_datadir}/icons/hicolor is ok, but not %{_datadir}/icons.  in short, drop the lines:
touch --no-create %{_datadir}/icons &>/dev/null
gtk-update-icon-cache %{_datadir}/icons &>/dev/null || :
gtk-update-icon-cache %{_datadir}/icons &>/dev/null || :


6.  MUST review/fix folder ownership.  Offhand, it would appear nothing owns
%{models}/  (expands to %{_libdir}/ascend/models)
%{models}/sensitivity/
%{johnpye}/  (expands to %{_libdir}/ascend/models/johnpye)
%{johnpye}/grena/
%{johnpye}/extpy/
%{johnpye}/extfn/
%{johnpye}/datareader/
...
you should get the idea.


7. SHOULD consider a -libs subpkg for 
%{_libdir}/libascend.so.*
This is mostly to be multilib friendly, as anything matching %{_libdir}/lib*.so.* will get multilib'd automatically (e.g. pkg.i686 will get pulled into x86_64 repo).
Comment 8 Antonio Trande 2014-02-18 16:42:11 EST
(In reply to Rex Dieter from comment #7)
> A few initial comments:
> 
> 1.  SHOULD drop deprecated rpm constructs, including Group: tags  (unless
> you plan on supporting building on/for epel-5)
> 
> 
> 2.  SHOULD give script or detailed instructions on how to (re)generate
> Source0 archive
> 
> 
> 3. MUST use
> BuildRequires: python2-devel
> and associated macros %{python2_sitelib} %{python2_sitearch} macros per
> https://fedoraproject.org/wiki/Packaging:Python
> 
> 
> 4.  MUST drop unwanted deps
> Requires(post): desktop-file-utils shared-mime-info
> Requires(postun): desktop-file-utils shared-mime-info
> see also:
> https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database
> https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#mimeinfo

Well, this package was also created for RHEL5, but now needs packages which are not in EPEL.
Fixed.

> 
> 
> 5.  MUST fix icon scriptlets.  
> stuff referencing %{_datadir}/icons/hicolor is ok, but not
> %{_datadir}/icons.  in short, drop the lines:
> touch --no-create %{_datadir}/icons &>/dev/null
> gtk-update-icon-cache %{_datadir}/icons &>/dev/null || :
> gtk-update-icon-cache %{_datadir}/icons &>/dev/null || :

Fixed.

> 
> 
> 6.  MUST review/fix folder ownership.  Offhand, it would appear nothing owns
> %{models}/  (expands to %{_libdir}/ascend/models)
> %{models}/sensitivity/
> %{johnpye}/  (expands to %{_libdir}/ascend/models/johnpye)
> %{johnpye}/grena/
> %{johnpye}/extpy/
> %{johnpye}/extfn/
> %{johnpye}/datareader/
> ...
> you should get the idea.

Ops! Important inattention.
Fixed.

> 
> 
> 7. SHOULD consider a -libs subpkg for 
> %{_libdir}/libascend.so.*
> This is mostly to be multilib friendly, as anything matching
> %{_libdir}/lib*.so.* will get multilib'd automatically (e.g. pkg.i686 will
> get pulled into x86_64 repo).

All libraries packaged in a -libs sub-package.

Spec URL: http://sagitter.fedorapeople.org/ascend/ascend.spec
SRPM URL: http://sagitter.fedorapeople.org/ascend/ascend-0.9.8-3.20140211svn4638.fc20.src.rpm
Comment 9 Antonio Trande 2014-02-22 11:07:20 EST
Ascend may provide a Tcl/Tk user interface by the tktable. The latter package was packaged in Fedora until two years ago.

Recently (May of 2013), it seems to have been resumed: http://sourceforge.net/projects/tktable/.
Build: http://koji.fedoraproject.org/koji/taskinfo?taskID=6559860

It must be reviewed again. What do you think ?
Comment 10 Antonio Trande 2014-02-24 16:54:37 EST
Spec URL: http://sagitter.fedorapeople.org/ascend/ascend.spec
SRPM URL: http://sagitter.fedorapeople.org/ascend/ascend-0.9.8-4.20140211svn4638.fc20.src.rpm

- Excluded CMSLV solver
- Included IDA solver option with patch
Comment 11 Rex Dieter 2014-05-27 20:48:44 EDT
My apologies for failing to notice your updating this... I'll try to finish this up by tomorrow.
Comment 12 Rex Dieter 2014-05-28 09:22:50 EDT
License: OK

Item 3 above not completely fixed, there's still one reference to 
%{python_sitearch}

8.  -libs subpkg MUST include ldconfig scriptlets (and now main pkg no longer needs them)

9.  nothing depends on new -libs subpkg.  You probably want to add a dependency to at least the main and -devel subpkgs:
Requires: %{name}-libs%{?_isa} = %{version}-%{release}

(and have -devel drop dep on main pkg).

While we're at it, make -libs, -data dep on main pkg *not* use %_isa (else you lose the whole point of making a -libs subpkg to be multilib -friendly)

10.  nothing depends on -data subpkg, should main pkg depend on it?  Or is it optional addon?

11. While on the topic of -libs, I'd recommend it *only* include the shared library here, plus any essential resources the library may require.  Is there a reason you added all that other stuff too?

12.  MUST omit extraneous python dep
Requires: python >= 2.4
https://fedoraproject.org/wiki/Packaging:Python#Multiple_Python_Runtimes
(it'll get an automatic dependency on python(abi) instead)

13. MUST omit gcc-c++ dependency,
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Exceptions_2

14. URL: http://ascend.cheme.cmu.edu/
seems inaccessible (at the moment?).  Googling found me http://ascend4.org/ too, but it's not working either.  Please verify which URL is correct.
Comment 13 Antonio Trande 2014-05-28 14:22:40 EDT
(In reply to Rex Dieter from comment #12)
> License: OK
> 
> Item 3 above not completely fixed, there's still one reference to 
> %{python_sitearch}

Done.
> 
> 8.  -libs subpkg MUST include ldconfig scriptlets (and now main pkg no
> longer needs them)

Done.
> 
> 9.  nothing depends on new -libs subpkg.  You probably want to add a
> dependency to at least the main and -devel subpkgs:
> Requires: %{name}-libs%{?_isa} = %{version}-%{release}
> 
> (and have -devel drop dep on main pkg).
> 
> While we're at it, make -libs, -data dep on main pkg *not* use %_isa (else
> you lose the whole point of making a -libs subpkg to be multilib -friendly)

Okay.

> 
> 10.  nothing depends on -data subpkg, should main pkg depend on it?  Or is
> it optional addon?

Honestly, I had wanted to split all files in a better way depending on their usage. Effectively -data files can be useful for testing and directly called by main software.


> 
> 11. While on the topic of -libs, I'd recommend it *only* include the shared
> library here, plus any essential resources the library may require.  Is
> there a reason you added all that other stuff too?

No particular reason, just to keep all libraries in same package.

> 
> 12.  MUST omit extraneous python dep
> Requires: python >= 2.4
> https://fedoraproject.org/wiki/Packaging:Python#Multiple_Python_Runtimes
> (it'll get an automatic dependency on python(abi) instead)
> 
> 13. MUST omit gcc-c++ dependency,
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Exceptions_2
> 
> 14. URL: http://ascend.cheme.cmu.edu/
> seems inaccessible (at the moment?).  Googling found me http://ascend4.org/
> too, but it's not working either.  Please verify which URL is correct.

All surplus coming from old packages.
Fixed.

Spec URL: http://sagitter.fedorapeople.org/ascend/ascend.spec
SRPM URL: http://sagitter.fedorapeople.org/ascend/ascend-0.9.8-5.20140211svn4638.fc20.src.rpm
Comment 14 Rex Dieter 2014-05-30 09:26:08 EDT
item 8 was fixed incorrectly.  -libs scriptlet should be ldconfig *only* the rest of the stuff left in the main pkg scriptlet.  Do this:

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

%post
update-desktop-database &> /dev/null || :
update-mime-database /usr/share/mime &> /dev/null || :
touch --no-create %{_datadir}/icons/hicolor &>/dev/null || :

%postun
update-desktop-database &> /dev/null || :
update-mime-database /usr/share/mime &> /dev/null || :
if [ $1 -eq 0 ] ; then
    touch --no-create %{_datadir}/icons/hicolor &>/dev/null
    gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :
fi

%posttrans
gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :


item 9 is not completely fixed, both -data and -libs still use %{_isa} in dep on main package, which we want to avoid.  In short, switch:
Requires: %{name}%{?_isa} = %{version}-%{release}
to
Requires: %{name} = %{version}-%{release}
Comment 15 Rex Dieter 2014-05-30 09:44:28 EDT
just noticed a typo in my comment about item 8, theres an extra - in there.  use: 
%post libs -p /sbin/ldconfig
%postun libs -p /sbin/ldconfig
...
Comment 16 Antonio Trande 2014-05-30 10:36:07 EDT
(In reply to Rex Dieter from comment #14)
> item 8 was fixed incorrectly.  -libs scriptlet should be ldconfig *only* the
> rest of the stuff left in the main pkg scriptlet.  Do this:
> 
> %post libs -p /sbin/ldconfig
> %postun -libs -p /sbin/ldconfig
> 
> %post
> update-desktop-database &> /dev/null || :
> update-mime-database /usr/share/mime &> /dev/null || :
> touch --no-create %{_datadir}/icons/hicolor &>/dev/null || :
> 
> %postun
> update-desktop-database &> /dev/null || :
> update-mime-database /usr/share/mime &> /dev/null || :
> if [ $1 -eq 0 ] ; then
>     touch --no-create %{_datadir}/icons/hicolor &>/dev/null
>     gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :
> fi
> 
> %posttrans
> gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :
> 
> 
> item 9 is not completely fixed, both -data and -libs still use %{_isa} in
> dep on main package, which we want to avoid.  In short, switch:
> Requires: %{name}%{?_isa} = %{version}-%{release}
> to
> Requires: %{name} = %{version}-%{release}

Done.
I left same release number.

Spec URL: http://sagitter.fedorapeople.org/ascend/ascend.spec
SRPM URL: http://sagitter.fedorapeople.org/ascend/ascend-0.9.8-5.20140211svn4638.fc20.src.rpm
Comment 17 Rex Dieter 2014-05-30 12:28:03 EDT
-data still has:
Requires: %{name}%{?_isa} = %{version}-%{release}
should be:
Requires: %{name} = %{version}-%{release}

-devel has:
Requires: %{name}-libs = %{version}-%{release}
should be:
Requires: %{name}-libs%{?_isa} = %{version}-%{release}


I'm curious now, if there's a circular dependency between the main pkg (Requires: %{name}-data) and -data (Requires: %{name}), why even make a -data subpkg?
Comment 18 Antonio Trande 2014-05-30 12:38:32 EDT
(In reply to Rex Dieter from comment #17)
> -data still has:
> Requires: %{name}%{?_isa} = %{version}-%{release}
> should be:
> Requires: %{name} = %{version}-%{release}
> 
> -devel has:
> Requires: %{name}-libs = %{version}-%{release}
> should be:
> Requires: %{name}-libs%{?_isa} = %{version}-%{release}
> 
> 
> I'm curious now, if there's a circular dependency between the main pkg
> (Requires: %{name}-data) and -data (Requires: %{name}), why even make a
> -data subpkg?

Initially, I prefered to split all files both to put them in order and to organise them in the better manner; even if they may be packaged in a single rpm.
Comment 19 Antonio Trande 2014-05-30 13:17:19 EDT
Briefly:

main rpm
 Requires: %{name}-libs = %{version}-%{release}
 Requires: %{name}-data = %{version}-%{release}

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

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

-data rpm 
 Requires: %{name} = %{version}-%{release}

Is it okay?
Comment 20 Rex Dieter 2014-05-30 14:23:10 EDT
No.  in short, you want all references to the main pkg to *not* use %{_isa} and any reference to -libs or -data to use %{_isa}

These 2 need to be:

main rpm
 Requires: %{name}-libs%{?_isa} = %{version}-%{release}
 Requires: %{name}-data%{?_isa} = %{version}-%{release}

-libs rpm
  Requires: %{name} = %{version}-%{release}
Comment 21 Antonio Trande 2014-05-30 14:28:41 EDT
(In reply to Rex Dieter from comment #20)
> No.  in short, you want all references to the main pkg to *not* use %{_isa}
> and any reference to -libs or -data to use %{_isa}
> 
> 
> -libs rpm
>   Requires: %{name} = %{version}-%{release}

-libs should not consider also the architecture?
Comment 22 Rex Dieter 2014-05-30 15:16:24 EDT
Correct, dependencies should not consider architecture (ie, and use %{_isa}) for packages that are not (or that you don't want to be ) multilib'd.  In this case, we do not want the main package to be multilib'd.

Otherwise, there is no point in making a -libs subpkg at all.
Comment 24 Rex Dieter 2014-06-02 14:56:11 EDT
OK, looks like we have a winner, APPROVED.
Comment 25 Antonio Trande 2014-06-02 15:13:49 EDT
Thank you very much Rex.

New Package SCM Request
=======================
Package Name: ascend
Short Description:  ASCEND modelling environment
Upstream URL: http://ascend4.org/
Owners: sagitter
Branches: f20
Comment 26 Jon Ciesla 2014-06-03 08:22:43 EDT
Git done (by process-git-requests).
Comment 27 Fedora Update System 2014-06-03 17:09:26 EDT
ascend-0.9.8-6.20140211svn4638.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/ascend-0.9.8-6.20140211svn4638.fc20
Comment 28 Fedora Update System 2014-06-04 03:55:02 EDT
Package ascend-0.9.8-6.20140211svn4638.fc20:
* should fix your issue,
* was pushed to the Fedora 20 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing ascend-0.9.8-6.20140211svn4638.fc20'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2014-7058/ascend-0.9.8-6.20140211svn4638.fc20
then log in and leave karma (feedback).
Comment 29 Fedora Update System 2014-06-14 21:50:49 EDT
ascend-0.9.8-6.20140211svn4638.fc20 has been pushed to the Fedora 20 stable repository.  If problems still persist, please make note of it in this bug report.