Bug 1069257 - Review Request: fparser - Function parser library for C++
Review Request: fparser - Function parser library for C++
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
: 814458 (view as bug list)
Depends On:
Blocks: 674008
  Show dependency treegraph
 
Reported: 2014-02-24 10:26 EST by Till Hofmann
Modified: 2014-04-04 05:46 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-03-18 12:51:48 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bugs.michael: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
SPEC file (1.83 KB, text/x-rpm-spec)
2014-02-24 10:26 EST, Till Hofmann
no flags Details
SRPM (168.37 KB, application/x-rpm)
2014-02-24 10:27 EST, Till Hofmann
no flags Details
cmake patch (42.26 KB, patch)
2014-02-24 10:28 EST, Till Hofmann
no flags Details | Diff
basic autotools files for building libfparser (10.00 KB, application/x-tar)
2014-03-19 07:12 EDT, Michael Schwendt
no flags Details

  None (edit)
Description Till Hofmann 2014-02-24 10:26:13 EST
Created attachment 866996 [details]
SPEC file

This C++ library offers a class which can be used to parse and evaluate a
mathematical function from a string (which might be for example requested
from the user). The syntax of the function string is similar to
mathematical expressions written in C/C++.

This request is based on a stalled Review Request for fparser (#814458). I've updated the SPEC file to the latest fparser version and included a patch to provide a cmake build system. As mentioned in the other request, upstream is unwillling to provide a build system or include the patch, therefore the patch is Fedora-only.
Comment 1 Till Hofmann 2014-02-24 10:27:10 EST
Created attachment 866997 [details]
SRPM
Comment 2 Till Hofmann 2014-02-24 10:28:05 EST
Created attachment 866998 [details]
cmake patch
Comment 3 Rich Mattes 2014-02-24 11:50:18 EST
*** Bug 814458 has been marked as a duplicate of this bug. ***
Comment 5 Susi Lehtola 2014-02-26 07:30:37 EST
If you aren't a packager yet, you need to
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers
Comment 6 Michael Schwendt 2014-02-26 16:32:37 EST
> License:        LGPLv3

That's what some of the source file headers say. But both the LGPL and GPL files are included:

  | %files
  | %doc docs/gpl.txt docs/lgpl.txt

The spec file doesn't comment on that. What's the full story here? Including the GPLv3 with the package only adds confusion, IMO.


> %package        devel
> …
> Requires:       %{name} = %{version}-%{release}

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


> %package        devel
> …
> Requires:       cmake, mpfr-devel, gpm-devel

It's a good habit to add comments to explicit dependencies. Here the dependency on "cmake" raises the question why it has been added? Not everyone uses CMake. If the dep has been added only for directory ownership, prefer:

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


> %install
> rm -rf $RPM_BUILD_ROOT

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


> %files devel
> …
> %{_libdir}/cmake/%{name}-%{version}/*

This would mean that the directory %{_libdir}/cmake/%{name}-%{version} is not included (aka being an "unowned" directory issue):

  https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
  https://fedoraproject.org/wiki/Packaging:UnownedDirectories


> -shared -Wl,-soname,libfparser4.5.1.so.4.5.1 -o libfparser4.5.1.so.4.5.1

Uh oh! That's a very fragile SONAME, which will require rebuilds of dependencies for all minor version updates.
Comment 7 Till Hofmann 2014-02-27 05:22:27 EST
Thanks for the hints and sorry for not following the guidelines.

Since I forgot to state it in the description: This is my first package and I'm seeking a sponsor.

There is a successful koji build for this package: http://koji.fedoraproject.org/koji/taskinfo?taskID=6576009

Is it OK to upload the SPEC and SRPM as attachment or should I rather put them somewhere else (e.g. fedorapeople.org)?

@Michael: Thanks for the first check, I'll update the SPEC file accordingly.
Comment 8 Till Hofmann 2014-02-27 05:38:13 EST
Koji build for rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=6576080
Comment 9 Susi Lehtola 2014-02-27 08:29:42 EST
(In reply to Till Hofmann from comment #7)
> Thanks for the hints and sorry for not following the guidelines.
>  
> Is it OK to upload the SPEC and SRPM as attachment or should I rather put
> them somewhere else (e.g. fedorapeople.org)?

(In reply to Susi Lehtola from comment #4)
> Please follow
> https://fedoraproject.org/wiki/New_package_process_for_existing_contributors

step 3
Comment 10 Till Hofmann 2014-02-27 11:47:27 EST
(In reply to Michael Schwendt from comment #6)
> 
>   | %files
>   | %doc docs/gpl.txt docs/lgpl.txt
> 
> The spec file doesn't comment on that. What's the full story here? Including
> the GPLv3 with the package only adds confusion, IMO.
> 

I'm not sure why upstream includes both license files, but the headers reference both, too. I could just remove gpl.txt since it's not really used, I just figured I'd put it in the package since they reference it in the headers.

> 
> > %package        devel
> > …
> > Requires:       %{name} = %{version}-%{release}
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package
> 

fixed

> 
> > %package        devel
> > …
> > Requires:       cmake, mpfr-devel, gpm-devel
> 
> It's a good habit to add comments to explicit dependencies. Here the
> dependency on "cmake" raises the question why it has been added? Not
> everyone uses CMake. 

I've removed cmake and added comments to the other two. I also fixed a typo, gpm-devel should be gmp-devel. MPFR and GMP are not necessary but provide extra features, so I'm not sure if I should include them as requirement? 
From their website:
> Different numerical types are supported: double, float, long double, long int,
> std::complex (of types double, float and long double), multiple-precision 
> floating point numbers using the MPFR library, and arbitrary precision 
> integers using the GMP library. (Note that it's not necessary for these two 
> libraries to exist in the system in order to use the Function Parser library 
> with the other numerical types. Support for these libraries is optionally 
> compiled in using preprocessor settings.)
> 

> > %install
> > rm -rf $RPM_BUILD_ROOT
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

fixed

> 
> 
> > %files devel
> > …
> > %{_libdir}/cmake/%{name}-%{version}/*
> 
> This would mean that the directory %{_libdir}/cmake/%{name}-%{version} is
> not included (aka being an "unowned" directory issue):

fixed

> 
> 
> > -shared -Wl,-soname,libfparser4.5.1.so.4.5.1 -o libfparser4.5.1.so.4.5.1
> 
> Uh oh! That's a very fragile SONAME, which will require rebuilds of
> dependencies for all minor version updates.

You're right. I'm not sure what a good solution would look like, I've stripped all versions from the file names (e.g. the cmake files are now in %{_libdir}/cmake/fparser) and set 4.5 as SO version, which results in libfparser.4.5 .
Comment 11 Till Hofmann 2014-02-27 12:02:06 EST
(In reply to Susi Lehtola from comment #9)
> (In reply to Till Hofmann from comment #7)
> > Thanks for the hints and sorry for not following the guidelines.
> >  
> > Is it OK to upload the SPEC and SRPM as attachment or should I rather put
> > them somewhere else (e.g. fedorapeople.org)?
> 
> (In reply to Susi Lehtola from comment #4)
> > Please follow
> > https://fedoraproject.org/wiki/New_package_process_for_existing_contributors
> 
> step 3

Well I wasn't sure if an attachment of a bug report would count as 'somewhere on the internet', thank you for the clarification. I'm currently waiting to get space on fedorapeople.org, but I've uploaded the files somewhere else:

SRPM: http://halnt.dyndns.org/till/fedora/fparser/fparser-4.5.1-2.fc20.src.rpm
SPEC: http://halnt.dyndns.org/till/fedora/fparser/fparser.spec
cmake patch: http://halnt.dyndns.org/till/fedora/fparser/fparser.cmake.patch
Comment 13 Michael Schwendt 2014-03-06 14:55:22 EST
> I'm not sure why upstream includes both license files,
> but the headers reference both, too.

Asking upstream for clarification would be an idea (since LGPL is less restrictive than GPL):

  https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Clarification


> MPFR and GMP are not necessary but provide extra features, so I'm not
> sure if I should include them as requirement? 

As a rule of thumb for -devel package "Requires" one could say: (1) If *any* of the headers within fparser-devel include headers from a different -devel package, that one ought to be added as a "Requires". The rationale would simply be that compiling with fparser-devel should never fail because of missing headers. (2) If headers/files from a different -devel package are fully optional, the app that uses the fparser API and those other -devel packages should add those -devel packages as Requires instead. That way the API users really need to depend on what they use actually instead of you trying to guess what they may want (or not).
Comment 14 Till Hofmann 2014-03-11 06:51:55 EDT
(In reply to Michael Schwendt from comment #13)
> 
> > MPFR and GMP are not necessary but provide extra features, so I'm not
> > sure if I should include them as requirement? 
> 
> As a rule of thumb for -devel package "Requires" one could say: (1) If *any*
> of the headers within fparser-devel include headers from a different -devel
> package, that one ought to be added as a "Requires". The rationale would
> simply be that compiling with fparser-devel should never fail because of
> missing headers. 

fparser_gmpint.hh includes files from gmp-devel, but clearly fparser_gmpint.hh is used only if the GMP Parser is to be used (similarly for MPFR). There are other parsers available which don't require GMP or MPFR. But GMP and MPFR support is determined at compile time, so if we don't add the dependency, there is no way we can provide GMP or MPFR support (see http://warp.povusers.org/FunctionParser/fparser.html#parsertypes ). Now I'm not sure what to do, either we force everyone to install mpfr-devel and gmp-devel or we omit the support for both completely. What do you think?

I've asked upstream about Licensing and I got the following answer:

> The reason is because of this clause at the beginning of the LGPL licence:
> 
> "This version of the GNU Lesser General Public License incorporates
> the terms and conditions of version 3 of the GNU General Public
> License, supplemented by the additional permissions listed below."
>
> In other words, LGPL is GPL plus the additions mentioned in the LGPL
> document. Thus you can't get the whole of the LGPL text if you don't
> include the GPL text as well.

This absolutely makes sense but it should apply to all LGPL-licensed packages, how is this usually handled?
Comment 15 Michael Schwendt 2014-03-11 09:54:03 EDT
According to the FAQ both license texts need to be included if it's the LGPL v3:

  https://www.gnu.org/licenses/gpl-faq.html#v3HowToUpgrade

Since probably not all developers are aware of that, once a project is found that only includes the LGPL v3 terms, a bug report would be justified. Sort of following https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

[...]

> fparser_gmpint.hh includes files from gmp-devel,

Can't confirm. I only see subdir includes and C++ stdlib:

$ grep \#include fparser/* -R
fparser/fparser_gmpint.hh:#include "fparser.hh"
fparser/fparser_gmpint.hh:#include "mpfr/GmpInt.hh"
fparser/fparser.hh:#include <string>
fparser/fparser.hh:#include <vector>
fparser/fparser.hh:#include <iostream>
fparser/fparser.hh:#include <complex>
fparser/fparser_mpfr.hh:#include "fparser.hh"
fparser/fparser_mpfr.hh:#include "mpfr/MpfrFloat.hh"
fparser/fpaux.hh:#include "fptypes.hh"
fparser/fpaux.hh:#include <cmath>
fparser/fpaux.hh:#include "mpfr/MpfrFloat.hh"
fparser/fpaux.hh:#include "mpfr/GmpInt.hh"
fparser/fpaux.hh:#include <complex>
fparser/fptypes.hh:#include "fpconfig.hh"
fparser/fptypes.hh:#include <cstring>
fparser/fptypes.hh:#include <map>
fparser/fptypes.hh:#include <vector>
fparser/fptypes.hh://#include "fpaux.hh"
fparser/mpfr/GmpInt.hh:#include <iostream>
fparser/mpfr/MpfrFloat.hh:#include <iostream>


> What do you think?

It's not a big issue. Either add the Requires or not would work. In my opinion, currently it would be cleaner, if no such dependencies were added.
Comment 16 Till Hofmann 2014-03-11 10:44:43 EDT
(In reply to Michael Schwendt from comment #15)
> It's not a big issue. Either add the Requires or not would work. In my
> opinion, currently it would be cleaner, if no such dependencies were added.

I've removed the dependencies. I've updated the SPEC file in-place, the new SRPM can be found at http://thofmann.fedorapeople.org/fparser-4.5.1-3.fc20.src.rpm. A koji build can be found at http://koji.fedoraproject.org/koji/taskinfo?taskID=6621823
Comment 17 Michael Schwendt 2014-03-14 19:40:38 EDT
An update:

 * Debian includes an older 4.3 version of the library. https://packages.debian.org/squeeze-backports/libfparser-4.3

 * They have chosen to patch it as well, but differently. They add an Autotools based configure script and include a pkgconfig file in their -dev package.

 * Their shared lib is named libfparser-4.3.so (and libfparser.so in the devel package for build-time linking).

Depending on which other projects rely on building with the pkgconfig file (or, in the future, your CMake files), it may become necessary to join forces and agree on a compatible build framework (even if upstream does not want to include those files [yet], maybe not even in a contrib folder).

  $ rpm -qp --provides fparser-4.5.1-3.fc20.x86_64.rpm |grep ^l
  libfparser.so.4.5()(64bit)

Your libfparser.so.4.5 SONAME currently differs from Debian's library naming scheme. That's a binary incompatibility to think about as well.

Inventing library sonames at the distribution level (without support from upstream) is non-trivial almost always. Skimming over the fparser changelog/news it's hard for me to predict how compatible the individual minor releases have been and will be. Especially the burden of checking releases for ABI compatibility (and updating the SONAME version appropriately) will be on your shoulders (i.e. the packager's shoulders).
Debian's libfoo-$major.$minor.so naming is a slightly better compromise, because it is explicit about $major.$minor being the product release version and not being a varying library interface version.

Thoughts?


> %{_libdir}/cmake/*

When not adding "Requires: cmake" the directory needs to be included in the package:

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


I think there's nothing else in the spec/packaging that needs a fix during review.
Comment 18 Till Hofmann 2014-03-17 07:30:56 EDT
(In reply to Michael Schwendt from comment #17)

> They have chosen to patch it as well, but differently. They add an Autotools > based configure script and include a pkgconfig file in their -dev package.

Do you think we should adopt their patches or can we stay with cmake?

> Depending on which other projects rely on building with the pkgconfig file
> (or, in the future, your CMake files), it may become necessary to join
> forces and agree on a compatible build framework (even if upstream does not
> want to include those files [yet], maybe not even in a contrib folder).
> 
>   $ rpm -qp --provides fparser-4.5.1-3.fc20.x86_64.rpm |grep ^l
>   libfparser.so.4.5()(64bit)
> 
> Your libfparser.so.4.5 SONAME currently differs from Debian's library naming
> scheme. That's a binary incompatibility to think about as well.
> 
> Inventing library sonames at the distribution level (without support from
> upstream) is non-trivial almost always. Skimming over the fparser
> changelog/news it's hard for me to predict how compatible the individual
> minor releases have been and will be. Especially the burden of checking
> releases for ABI compatibility (and updating the SONAME version
> appropriately) will be on your shoulders (i.e. the packager's shoulders).
> Debian's libfoo-$major.$minor.so naming is a slightly better compromise,
> because it is explicit about $major.$minor being the product release version
> and not being a varying library interface version.
> 
> Thoughts?

Seems reasonable. From the changelog it seems like any patch version update is compatible to the previous version while all minor version updates may change the ABI, but I guess we shouldn't rely on that. I've adopted the changes you suggested. 
SPEC file: http://thofmann.fedorapeople.org/fparser.spec
cmake patch: http://thofmann.fedorapeople.org/fparser.cmake.patch
SRPM: http://thofmann.fedorapeople.org/fparser-4.5.1-4.fc20.src.rpm

I don't know much about cmake so I'm not sure if the way I added the symlink libfparser.so -> libfparser-4.5.so is the proper way to do so.

> > %{_libdir}/cmake/*
> 
> When not adding "Requires: cmake" the directory needs to be included in the
> package

Fixed
Comment 19 Michael Schwendt 2014-03-17 15:36:14 EDT
> Do you think we should adopt their patches or can we stay with cmake?

For building fparser, it doesn't matter. Use what works for you. ;)

For the fparser-devel package contents, including a pkgconfig file would add some convenience for developers, who don't use CMake and don't want to use fparser as a copylib.

Personally, I don't know whether it is worthwhile to maintain the >43 KB patch for cmake support.


> the symlink libfparser.so -> libfparser-4.5.so 

It's correct. The libfparser.so file makes -lfparser work when compiling/linking.

But the new package release has removed the library SONAME. See output from "fedora-review -b 1069257" in case you haven't tried out that tool before.

  $ rpmlint SRPM/fparser-4.5.1-4.fc20.src.rpm x86_64/*
  fparser.x86_64: W: no-soname /usr/lib64/libfparser-4.5.so


If you feel confident about fixing this in dist git, go ahead. The spec file meets Fedora's requirements for a basic library package. There's a typo in the last %changelog entry, btw.

APPROVED
Comment 20 Till Hofmann 2014-03-18 10:59:17 EDT
(In reply to Michael Schwendt from comment #19)

> 
> But the new package release has removed the library SONAME. See output from
> "fedora-review -b 1069257" in case you haven't tried out that tool before.
> 
>   $ rpmlint SRPM/fparser-4.5.1-4.fc20.src.rpm x86_64/*
>   fparser.x86_64: W: no-soname /usr/lib64/libfparser-4.5.so

I actually thought that was what you suggested, I didn't run rpmlint again. I'll re-add the soname in dist git. 

> There's a typo in the last %changelog entry, btw.

I can't find a typo, what do you mean?


Thanks for reviewing!
Comment 21 Till Hofmann 2014-03-18 11:02:29 EDT
New Package SCM Request
=======================
Package Name: fparser
Short Description: Function parser library for C++
Owners: thofmann
Branches: f20
InitialCC:
Comment 22 Jon Ciesla 2014-03-18 11:19:28 EDT
Git done (by process-git-requests).
Comment 23 Michael Schwendt 2014-03-18 12:54:28 EDT
Without the SONAME rpmbuild could not detect library dependencies in programs linked with libfparser.


> I can't find a typo, what do you mean?

- Change library naming to fparser-$major-$minor.so

1) it's $major.$minor.so
2) it's libfparser-$major.$minor.so

;-)
Comment 24 Fedora Update System 2014-03-18 12:57:08 EDT
fparser-4.5.1-5.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/fparser-4.5.1-5.fc20
Comment 25 Till Hofmann 2014-03-18 13:12:36 EDT
(In reply to Michael Schwendt from comment #23)
> - Change library naming to fparser-$major-$minor.so
> 
> 1) it's $major.$minor.so
> 2) it's libfparser-$major.$minor.so
> 
> ;-)

Oh, thanks! I definitely didn't see that.

I've re-added the SONAME before I submitted the package, so the built packages already contain the SONAME:
% rpm -qlp RPMS/x86_64/fparser-4.5.1-5.fc20.x86_64.rpm | grep so
/usr/lib64/libfparser-4.5.so.4.5
/usr/lib64/libfparser-4.5.so.4.5.1

% rpm -qlp RPMS/x86_64/fparser-devel-4.5.1-5.fc20.x86_64.rpm | grep so
/usr/lib64/libfparser-4.5.so
/usr/lib64/libfparser.so
Comment 26 Michael Schwendt 2014-03-18 13:18:05 EDT
> /usr/lib64/libfparser-4.5.so.4.5

That's the strange one with the duplicate version again:

  $ rpmsoname ~/Downloads/fparser-4.5.1-5.fc21.x86_64.rpm 
  /usr/lib64/libfparser-4.5.so.4.5.1	libfparser-4.5.so.4.5

  $ eu-readelf -d libfparser-4.5.so.4.5.1 |grep SON
    SONAME            Library soname: [libfparser-4.5.so.4.5]

Is it because of CMake messing it up?

Just "libfparser-4.5.so" as the SONAME should suffice and match the file name and also match Debian's libfparser-4.3.so naming scheme.
Comment 27 Till Hofmann 2014-03-19 05:58:55 EDT
(In reply to Michael Schwendt from comment #26)
> 
> Is it because of CMake messing it up?
> 
> Just "libfparser-4.5.so" as the SONAME should suffice and match the file
> name and also match Debian's libfparser-4.3.so naming scheme.

Hm, I'm not sure this is possible with cmake. If I don't set NO_SONAME and set SOVERSION to "", I get 

$ eu-readelf -d libfparser-4.5.so | grep SONAME
  SONAME            Library soname: [libfparser-4.5.so.]

because cmake automatically generates the prefix 'libfparser-4.5.so.' and appends the SOVERSION. Also, I don't know if I can set the SONAME manually, I didn't find a way to do so.

An easy solution would be to set the SOVERSION to 0, resulting in a SONAME 'libfparser-4.5.so.0', but then this would be incompatible with the debian SONAME. Also, this SONAME is probably more confusing than 'libfparser-4.5.so.4.5'


Since we've already talked about switching to autotools, I'll have a look at the debian patch, maybe it's easy to adopt.
Comment 28 Michael Schwendt 2014-03-19 07:12:14 EDT
Created attachment 876324 [details]
basic autotools files for building libfparser

"autoreconf -f -i" to get going
Comment 29 Till Hofmann 2014-03-19 13:04:47 EDT
Thanks for the patch!

I've included the patch and updated the SPEC file, could you check it before I update the package?
SRPM: http://thofmann.fedorapeople.org/fparser-4.5.1-6.fc20.src.rpm
SPEC: http://thofmann.fedorapeople.org/fparser.spec
patch: http://thofmann.fedorapeople.org/fparser.autotools.patch

I wasn't sure whether to include the autotools files or only the generated files. In https://fedoraproject.org/wiki/PackagingDrafts/AutoConf, it is recommended to only include the generated files, but in the corresponding discussion, many people disagreed. Since we maintain the autotools files ourselves, we shouldn't get into trouble with incompatible autotools versions. Following the suggestions in the discussion, I included the autoconf files and changed the SPEC file appropiately.
Comment 30 Michael Schwendt 2014-03-19 15:57:41 EDT
Hmmm... I'm not so sure anymore whether it has been a good idea to approve the package.

The attached patch is just an example of a basic framework to get started. It does not use all source files from the fparser-4.5.1.zip archive. For example, if you wanted to add the optional gmp/mpfr features, the patch would need to be adjusted to compile the extra sources into the library *and* install the extra headers, too.

That's a big of a chicken'n'egg problem. Once you decide on which features to include in the library, they cannot simply be turned on/off in the same way as when using fparser as a copylib (with modifications to fpconfig.hh possibly).

Oh, and after a second look, only the header file fparser.hh is needed in the fparser-devel package. fpaux.hh, fptypes.h and fpconfig.h are internal to the lib and not included by fparser.hh.


> I wasn't sure whether to include the autotools files or only the generated files.

Regenerating them at the end of %prep (not the start of %build) is fine. The topic of including pregenerated files as a patch is more relevant to large projects where upstream maintains the autotools files, macros and makefiles.

> Since we maintain the autotools files ourselves, we shouldn't get
> into trouble with incompatible autotools versions.

And as a last resort, it would be possible to fix the files and build a pregenerated tarball based on "make dist".
Comment 31 Till Hofmann 2014-03-19 17:09:50 EDT
(In reply to Michael Schwendt from comment #30)
> For example, if you wanted to add the optional gmp/mpfr features, the patch
> would need to be adjusted to compile the extra sources into the library
> *and* install the extra headers, too.
> 
> That's a big of a chicken'n'egg problem. Once you decide on which features
> to include in the library, they cannot simply be turned on/off in the same
> way as when using fparser as a copylib (with modifications to fpconfig.hh
> possibly).

True, and that's why they don't include any build system upstream. As you said earlier:
> It's not a big issue. Either add the Requires or not would work. In my
> opinion, currently it would be cleaner, if no such dependencies were added.

But I don't see how this changed by using autotools? True, the build system doesn't support MPFR/GMP anymore, but didn't we decide to omit this feature anyway?

I guess before, MPFR/GMP features were included if MPFR or GMP were installed on the build system, but that was rather uninentional (otherwise I would have kept the BuildRequires).


> The attached patch is just an example of a basic framework to get started.

OK, but I don't see what is missing exactly? Do you want me to include the possibility to turn MPFR/GMP on/off?
Comment 32 Michael Schwendt 2014-03-19 18:51:16 EDT
What has changed is that two of the installed headers are wrong (= I copied all the names from the topdir instead of checking that three of them should not be installed). That's wrong, especially since the two gmp/mpfr headers could not be included anyway because of a missing subdir:

Cmake based:
$ rpmls -p fparser-devel-4.5.1-4.fc20.x86_64.rpm |grep inc
drwxr-xr-x  /usr/include/fparser
-rw-r--r--  /usr/include/fparser/fparser.hh
-rw-r--r--  /usr/include/fparser/fpaux.hh
-rw-r--r--  /usr/include/fparser/fpconfig.hh
-rw-r--r--  /usr/include/fparser/fptypes.hh

 -> fpaux.hh, fptypes.hh, fpconfig.hh are internal headers only

Autotools based:
$ rpmls -p fparser-devel-4.5.1-6.fc20.x86_64.rpm |grep inc
drwxr-xr-x  /usr/include/fparser
-rw-r--r--  /usr/include/fparser/fparser.hh
-rw-r--r--  /usr/include/fparser/fparser_gmpint.hh
-rw-r--r--  /usr/include/fparser/fparser_mpfr.hh
-rw-r--r--  /usr/include/fparser/fpconfig.hh

 -> fpconfig.hh is an internal header only
 -> fparser_*.hh should not be installed if gmp/mpfr support is not compiled in


> Do you want me to include the possibility to turn MPFR/GMP on/off?

That depends on whether those features will be needed. Building with MPFR/GMP would make the library depend on libmpfr/libgmp, but using the extra headers would still be optional -> the case early in the review.

Anyway, if you say MPFR/GMP is not needed, only the three headers ought to be dropped. Shipping internal headers is not a great idea. Eventually somebody would start including them.
Comment 33 Till Hofmann 2014-03-20 05:24:52 EDT
(In reply to Michael Schwendt from comment #32)
> Anyway, if you say MPFR/GMP is not needed, only the three headers ought to
> be dropped. Shipping internal headers is not a great idea. Eventually
> somebody would start including them.

I'm not saying it isn't needed, but it isn't essential. I don't think it would be a good idea to build the library with MPFR/GMP support without adding dependencies to these libraries, so I guess not supporting MPFR/GMP altogether is the best solution (as we discussed earlier). If somebody needs MPFR/GMP support, they can still use fparser as copylib.

That said, I removed the internal header files from the package. I also had another look at the debian package. They use some macros in configure.ac which don't seem to have any effect (because they define something which isn't used/checked by fparser). Other than that, it is similar to ours (yours).

I moved the autoreconf call to %prep.


Thanks for your help!


SPEC: http://thofmann.fedorapeople.org/fparser.spec
SRPM http://thofmann.fedorapeople.org/fparser-4.5.1-7.fc20.src.rpm
patch: http://thofmann.fedorapeople.org/fparser.autotools.patch
Comment 34 Michael Schwendt 2014-03-20 11:45:43 EDT
> I don't think it would be a good idea to build the library
> with MPFR/GMP support without adding dependencies to these libraries,

That dependency would be _automatic_, because libfparser would be linked with libgmp+libmpfr. That would result in an automatic dep on the lib SONAME.

On the contrary, using that extra part of the library API would be _optional_ -> the fparser.hh header does not include any header from gmp/mpfr. Only the optional extra headers do -> the fparser-devel package would not need to depend on those external -devel packages.
Comment 35 Till Hofmann 2014-03-26 08:46:49 EDT
You're right. 

Nevertheless, I think it is better not to add the optional parsers. Since all the parsers basically offer the same functionality with different types, there is no need for including them all. If all parsers are enabled, the resulting lib file's size blows up by a factor 4.

If there is a need for a special parser by another package, it could still be included later on.
Comment 36 Fedora Update System 2014-03-26 09:43:29 EDT
fparser-4.5.1-7.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/fparser-4.5.1-7.fc20
Comment 37 Fedora Update System 2014-04-04 05:46:36 EDT
fparser-4.5.1-7.fc20 has been pushed to the Fedora 20 stable repository.

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