Spec URL: http://konradm.fedorapeople.org/fedora/SPECS/cddlib.spec SRPM URL: http://konradm.fedorapeople.org/fedora/SRPMS/cddlib-094f-1.fc9.src.rpm Description: The C-library cddlib is a C implementation of the Double Description Method of Motzkin et al. for generating all vertices (i.e. extreme points) and extreme rays of a general convex polyhedron in R^d given by a system of linear inequalities: P = { x=(x1, ..., xd)^T : b - A x >= 0 } where A is a given m x d real matrix, b is a given m-vector and 0 is the m-vector of all zeros. The program can be used for the reverse operation (i.e. convex hull computation). This means that one can move back and forth between an inequality representation and a generator (i.e. vertex and ray) representation of a polyhedron with cdd. Also, cdd can solve a linear programming problem, i.e. a problem of maximizing and minimizing a linear function over P.
Hi, this package produces two rpms. The first contains only the documentation, with a single file: cddlibman.dvi I think this dvi file should be converted to a pdf file, so people can read it with any pdf reader installed on the system. The second rpm is the devel rpm, and contains 7 .h and 2 .a files (static libraries). You did the right thing, putting these files in a devel package. However, you suppressed the generation of the examples in the spec file, but I think they are important for the users. When I generated the examples, I saw that they were installed in /usr/bin and that they were quite a few files (26). I would suggest installing the examples in the main package, in a directory examples, which can be part of the documentation.
All that should be in -devel, even the doc and the examples, if they aren't too big in size. Main package shouldn't contain just one doc file, it's counter-intuitive (to me). You can simply not produce the main package (by omitting its %files section) and put the doc in -doc subpackage if you must.
Example binaries shouldn't be in documentation! The source for them is already there (IIRC) or if not can be added there. Maybe they can be placed in %libexec if they are helpful, but I thought the source would be helpful, while the binaries would not be. But I will take all the other suggestions for the next spec.
Here are some of the examples created: /usr/bin/testlp2_gmp /usr/bin/testlp3 /usr/bin/testlp3_gmp /usr/bin/testshoot /usr/bin/testshoot_gmp It seems to me that this package requires gmp. The README advises to use gmp for getting accurate results. "Whenever the time is not important, it is safer to use gmp rational arithmetic." In the doc directory there is already a pdf version of the documentation. All the example sources are in the directory src. At least these sources should be in the documentation.
(In reply to comment #4) > ... > It seems to me that this package requires gmp. This builds one library that uses gmp and one that doesn't. I'm not terribly familiar with the use of the library so I felt it was best to include both options; I would appreciate input from anyone who is more informed. > In the doc directory there is already a pdf version of > the documentation. You prefer pdf to dvi? They are both supposed to be portable printable formats, but I'm not sure which we prefer in Fedora. > All the example sources are in the directory src. > At least these sources should be in the documentation. OK.
I would suggest a spec file like this: Spec URL: http://orion.lcg.ufrj.br/RPMS/SPECS/cddlib.spec Without "BR: gmp-devel" it does not build in mock... and for developing with cddlib, gmp-devel is also necessary.
New URLs: http://konradm.fedorapeople.org/fedora/SPECS/cddlib.spec http://konradm.fedorapeople.org/fedora/SRPMS/cddlib-094f-2.fc9.src.rpm
Hi, Some of the source/example files had the wrong permissions (executable bit on). rpmlint outputs some warnings: cddlib-devel.x86_64: W: spurious-executable-perm /usr/share/doc/cddlib-devel-094f/src-mathlink2/mathlink.h cddlib-devel.x86_64: W: spurious-executable-perm /usr/share/doc/cddlib-devel-094f/src-mathlink/mathlink.h ..... Specifying 0644 for files in defattr will fix the issue. This package does not have any executable file, anyway. Even if it had, you could an extra defattr or a per file permission change. %files devel %defattr(0644,root,root,0755) %doc doc/cddlibman.pdf %doc examples* src* %{_includedir}/*.h %{_libdir}/libcdd.a %{_libdir}/libcddgmp.a
.a libs are supposed to be +x, I think this problem is easier to fix in prep stage. New URLs: http://konradm.fedorapeople.org/fedora/SPECS/cddlib.spec http://konradm.fedorapeople.org/fedora/SRPMS/cddlib-094f-3.fc9.src.rpm
(In reply to comment #9) > .a libs are supposed to be +x, I think this problem is easier to fix in prep > stage. New URLs: > http://konradm.fedorapeople.org/fedora/SPECS/cddlib.spec > http://konradm.fedorapeople.org/fedora/SRPMS/cddlib-094f-3.fc9.src.rpm Not really. .a libs, generally, are 644 (ls -al /usr/lib64/*.a or /usr/lib/*.a) But your way works, and you also removed the .* files, which was really good. I am going to give you a practice review, which is NOT an official review. ------------------------------------------------------------------------ MUST: rpmlint must be run on every package. The output should be posted in the review. [lua:~/rhrpms] rpmlint cddlib-devel-094f-3.fc8.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Clean. - MUST: The package must be named according to the Package Naming Guidelines . OK. - MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption on Package Naming Guidelines . OK. - MUST: The package must meet the Packaging Guidelines . OK. - MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines . OK. - MUST: The License field in the package spec file must match the actual license. OK. - 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. OK. - MUST: The spec file must be written in American English. OK. - MUST: The spec file for the package MUST be legible. If the reviewer is unable to read the spec file, it will be impossible to perform a review. Fedora is not the place for entries into the Obfuscated Code Contest (http://www.ioccc.org/). OK. - MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this. OK. - MUST: The package must successfully compile and build into binary rpms on at least one supported architecture. Builds on F-8 x86_64 and F-9 i386 (using mock). - MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch needs to have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number should then be placed in a comment, next to the corresponding ExcludeArch line. New packages will not have bugzilla entries during the review process, so they should put this description in the comment until the package is approved, then file the bugzilla entry, and replace the long explanation with the bug number. The bug should be marked as blocking one (or more) of the following bugs to simplify tracking such issues: FE-ExcludeArch-x86 , FE-ExcludeArch-x64 , FE-ExcludeArch-ppc , FE-ExcludeArch-ppc64 NA. - MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense. OK. - MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden. NA. - MUST: Every binary RPM package which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. If the package has multiple subpackages with libraries, each subpackage should also have a %post/%postun section that calls /sbin/ldconfig. An example of the correct syntax for this is: %post -p /sbin/ldconfig %postun -p /sbin/ldconfig NA. - MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. NA. - 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. Refer to the Guidelines for examples. OK. - MUST: A package must not contain any duplicate files in the %files listing. OK. - 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. OK. - MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} ( or $RPM_BUILD_ROOT ). OK. - MUST: Each package must consistently use macros, as described in the macros section of Packaging Guidelines . OK. - MUST: The package must contain code, or permissable content. This is described in detail in the code vs. content section of Packaging Guidelines . OK. - MUST: Large documentation files should go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity) Maybe. There is a considerable amount of files. - MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. OK. - MUST: Header files must be in a -devel package. OK. - MUST: Static libraries must be in a -static package. The package contains basically two static libraries. The spec has a "Provides: %{name}-static = %{version}-%{release}" clause. This is fine for me. - MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). NA. - 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. NA. - MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} NA. - MUST: Packages must NOT contain any .la libtool archives, these should be removed in the spec. NA. - 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. This is described in detail in the desktop files section of the Packaging Guidelines . If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. NA. - MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. OK. - MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} ( or $RPM_BUILD_ROOT ). See Prepping BuildRoot For %install for details. OK. Summary: the documentation contains several examples, which could be in a separate doc package, for clarity. See item above. But this is my personal opinion.
(In reply to comment #10) > Not really. .a libs, generally, are 644 (ls -al /usr/lib64/*.a or /usr/lib/*.a) Ah, when I checked a random lib I must have found a lib that was 755. My mistake! I am glad this way works ok also. I looked at the examples and source and they are only a few megs uncompressed and probably compress well, so I don't think it's a big deal having it all in one package. Besides, to use the library (i.e. link against it, it's static) you probably want the documentation anyway. In the future doing reviews you might just omit the stuff that's OK or N/A and just tell me the stuff I'm doing wrong :D. But I appreciate the (practice) review.
rpmlint not clean for me: cddlib-debuginfo.i386: E: empty-debuginfo-package This debuginfo package contains no files. This is often a sign of binaries being unexpectedly stripped too early during the build, rpmbuild not being able to strip the binaries, the package actually being a noarch one but erratically packaged as arch dependent, or something else. Verify what the case is, and if there's no way to produce useful debuginfo out of it, disable creation of the debuginfo package. The file: examples-ml/Combinatorica5.m Cannot be included in Fedora cue to the following licensing clause: This package may be copied in its entirety for nonprofit purposes only. Sale, other than for the direct cost of the media, is prohibited. This copyright notice must accompany all copies. Paulo, grep -i license * and */* and */*/* . . .etc will help you find this sort of thing. Why -devel and not -static, as there are no solibs? I'm ok with the docs/examples being bundled, they're not that big, left to packager discretion IMHO. Otherwise, a good review. Checking mock BRs, will be slightly delayed there due to system issues. :(
(In reply to comment #12) > cddlib-debuginfo.i386: E: empty-debuginfo-package http://fedoraproject.org/wiki/Packaging/Debuginfo suggests I should look for -s being passed to gcc or ld, or strip being called somewhere. I'm now building in mock to be able to grep over a build.log for these. > The file: > examples-ml/Combinatorica5.m > Cannot be included in Fedora cue to the following licensing clause: > > This package may be copied in its entirety for nonprofit purposes only. > Sale, other than for the direct cost of the media, is prohibited. This > copyright notice must accompany all copies. Good spot. Will rm in prep. > Why -devel and not -static, as there are no solibs? "2. Static libraries only. When a package only provides static libraries you can place all the static library files in the *-devel subpackage. When doing this you also must have a virtual Provide for the *-static package." from http://fedoraproject.org/wiki/Packaging/Guidelines#Packaging_Static_Libraries My personal reason? There are header files too, therefore it's useful for devel. > I'm ok with the docs/examples being bundled, they're not that big, left to > packager discretion IMHO. > > Otherwise, a good review. Checking mock BRs, will be slightly delayed there > due to system issues. :( Thanks for helping Paulo out. I appreciate the input from both of you!
Mock build.log on my machine: http://pastie.caboo.se/302506 I don't see any gcc -s or strip commands being run. However, I think mock/rpmbuild might be using *my* CFLAGS which differ from %{optflags}, so I'm exporting CFLAGS="%{optflags}" in %build now and seeing if that yields a non-empty debuginfo package.
(In reply to comment #14) > Mock build.log on my machine: http://pastie.caboo.se/302506 > > I don't see any gcc -s or strip commands being run. However, I think > mock/rpmbuild might be using *my* CFLAGS which differ from %{optflags}, so I'm > exporting CFLAGS="%{optflags}" in %build now and seeing if that yields a > non-empty debuginfo package. Nope, still empty. I guess that means I should disable it?
>> The file: >> examples-ml/Combinatorica5.m >> Cannot be included in Fedora cue to the following licensing clause: >> >> This package may be copied in its entirety for nonprofit purposes only. >> Sale, other than for the direct cost of the media, is prohibited. This >> copyright notice must accompany all copies. > >Good spot. Will rm in prep. No, you need a modified tarball, so it doesn't wind up in the SRPM. See:https://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code Yeah, probably disable. And your -static/-devel explanation is IMHO sound.
(In reply to comment #16) > >> The file: > >> examples-ml/Combinatorica5.m > >> Cannot be included in Fedora cue to the following licensing clause: > >> > >> This package may be copied in its entirety for nonprofit purposes only. > >> Sale, other than for the direct cost of the media, is prohibited. This > >> copyright notice must accompany all copies. > > > >Good spot. Will rm in prep. > > No, you need a modified tarball, so it doesn't wind up in the SRPM. > See:https://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code That links says "Some upstream packages include patents or trademarks that we are not allowed to ship even as source code." It does not say we need to remove improperly licensed files from the source tarball. > Yeah, probably disable. And your -static/-devel explanation is IMHO sound. Alright.
New URLs: http://konradm.fedorapeople.org/fedora/SPECS/cddlib.spec http://konradm.fedorapeople.org/fedora/SRPMS/cddlib-094f-4.fc9.src.rpm
(In reply to comment #17) > (In reply to comment #16) > > >> The file: > > >> examples-ml/Combinatorica5.m > > >> Cannot be included in Fedora cue to the following licensing clause: > > >> > > >> This package may be copied in its entirety for nonprofit purposes only. > > >> Sale, other than for the direct cost of the media, is prohibited. This > > >> copyright notice must accompany all copies. > > > > > >Good spot. Will rm in prep. > > > > No, you need a modified tarball, so it doesn't wind up in the SRPM. > > See:https://fedoraproject.org/wiki/Packaging/SourceURL#When_Upstream_uses_Prohibited_Code > > That links says "Some upstream packages include patents or trademarks that we > are not allowed to ship even as source code." It does not say we need to remove > improperly licensed files from the source tarball. Read the next sentence in that paragraph: "In these cases you have to modify the source tarball to remove this code before you even upload it to the build system." > > Yeah, probably disable. And your -static/-devel explanation is IMHO sound. > > Alright.
(In reply to comment #19) > Read the next sentence in that paragraph: > > "In these cases you have to modify the source tarball to remove this code > before you even upload it to the build system." Yes, my point being the quoted sentence only applies if the first statement is true, which it is not. There is nothing patented or trademarked about that one file, it is simply improperly licensed. We can delete it and be on our merry way.
And if it's not removed from the tarball, it's distributed in the SRPM, which is a no-no. Blocking FE-LEGAL for confirmation.
Ok.
This is correct. That license text says that you cannot distribute it for profit, which would mean that someone putting the SRPM on a CD couldn't resel the CD to cover their costs because that file is in the original tarball, even if we patch it out or immediately delete it. I know it is a pain, but you must make a modified tarball that doesn't have any bits under non-commercial licenses. You should probably point this out to upstream and recommend that they pull this code out of their tarball, and offer it separately on their website instead.
New URLs: http://konradm.fedorapeople.org/fedora/SPECS/cddlib.spec http://konradm.fedorapeople.org/fedora/SRPMS/cddlib-094f-5.fc9.src.rpm
Good, but it'd be best to comment on exactly how one may create the modified tarball from upstream, and not have the URL in the tag. i.e. Change: Source0: ftp://ftp.ifor.math.ethz.ch/pub/fukuda/cdd/%{name}-094f.tar.gz to: #Source0: ftp://ftp.ifor.math.ethz.ch/pub/fukuda/cdd/%{name}-094f.tar.gz #tar -xzf cddlib-094f.tar.gz #rm foo #tar -czf cddlib-094f/ cddlib-094f.tar.gz Source0: cddlib-094f.tar.gz This way someone testing md5sum will understand. . .
Or even rename the tarball to cddlib-094f-free.tar.gz.
(In reply to comment #26) > Or even rename the tarball to cddlib-094f-free.tar.gz. Which would go even farther in clarifying the issue. Good idea.
Ok. Thanks for the feedback. http://konradm.fedorapeople.org/fedora/SPECS/cddlib.spec http://konradm.fedorapeople.org/fedora/SRPMS/cddlib-094f-6.fc9.src.rpm
Any new comments?
Paulo, were you doing to take this one and to the formal review now that you're sponsored, or shall I? Conrad, if he doesn't want to or can't soon, I'll do it.
Hi, Jon I can finish the review. In fact, the only problem was the legal issued you raised and Spot clarified for us. I'll finish the review tonight.
Lifting FE-Legal, as this is now okay with the stripped tarball.
I extracted the files from the .src.rpm: rpm2cpio ../cddlib-094f-6.fc9.src.rpm | cpio -ivmud cddlib-094f-free.tar.gz cddlib.spec 2328 blocks [cascavel:~/temp/temp] tar -ztvf cddlib-094f-free.tar.gz gzip: stdin: not in gzip format tar: Child returned status 1 tar: Error exit delayed from previous errors Although the file has an extension .tar.gz it seems to be just an uncompressed tar archive (.tar). Anyway, the file with the license issue has been removed. [cascavel:~/temp/temp] tar -tvf cddlib-094f-free.tar.gz | grep Combinatorica5.m [cascavel:~/temp/temp] and the debug package generation has been disabled. Please, just check the .tar.gz file creation and the package will be ready.
It's actually a .tar.bz2. I've renamed the SOURCE file and await your approval.
Oh, I see. That is right, it is a tar.bz2 Did you update the links? Also, the spec file should change Source0: %{name}-094f-free.tar.gz for Source0: %{name}-094f-free.tar.bz2
Yes, I fixed the spec file. No, I didn't bother uploading the changes to the same place because it is trivial. When you set "fedora-review" to "+" I can start getting it imported into Fedora.
Approved.
Thanks. New Package CVS Request ======================= Package Name: cddlib Short Description: A library for generating all vertices in convex polyhedrons Owners: konradm Branches: F-10 F-9 InitialCC:
CVS Done
**** Access denied: konradm is not in ACL for rpms/cddlib/devel cvs commit: Pre-commit check failed Hm?
Apparently CVS issue was resolved. Built in rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=957127 and closing.
I haven't seen discussed why upstream only provides a static library instead of a Shared Object ? Which Package will use this one (does a runtime test was done) ? There is no needs to export CFLAGS before the %configure macro (as already done within the %configure). Headers got installed in /usr/include which is the standard path. Does someone checked if there is possible namespace issue ? Also, headers should have been installed using install -p to prevent timestramp changes. Just quick notes...
I don't know why upstream chooses static v. shared. No packages depend on this one *yet*, though sage will when it is packaged. The other issues will be addressed in the next update.
(In reply to comment #42) > I haven't seen discussed why upstream only provides a static library instead of > a Shared Object ? Some people still prefer static libraries. > Which Package will use this one (does a runtime test was done) ? All the examples have been run. > There is no needs to export CFLAGS before the %configure macro (as already done > within the %configure). Agreed, but this is just redundant. > Headers got installed in /usr/include which is the standard path. Does someone > checked if there is possible namespace issue ? All the identifiers in the includes have prefixes, such as dd_ , ddd_ , ddf_ Very unlike they will conflict with anything else. > Also, headers should have been installed using install -p to prevent timestramp > changes. > What headers?
Can we close this?
For me, you can review the export CFLAGS, and build the package.
Using static libraries instead of shared libraries isn't a matter of preference. Upstream should be educated to use shared library. Unless there is a very imperial technical reason (circle dependency, ABI un-stability, etc). This package shouldn't make use of a static library instead of creating a shared object. Futhermore src-mathlink and src-mathlink2 aren't compiled Again, there is no reason for such package to install theses headers in /usr/include instead of /usr/include/cddlib to prevent namespace issues. Also, Adding pkgconfig support should have been done to ease the -I/usr/include/cddlib addition. cddlibman.pdf should have been generated from the .tex file. How do you run the example ?
(In reply to comment #47) > Using static libraries instead of shared libraries isn't a matter of > preference. > Upstream should be educated to use shared library. Unless there is a very > imperial technical reason (circle dependency, ABI un-stability, etc). > > This package shouldn't make use of a static library instead of creating a > shared object. The last update from upstream was in early 2008 and the email address bounces. > Futhermore src-mathlink and src-mathlink2 aren't compiled Are these useful? > Again, there is no reason for such package to install theses headers in > /usr/include instead of /usr/include/cddlib to prevent namespace issues. Agreed. > Also, Adding pkgconfig support should have been done to ease the > -I/usr/include/cddlib addition. Unneeded work considering how many dependencies this library will have. > cddlibman.pdf should have been generated from the .tex file. Done. > How do you run the example ? $ gcc /usr/share/doc/cddlib-devel-094f/src/testcdd1.c -I/usr/include/cddlib -lcdd $ ./a.out When it asks you for a file, pass one of the files in /usr/share/doc/cddlib-devel-094f/examples/*.ine.
The mathlink stuff is interfacing code to Mathematica. I don't know if mathlink is Free Software, Mathematica definitely isn't. :-) In any case, that code can't be built without mathlink.
Any other improvement ? If this package isn't usable, it "have to" be removed from the Fedora package collection.
Uh, what? It's quite usable.
so, which package is using cddlib-devel then ?
Sage, which hasn't been admitted into Fedora yet. It's perfectly fine for libraries to be packaged for Fedora even if there is no software in Fedora uses them (yet).
(In reply to comment #53) > Sage, which hasn't been admitted into Fedora yet. It's perfectly fine for > libraries to be packaged for Fedora even if there is no software in Fedora uses > them (yet). yep, even if said package itself is (for whatever reason) forbidden in fedora, there is a need to be assured that cddlib package itself works. So how can I test that sage works against the current fedora cddlib package ? even if Sage is at preliminary packaging step ... BTW: I guess you didn't meant https://bugzilla.redhat.com/show_bug.cgi?id=198834
(In reply to comment #54) > yep, even if said package itself is (for whatever reason) forbidden in fedora, > there is a need to be assured that cddlib package itself works. > > So how can I test that sage works against the current fedora cddlib package ? > even if Sage is at preliminary packaging step ... It doesn't yet. > BTW: I guess you didn't meant > https://bugzilla.redhat.com/show_bug.cgi?id=198834 No. https://fedoraproject.org/wiki/SIGs/SciTech/SAGE
Anyway, I'm only interested in having this bug closed properly. So... good luck for the next steps.
Shall we close this bug now that it is imported ?
That is my thought. Closing.
Package Change Request ====================== Package Name: cddlib New Branches: EL-5 Owners: tremble Fedora owner not interested in EL: https://bugzilla.redhat.com/show_bug.cgi?id=574082
cvs done.