Bug 476398 - Review Request: eclib - A Library for Doing Computations on Elliptic Curves
Review Request: eclib - A Library for Doing Computations on Elliptic Curves
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Alex Lancaster
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-13 23:33 EST by Conrad Meyer
Modified: 2009-05-23 04:24 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-05-23 04:24:19 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
alexl: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
spec patch for check section (286 bytes, patch)
2009-03-19 16:09 EDT, Michael Schwendt
no flags Details | Diff
eclib-20080310.p7-build.patch (593 bytes, patch)
2009-03-22 09:46 EDT, Michael Schwendt
no flags Details | Diff
spec patch for build fix (1.09 KB, patch)
2009-03-22 09:47 EDT, Michael Schwendt
no flags Details | Diff

  None (edit)
Description Conrad Meyer 2008-12-13 23:33:21 EST
Spec URL: http://konradm.fedorapeople.org/fedora/SPECS/eclib.spec
SRPM URL: http://konradm.fedorapeople.org/fedora/SRPMS/eclib-20080310-1.p7.fc9.src.rpm
Description:
John Cremona's programs for enumerating and computating with elliptic
curves defined over the rational numbers.
Comment 1 Conrad Meyer 2008-12-14 00:35:04 EST
Koji scratch build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=997355
Comment 2 Alex Lancaster 2009-02-19 05:44:18 EST
Here is the review:

Is there really no base package?  Not sure what the best way is to deal with the lack of soname versions here.

 +:ok, =:needs attention, -:needs fixing, N/A =: not applicable

MUST Items:
[-] MUST: rpmlint must be run on every package.
# rpmlint eclib-devel-20080310-1.p7.fc11.i586.rpm eclib-static-20080310-1.p7.fc11.i586.rpm /home/alex/RPMS/SRPMS/eclib-20080310-1.p7.fc9.src.rpm 
eclib-devel.i586: W: no-dependency-on eclib/eclib-libs/libeclib
eclib-devel.i586: W: no-soname /usr/lib/libjcntl.so
eclib-devel.i586: W: no-soname /usr/lib/librankntl.so
eclib-devel.i586: W: shared-lib-calls-exit /usr/lib/librankntl.so exit@GLIBC_2.0
eclib-devel.i586: W: no-soname /usr/lib/libg0nntl.so
eclib-devel.i586: W: no-soname /usr/lib/libcurvesntl.so
eclib-static.i586: W: no-documentation
eclib-static.i586: W: binaryinfo-readelf-failed 
3 packages and 0 specfiles checked; 0 errors, 8 warnings.

Looks like there needs to be some soname patching perhaps.  See the Debian package

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

Version number issues, the "patch 7" can cause problems.  Let's say you increase the patch version and reset the release number to one, then you would have

# rpmdev-vercmp 20080310-2.p7 20080310-1.p8
0:20080310-2.p7 is newer

but you really want -1.p8 to be newer.  This is OK if you always increment the release number as patch version increases.

[x] MUST: The spec file name must match the base package %{name}
[x] MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines.
[x] MUST: The License field in the package spec file must match the actual license.
[N/A] 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.
[x] MUST: The spec file must be written in American English.
[x] MUST: The spec file for the package MUST be legible.
[x] MUST: The sources used to build the package must match the upstream source, as provided in the spec URL.
md5sum eclib-20080310.p7.spkg rpmbuild/SOURCES/eclib-20080310.p7.spkg 
bfef44c232be8cfad5e9cb7030dc1c2e  eclib-20080310.p7.spkg
bfef44c232be8cfad5e9cb7030dc1c2e  rpmbuild/SOURCES/eclib-20080310.p7.spkg
[x] MUST: The package must successfully compile and build into binary rpms on at least one supported architecture.

koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1139171

[N/A] 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.
[x] MUST: All build dependencies must be listed in BuildRequires
[N/A] MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro.
[-] 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.

This is related to the soname issues above.

[N/A] MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review
[x] MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory.
[x] MUST: A package must not contain any duplicate files in the %files listing.
[x] MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line.
[x] MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[x] MUST: Each package must consistently use macros, as described in the macros section of Packaging Guidelines.
[x] MUST: The package must contain code, or permissible content. This is described in detail in the code vs. content section of Packaging Guidelines.
[N/A] MUST: Large documentation files should go in a doc subpackage.
[N/A] MUST: If a package includes something as %doc, it must not affect the runtime of the application.
[x] MUST: Header files must be in a -devel package.
[x] MUST: Static libraries must be in a -static package.
[N/A] MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability).
[x] MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
[x] MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} 
[x] MUST: Packages must NOT contain any .la libtool archives, these should be removed in the spec.
[N/A] MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section.
[x] MUST: Packages must not own files or directories already owned by other packages.
[x] MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
[x] MUST: All filenames in rpm packages must be valid UTF-8.

SHOULD Items:
[-] SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[N/A] SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available.
[x] SHOULD: The reviewer should test that the package builds in mock.
[x] SHOULD: The package should compile and build into binary rpms on all supported architectures.
[] SHOULD: The reviewer should test that the package functions as described.
[N/A] SHOULD: If scriptlets are used, those scriptlets must be sane.
[N/A] SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[N/A] SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb.
[N/A] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself.
[-] SHOULD: Packages should try to preserve timestamps of original installed files.

The install probably should use  "install -p"
Comment 3 Conrad Meyer 2009-03-13 01:35:25 EDT
So, the issues are:
- use install -p
- fix the soname mess, probably install to a subdirectory of _libdir?

(I'm aware that the release cannot be reset to 1 when a new patch comes along, a number of packages I have use pre-release or otherwise weird versioning.)
Comment 4 Alex Lancaster 2009-03-13 04:56:43 EDT
(In reply to comment #3)
> So, the issues are:
> - use install -p
> - fix the soname mess, probably install to a subdirectory of _libdir?

Right, yes fix those issues and generate a new spec and I'll finish off the review.  Also ping upstream about the license if you get a chance (not necessary for finishing the review).
 
> (I'm aware that the release cannot be reset to 1 when a new patch comes along,
> a number of packages I have use pre-release or otherwise weird versioning.)  

OK, that's fine, so long as you are aware of that and ensure that upgrade paths are always maintained.  (Also please make a note of this situation in the spec file near the version number so that other provenpackager maintainers who may need to fix the package from time to time).
Comment 5 Conrad Meyer 2009-03-13 19:49:27 EDT
(In reply to comment #4)
> OK, that's fine, so long as you are aware of that and ensure that upgrade paths
> are always maintained.  (Also please make a note of this situation in the spec
> file near the version number so that other provenpackager maintainers who may
> need to fix the package from time to time).  

It's a fairly common scheme for non-numeric versions and is specified specifically in the naming guidelines; I don't think this is needed. I'll try to get around to the other things soon, but I have finals in the next couple days.
Comment 7 Alex Lancaster 2009-03-19 01:18:46 EDT
Did a scratch koji build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1249388

However, still some warnings from rpmlint:

$ rpmlint eclib-devel-20080310-2.p7.fc11.i586.rpm eclib-static-20080310-2.p7.fc11.i586.rpm 
eclib-devel.i586: W: no-dependency-on eclib/eclib-libs/libeclib
eclib-static.i586: W: no-documentation
eclib-static.i586: W: binaryinfo-readelf-failed 
2 packages and 0 specfiles checked; 0 errors, 3 warnings.


The no-documentation looks like fine to ignore, not sure about no-dependency-on and  binaryinfo-readelf-failed.  Can you either fix them, or explain why we can ignore them.  Is it because upstream doesn't use proper sonames?
Comment 8 Conrad Meyer 2009-03-19 01:24:06 EDT
No-dependency-on is completely safe to ignore, there is no main package to depend on.

Not sure why the binaryinfo-readelf-failed is happening, I didn't get that from rpmlint on my system (F-10, x86_64).
Comment 9 Conrad Meyer 2009-03-19 01:24:44 EDT
Can you run rpmlint -i and get the full explanation for binaryinfo-readelf-failed?
Comment 10 Alex Lancaster 2009-03-19 01:30:10 EDT
(In reply to comment #9)
> Can you run rpmlint -i and get the full explanation for
> binaryinfo-readelf-failed?  

$ rpmlint -i eclib-devel-20080310-2.p7.fc11.i586.rpm eclib-static-20080310-2.p7.fc11.i586.rpm 
eclib-devel.i586: W: no-dependency-on eclib/eclib-libs/libeclib
eclib-static.i586: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

eclib-static.i586: W: binaryinfo-readelf-failed 
Executing readelf on this file failed, all checks could not be run.

2 packages and 0 specfiles checked; 0 errors, 3 warnings.

I'm running rpmlint on a F-10/i386 system which might be the cause of the warning.
Comment 11 Alex Lancaster 2009-03-19 01:39:24 EDT
By the way I tried compiling this on F-10 to see if the i386/i586 mismatch made a difference to rpmlint output, but it failed:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1249408

(this isn't a pre-condition of passing the review, but it should ultimately compiled on F-10).
Comment 12 Conrad Meyer 2009-03-19 01:40:36 EDT
Odd, it builds just fine in mock on F-10 for me.
Comment 13 Conrad Meyer 2009-03-19 01:41:13 EDT
Ah, I'm missing an -fPIC I guess.
Comment 14 Conrad Meyer 2009-03-19 01:41:52 EDT
Wait, no, I'm not. That's just odd.
Comment 15 Alex Lancaster 2009-03-19 02:00:38 EDT
(In reply to comment #12)
> Odd, it builds just fine in mock on F-10 for me.  

Do a scratch build on koji is a more reliable indicator of success since you're actually using the real buildsystem.  Perhaps there is a missing BuildRequires?
Comment 16 Conrad Meyer 2009-03-19 02:08:00 EDT
Mock would catch a missing buildrequires.
Comment 17 Alex Lancaster 2009-03-19 02:17:35 EDT
(In reply to comment #16)
> Mock would catch a missing buildrequires.  

But it still fails in koji for some reason, which is where it counts (although for the review it only has to build in rawhide).
Comment 18 Alex Lancaster 2009-03-19 03:30:38 EDT
I think we should switch back to using eclib rather than eclib-devel, regarding versioning, I posted on f-d-l asking for advice, and here's my post and a good suggestion from Hans:

http://www.redhat.com/archives/fedora-devel-list/2009-March/msg01117.html

which I think might be the best way to go.
Comment 19 Michael Schwendt 2009-03-19 05:38:10 EDT
> g++ -c -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-
> protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
> -fasynchronous-unwind-tables -fPIC -DUSE_PARI_FACTORING -DNTL_ALL
> -I/usr/local/include -I/usr/local/include interface.cc -o interface_n.o

> pslave_n.o parifact_n.o  -lpari -L/usr/local/lib -L/usr/local/lib -lntl
> -lgmp -lpari -lm  

Find out where the -I/usr/local/include and -L/usr/local/lib come from, then get rid of them for sake of reproducible builds. No issue for builds in clean buildroots. Not clean for ordinary builds on installed Fedora systems where /usr/local might contain locally built stuff.


> %package        devel

According to the guidelines, this ought to
Requires: %{name} = %{version}-%{release}
to enforce a matching pair of development files and binaries.
This guideline alone should make a packager realise "oh, wait, there are shared libs in this package, so not creating a main library package for them would be strange".


In reply to comment 3:
> - fix the soname mess, probably install to a subdirectory of _libdir?

> %{_libdir}/%{name}/*.so
> %{_libdir}/%{name}/lib*.a

That doesn't fix the "soname mess".

It makes things worse, because you've moved the shared libs out of run-timer linker's search path. Any application linked to these libs would fail to start. The static archive would not be found either at build-time. It would be necessary to adjust the compiler's library search path (-L%{_libdir}/%{name}), which probably no existing application does, because it expects to find the eclib libraries in default search path.

Further, the shared libraries [if moved incorrectly as in current spec file] are still seen by rpmbuild's dependency generators. They still lead to automatic SONAME "Provides" and "Requires", even if the libraries won't be found at run-time.

The SONAME is the internal shared object name as stored in the ELF header. You can display it with objdump -p or eu-readelf like this:

$ eu-readelf -d /usr/lib/libgthread-2.0.so|grep -i soname
  SONAME            Library soname: [libgthread-2.0.so.0]



Noticing that the package builds several test programs, consider including a %check section for "make check". It is good packaging-practise to run a test-suite at build-time unless it is known/confirmed to be broken.
Comment 20 Conrad Meyer 2009-03-19 13:40:26 EDT
(In reply to comment #19)
> Find out where the -I/usr/local/include and -L/usr/local/lib come from, then
> get rid of them for sake of reproducible builds. No issue for builds in clean
> buildroots. Not clean for ordinary builds on installed Fedora systems where
> /usr/local might contain locally built stuff.

This doesn't matter on Koji or in mock.

> > %package        devel
> 
> According to the guidelines, this ought to
> Requires: %{name} = %{version}-%{release}
> to enforce a matching pair of development files and binaries.
> This guideline alone should make a packager realise "oh, wait, there are shared
> libs in this package, so not creating a main library package for them would be
> strange".

Except you don't require the main package if one doesn't exist. It's not obvious.

> In reply to comment 3:
> > - fix the soname mess, probably install to a subdirectory of _libdir?
> 
> > %{_libdir}/%{name}/*.so
> > %{_libdir}/%{name}/lib*.a
> 
> That doesn't fix the "soname mess".

That's a comma. It's there to separate two ideas.

> It makes things worse, because you've moved the shared libs out of run-timer
> linker's search path. Any application linked to these libs would fail to start.

Right, and the exactly one application that cares isn't in Fedora yet and ships its own copy of eclib.

> The static archive would not be found either at build-time. It would be
> necessary to adjust the compiler's library search path (-L%{_libdir}/%{name}),
> which probably no existing application does, because it expects to find the
> eclib libraries in default search path.

Doesn't matter, it's easy enough to adjust search path at build time for static libs.

> Further, the shared libraries [if moved incorrectly as in current spec file]
> are still seen by rpmbuild's dependency generators. They still lead to
> automatic SONAME "Provides" and "Requires", even if the libraries won't be
> found at run-time.

They can be moved back if you like.
 
> Noticing that the package builds several test programs, consider including a
> %check section for "make check". It is good packaging-practise to run a
> test-suite at build-time unless it is known/confirmed to be broken.

I'm happy to take a patch.
Comment 21 Michael Schwendt 2009-03-19 16:09:36 EDT
Created attachment 335928 [details]
spec patch for check section

Great, one of the tests fails even. :)  If upstream cannot reproduce it by running "spkg-check" manually, it would be Fedora-specific.


> Except you don't require the main package if one doesn't exist.
> It's not obvious.

That's splitting-hairs. You could apply your weird theory to every package and move even applications into -devel packages. *This* package builds shared libraries, which are used at run-time. A -devel package, on the contrary, is fully optional. A run-time package must not depend on a -devel package.


> That's a comma. It's there to separate two ideas.

Doesn't matter, you moved the shared libs actually. That breaks the package badly already. It's a blocker during review. Shared libs in wrong path, no ldconfig scriptlets either.


> They can be moved back if you like.

It will be required for this package to pass review.


> it's easy enough to adjust search path at build time for static libs.

Why exactly do you move them to a non-standard location?
Comment 22 Conrad Meyer 2009-03-19 16:26:45 EDT
(In reply to comment #21)
> Created an attachment (id=335928) [details]
> spec patch for check section
> 
> Great, one of the tests fails even. :)  If upstream cannot reproduce it by
> running "spkg-check" manually, it would be Fedora-specific.

Probably why I didn't include them initially.

> That's splitting-hairs. You could apply your weird theory to every package and
> move even applications into -devel packages. *This* package builds shared
> libraries, which are used at run-time. A -devel package, on the contrary, is
> fully optional. A run-time package must not depend on a -devel package.

It's not some weird theory I personally have to go out of my way to harm Fedora, I just assume the writers of the guidelines were more knowledgeable about shared libraries and sonames than I am.

> > That's a comma. It's there to separate two ideas.
> 
> Doesn't matter, you moved the shared libs actually. That breaks the package
> badly already. It's a blocker during review. Shared libs in wrong path, no
> ldconfig scriptlets either.

I also added sonames, which is what "fixing the soname mess" was referring to. They don't have versions, though. Look at the patch in the last SRPM.

> > They can be moved back if you like.
> 
> It will be required for this package to pass review.

Done.


You know, you can offer constructive criticism without being rude or attacking. I don't mean you or Fedora any harm.
Comment 23 Conrad Meyer 2009-03-19 16:49:44 EDT
New URLs:
http://konradm.fedorapeople.org/fedora/SPECS/eclib.spec
http://konradm.fedorapeople.org/fedora/SRPMS/eclib-20080310-3.p7.fc10.src.rpm

Rpmlint output:
eclib.x86_64: W: shared-lib-calls-exit /usr/lib64/librankntl.so.20080310 exit@GLIBC_2.2.5
eclib-devel.x86_64: W: no-documentation
eclib-static.x86_64: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 3 warnings.
Comment 24 Michael Schwendt 2009-03-19 17:45:15 EDT
> being rude or attacking.

What do you refer to here? Comment 21? Did you consider your comment 20 as particularly nice and friendly? I found it quite arrogant, but I would have replied to you in private if I had considered it to be a real problem.

Your reply about the test-suite is not much different. Nowhere before have you mentioned the failing test. No comment in the spec file at all. As soon as I point out the existance of a test-suite and provide the patch you asked for, you brush aside all this with a single sentence. 

A failing "make check" ought to raise an alarm bell in a packager's head. A packager should really look into it and report it upstream, too.

[...]

The "soname mess" is much more than rpmlint's warning about lack of sonames. Release 2.p7.fc10 of the src.rpm added non-versioned sonames, but that didn't fix the mess.  3.p7.fc10 is _much_ better, except that typically upstream developers ought to participate in the decision on what SONAMEs and versions to choose. librankntl.so.20080310 is an interesting approach, but it is different than upstream's releases and also different than all other distributions, for example.
Comment 25 Conrad Meyer 2009-03-19 18:04:26 EDT
Let's not get off topic on this bug, but I would appreciate it if you wouldn't be so quick to attack in the future. I will try to be less arrogant.

(In reply to comment #24)
> A failing "make check" ought to raise an alarm bell in a packager's head. A
> packager should really look into it and report it upstream, too.

In the few sources that I have packaged that provided tests, few of them pass. This is not a problem with the packaging, but rather that upstream doesn't bother keeping their tests passing. Generally upstream will fix them on its own if it wants to, or ignore my reports about it being broken if they don't want to. So I don't feel especially motivated to contact them about it.

> 3.p7.fc10 is _much_ better, except that typically upstream
> developers ought to participate in the decision on what SONAMEs and versions to
> choose. librankntl.so.20080310 is an interesting approach, but it is different
> than upstream's releases and also different than all other distributions, for
> example.  

I am glad to hear it's better. I feel like we should probably include the p7 tag into the SONAME as well. What do you think? My rationale for librankntl.so.20080310 is Hans' post on f-d-l thread about this bug (http://article.gmane.org/gmane.linux.redhat.fedora.devel/107950). I don't know if there is a way we can be compatible with all other distributions; as I've said before this library has exactly one user (Sage), and upstream has stopped even releasing independent tarballs (instead, they release .spkgs in Sage).
Comment 26 Alex Lancaster 2009-03-19 22:03:20 EDT
Gentlemen, yes, let's keep the focus on the review.  I agree with most of the issues Michael raised.   However, I also think Conrad is doing his best and may have misinterpreted Michael's initial terseness as rudeness, although it wasn't intended that way.   These issues aren't as well documented on the wiki and in the packaging guidelines as they could be, so I can understand the confusion.  (I'm still learning about some of the more subtle issues with dynamic linking myself).

In any case, as I am still the reviewer, here are the things I think still need fixing:

1. fix the SONAME issues.  Perhaps check with Debian's package of eclib to see how they are doing it, so we can be consistent.  One of the problems is that upstream doesn't maintain a stable API/ABI.

2. move the libraries to the standard location (I believe this was fixed in the last version)

3. implement "make check".  disable the tests that fail for the moment and report them to upstream so they can be fixed in a later release (hopefully they will tell us which one's are expected to work and not work).
Comment 27 Conrad Meyer 2009-03-19 22:36:56 EDT
(In reply to comment #26)
> 1. fix the SONAME issues.  Perhaps check with Debian's package of eclib to see
> how they are doing it, so we can be consistent.  One of the problems is that
> upstream doesn't maintain a stable API/ABI.

The latest spec is an attempt at that -- the SONAME for each library is set to "lib<library>.so.%{version}".

> 2. move the libraries to the standard location (I believe this was fixed in the
> last version)

Yep, that was fixed.
 
> 3. implement "make check".  disable the tests that fail for the moment and
> report them to upstream so they can be fixed in a later release (hopefully they
> will tell us which one's are expected to work and not work).  

That was done in the last version also (except all tests are disabled). How do you want me to disable just the failing tests?
Comment 28 Alex Lancaster 2009-03-20 04:18:35 EDT
(In reply to comment #27)

> The latest spec is an attempt at that -- the SONAME for each library is set to
> "lib<library>.so.%{version}".

Compiles on koji for rawhide:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1250851

Regarding versioned SONAMES on other distributions, it seems that only Debian is packaging this at the moment, and they include it the sagemath package internally:

http://packages.debian.org/sid/amd64/sagemath/filelist

and don't include any version in the sonames:

/usr/lib/sagemath/local/lib/libcurvesntl.so
/usr/lib/sagemath/local/lib/libg0nntl.so
/usr/lib/sagemath/local/lib/libjcntl.so
/usr/lib/sagemath/local/lib/librankntl.so

> > 3. implement "make check".  disable the tests that fail for the moment and
> > report them to upstream so they can be fixed in a later release (hopefully they
> > will tell us which one's are expected to work and not work).  
> 
> That was done in the last version also (except all tests are disabled). How do
> you want me to disable just the failing tests?  

I would just patch out the targets in the Makefile.
Comment 29 Alex Lancaster 2009-03-20 05:52:31 EDT
Regarding %check, looks like the export line should be:

export LD_LIBRARY_PATH=$RPM_BUILD_ROOT%{_libdir}/

otherwise it doesn't find the right versioned .so files at run-time.
Comment 30 Michael Schwendt 2009-03-20 07:49:33 EDT
My patch was made for pkg Release 2.p7 and was correct for that release. Maybe the versioned soname changes in 3.p7 require changes to %check.

[...]

Meanwhile, I've contacted the upstream developer. He's interested in it and has responded very quickly with a guess that the test fails because it cannot write to a temporary directory. That's not it. I've forwarded the manually created verbose test output upstream.
Comment 31 Conrad Meyer 2009-03-20 13:16:00 EDT
(In reply to comment #29)
> Regarding %check, looks like the export line should be:
> 
> export LD_LIBRARY_PATH=$RPM_BUILD_ROOT%{_libdir}/
> 
> otherwise it doesn't find the right versioned .so files at run-time.  

I've updated this:
http://konradm.fedorapeople.org/fedora/SPECS/eclib.spec
http://konradm.fedorapeople.org/fedora/SRPMS/eclib-20080310-3.p7.fc10.src.rpm

(In reply to comment #30)
> Meanwhile, I've contacted the upstream developer. He's interested in it and has
> responded very quickly with a guess that the test fails because it cannot write
> to a temporary directory. That's not it. I've forwarded the manually created
> verbose test output upstream.  

Thank you very much. I guess we're in luck that upstream cares :).
Comment 32 Conrad Meyer 2009-03-20 13:17:32 EDT
(In reply to comment #29)
> Regarding %check, looks like the export line should be:
> 
> export LD_LIBRARY_PATH=$RPM_BUILD_ROOT%{_libdir}/
> 
> otherwise it doesn't find the right versioned .so files at run-time.  

I've updated this:
http://konradm.fedorapeople.org/fedora/SPECS/eclib.spec
http://konradm.fedorapeople.org/fedora/SRPMS/eclib-20080310-4.p7.fc10.src.rpm

(In reply to comment #30)
> Meanwhile, I've contacted the upstream developer. He's interested in it and has
> responded very quickly with a guess that the test fails because it cannot write
> to a temporary directory. That's not it. I've forwarded the manually created
> verbose test output upstream.  

Thank you very much. I guess we're in luck that upstream cares :).
Comment 33 Michael Schwendt 2009-03-20 13:35:17 EDT
News: Upstream cannot reproduce the test failure ... and still thinks it is somehow related to not finding a file. Doubtful IMO, also after checking strace -- the wrong value found during the test is read from a file which is created from scratch.

[...]

eclib.spec should "BuildRequires: ntl-static" instead of ntl-devel, because NTL is a static-only package. Due to a bug in ntl.spec, ntl-devel didn't provide ntl-static so far. ntl-5.4.2-7.fc11 shall fix this.
Comment 34 Conrad Meyer 2009-03-20 13:54:47 EDT
(In reply to comment #33)
> News: Upstream cannot reproduce the test failure ... and still thinks it is
> somehow related to not finding a file. Doubtful IMO, also after checking strace
> -- the wrong value found during the test is read from a file which is created
> from scratch.

Hopefully they'll be able to figure it out.

> eclib.spec should "BuildRequires: ntl-static" instead of ntl-devel, because NTL
> is a static-only package. Due to a bug in ntl.spec, ntl-devel didn't provide
> ntl-static so far. ntl-5.4.2-7.fc11 shall fix this.

Ah, thanks.
http://konradm.fedorapeople.org/fedora/SPECS/eclib.spec
http://konradm.fedorapeople.org/fedora/SRPMS/eclib-20080310-5.p7.fc10.src.rpm
Comment 35 Michael Schwendt 2009-03-22 09:46:53 EDT
Created attachment 336199 [details]
eclib-20080310.p7-build.patch

I've found the problem after a close look. It's a packaging mistake.

Parts of the code are miscompiled because the OPTFLAG environment variable is redefined and alters the source tarball's build flags. The NEW_OP_ORDER definition is lost. That miscompiles some source files.

The cleaner way would be to patch the src/Makefile and make it accept our $RPM_OPT_FLAGS from the outside. It already accepts $PICFLAG for "-fPIC". With this change it is not necessary to move fundamental build flags into the spec file.
Comment 36 Michael Schwendt 2009-03-22 09:47:40 EDT
Created attachment 336200 [details]
spec patch for build fix
Comment 37 Conrad Meyer 2009-03-22 17:59:48 EDT
Thank you for both patches; they have been incorporated in:
http://konradm.fedorapeople.org/fedora/SPECS/eclib.spec
http://konradm.fedorapeople.org/fedora/SRPMS/eclib-20080310-6.p7.fc10.src.rpm
Comment 38 Alex Lancaster 2009-04-01 01:00:47 EDT
[x] builds OK in koji in dist-f11:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1267138

[x] have a sane main package with versioned so files, relocated to %{_libdir} where they should be
[x] -devel package has symlinks
[x] added "BuildRequires:  ntl-static" (comment #33)
[x] fixed compilation flags with Michael's patch (comment #35) this allowed "make check" to work and tests now pass, so tests re-enabled

[=] new rpmlint output:

$ rpmlint eclib-*
eclib.i586: W: shared-lib-calls-exit /usr/lib/librankntl.so.20080310 exit@GLIBC_2.0
eclib-devel.i586: W: no-documentation
eclib-static.i586: W: no-documentation
eclib-static.i586: W: binaryinfo-readelf-failed 
3 packages and 0 specfiles checked; 0 errors, 4 warnings.

can you investigate 1) the shared-lib-calls-exit warning and if we can ignore, please justify and 2) the binaryinfo-readelf-failed for the -static subpackage
Comment 39 Alex Lancaster 2009-04-01 01:24:56 EDT
Btw, this fails on F-10:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1269432

because ntl-static doesn't yet exist:

 No Package Found for ntl-static

Is there a pending update for ntl?
Comment 40 Conrad Meyer 2009-04-01 12:00:36 EDT
(In reply to comment #38)
> can you investigate 1) the shared-lib-calls-exit warning and if we can ignore,
> please justify and 2) the binaryinfo-readelf-failed for the -static subpackage  

1) Shared-lib-calls-exit means that somewhere in the library a piece of code calls exit(). This is a poor practice for a library but it's not our fault and we can ignore it.

2) I don't know why this happens, it should be resolved before this is approved.

(In reply to comment #39)
> Btw, this fails on F-10:
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=1269432
> 
> because ntl-static doesn't yet exist:
> 
>  No Package Found for ntl-static
> 
> Is there a pending update for ntl?  

No, AFAIK the maintainer only fixed ntl-devel to provide ntl-static in Rawhide.
Comment 41 Alex Lancaster 2009-05-05 04:24:03 EDT
(In reply to comment #40)
> (In reply to comment #38)
> > can you investigate 1) the shared-lib-calls-exit warning and if we can ignore,
> > please justify and 2) the binaryinfo-readelf-failed for the -static subpackage  
> 
> 1) Shared-lib-calls-exit means that somewhere in the library a piece of code
> calls exit(). This is a poor practice for a library but it's not our fault and
> we can ignore it.

OK.

> 2) I don't know why this happens, it should be resolved before this is
> approved.

Ping?  I think this is the last issue remaining before approval.
Comment 42 Mamoru TASAKA 2009-05-05 06:39:41 EDT
Only for the following "issue" (as I found this comment
by chance):

(In reply to comment #41)
> (In reply to comment #40)
> > (In reply to comment #38)
> > 2) I don't know why this happens, it should be resolved before this is
> > approved.
> 
> Ping?  I think this is the last issue remaining before approval.  

From rpmlint, binaryinfo-readelf-failed message actually means that
(please check /usr/share/rpmlint/BinariesCheck.py )
$ readelf -W -S -l -d -s libjcntl.a >/dev/null ; echo $?
readelf: Error: Not an ELF file - it has the wrong magic bytes at the start
readelf: Error: ../libjcntl.a(smatrix_elim_n.o): Failed to read file header
readelf: Error: Not an ELF file - it has the wrong magic bytes at the start
readelf: Error: ../libjcntl.a(realroots_n.o): Failed to read file header
1
(Note that this result is for -static package rebuilt
 by koji scratch build)

Then when checking libjcnl.a, it shows:
$ ar x ../libjcntl.a 
$ ls -al smatrix_elim_n.o realroots_n.o
-rw-r--r-- 1 tasaka1 tasaka1 0 2009-05-05 19:30 realroots_n.o
-rw-r--r-- 1 tasaka1 tasaka1 0 2009-05-05 19:30 smatrix_elim_n.o
i.e. these files are empty.

Strangely, when I try mockbuild on my local machine, rpmlint
warns nothing for -static package and actually
$ ls -al smatrix_elim_n.o realroots_n.o
-rw-r--r-- 1 tasaka1 mock 52724 2009-05-05 19:22 realroots_n.o
-rw-r--r-- 1 tasaka1 mock 92368 2009-05-05 19:22 smatrix_elim_n.o

All other files in libjcntl.a (rebuilt on my local machine)
have the same sizes as those in libjcntl.a rebuilt by koji scratch
build.
So I am not sure why these 2 files are empty when rebuilt on koji.
Comment 43 Mamoru TASAKA 2009-05-05 07:30:46 EDT
Ah, killing parallel make seems to work...
Comment 45 Mamoru TASAKA 2009-05-17 08:10:45 EDT
Alex, ping?
Comment 46 Alex Lancaster 2009-05-17 10:52:12 EDT
koji scratch build for F-11:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1358794

-static subpackage now appears to pass rpmlint tests:

$ rpmlint eclib-20080310-7.p7.fc11.i586.rpm eclib-devel-20080310-7.p7.fc11.i586.rpm eclib-static-20080310-7.p7.fc11.i586.rpm 
eclib.i586: W: shared-lib-calls-exit /usr/lib/librankntl.so.20080310 exit@GLIBC_2.0
eclib-devel.i586: W: no-documentation
eclib-static.i586: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 3 warnings.

So, marking this as APPROVED.

(The rawhide/F-12 fails to build because of issues with the dependent packages like pari-devel not yet being rebuilt:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1358789

this will probably go away soon, and shouldn't hold up building the package, even if only for F-11/F-10 initially).
Comment 47 Michael Schwendt 2009-05-17 12:15:49 EDT
No, ntl in devel now builds a shared lib and no ntl-static anymore:
http://koji.fedoraproject.org/koji/buildinfo?buildID=98367
Comment 48 Conrad Meyer 2009-05-18 13:06:40 EDT
Ok, I'll fix this later today (to shared-only).
Comment 50 Conrad Meyer 2009-05-18 18:18:24 EDT
New Package CVS Request
=======================
Package Name: eclib
Short Description: A Library for Doing Computations on Elliptic Curves
Owners: konradm
Branches: F-11
InitialCC:
Comment 51 Kevin Fenzi 2009-05-18 19:42:39 EDT
cvs done.
Comment 52 Conrad Meyer 2009-05-23 04:24:19 EDT
Imported and built in rawhide:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1372086

Closing. Thanks, Alex!

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