[Bug 199168] Review Request: CGAL

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: CGAL
Alias: CGAL

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=199168





------- Additional Comments From laurent.rineau__fedora_extras@xxxxxxxxxxxxxx  2006-08-17 10:12 EST -------
Update:
  Spec URL: http://www.di.ens.fr/~rineau/Fedora/CGAL-3.2.1-15.fc5.src.rpm
  SRPM URL: http://www.di.ens.fr/~rineau/Fedora/CGAL.spec

%changelog
* Thu Aug 17 2006 Laurent Rineau 
<laurent.rineau__fedora_extras@xxxxxxxxxxxxxx> - 3.2.1-15
- Change the permissions of /etc/profile.d/cgal.*sh
- Remove the meta package CGAL. CGAL-libs is renamed CGAL.
- Added two patchs:
  - CGAL-3.2.1-config.h-endianness_detection.patch which is an upstream patch
    to fix the endianness detection, so that is is no longer hard-coded in
    <CGAL/compiler_config.h>,
  - CGAL-3.2.1-install_cgal-no_versions_in_compiler_config.h.patch that
    removes hard-coded versions in <CGAL/compiler_config.h>.


I have new errors from rpmlint:
  E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.sh
  E: CGAL-devel non-executable-script /etc/profile.d/cgal.sh 0644
  E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.csh
  E: CGAL-devel non-executable-script /etc/profile.d/cgal.csh 0644
that comes from the new permissions of those files.

And two new warnings:
  W: CGAL-devel non-conffile-in-etc /etc/profile.d/cgal.sh
  W: CGAL-devel non-conffile-in-etc /etc/profile.d/cgal.csh
that could be fixed easily.

Reporter   Laurent Rineau (laurent.rineau__fedora_extras@xxxxxxxxxxxxxx)    
  Assigned To   Ed Hill (ed@xxxxxxx)    

   Save Changes 
 Bug Comments 
Opened by Laurent Rineau (laurent.rineau__fedora_extras@xxxxxxxxxxxxxx)   on 
2006-07-17 13:36 EST  [reply]      
  Spec URL: http://www.di.ens.fr/~rineau/Fedora/CGAL-3.2.1-13-fc5.src.rpm
SRPM URL: http://www.di.ens.fr/~rineau/Fedora/CGAL.spec
Description: Computational Geometry Algorithms Library

CGAL is a collaborative effort of several sites in Europe and
Israel. The goal is to make the most important of the solutions and
methods developed in computational geometry available to users in
industry and academia in a C++ library. The goal is to provide easy
access to useful, reliable geometric algorithms.

Homepage: http://www.cgal.org/

Packager notes:
* With CGAL-3.2.1, the tarball has been pruned from documentation files with 
undecided license, in order to make packaging possible.
* A Debian package has been submitted, and has been accepted 
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=251885
* Whereas the Debian packager choose to extract the core++ library and put it 
in a separate package, I choose to ship libcore++.a in the CGAL package. It 
could be modified, if needed.
* rpmlint shows several errors or warnings. Some of them come from the 
meta-package CGAL that requires all sub-packages.
* The -devel sub-package ships several static library. It is because upstream 
developers do not want to maintain SOMAJOR numbers for them, now, for 
libcore++.a, and libCGALQt.a

 
20060801173634  Comment #1 From Dennis Gilmore (dennis@xxxxxxxx)    on 
2006-08-01 17:36 EST  [reply]      
  static linking is highly frowned upon 
http://fedoraproject.org/wiki/Packaging/Guidelines  for more info developer 
laziness is generally not considered a good enough reason.  

Looking at the sepc  all those macros  make the spec file confusing. Dont 
redefine  name version and release.

you need full urls  to the upstream source tarball. 

I really sugegst reading the packaging guidelines  and doing some work on them

 
20060802035012  Comment #2 From Laurent Rineau 
(laurent.rineau__fedora_extras@xxxxxxxxxxxxxx)    on 2006-08-02 03:50 EST  
[reply]      
  1/ I know that static libraries should be avoided, when possible (see my not 
in comment #1). In that case, the upstream developpers do not provide shared 
library for libCGALQt.a and libcore++.a. For libcore++, I could package Core 
separately (http://www.cs.nyu.edu/exact/core/download/core_v1.7/). But, for 
libCGALQt.a, do you see a solution? Waiting for the next release which could 
have shared version for all libraries cannot be a solution: CGAL releases come 
each year. It was really a chance that I manage to make the documention files  
removed from the upstream tarball of CGAL-3.2.1 (for license issues).

2/ As regards the macros... yes I know. This spec file is configurable, so 
that it can be applied to internal release of CGAL as well. What do you mean 
by redefining name of version or release? If I am not wrong, the conditionals 
make them be defined only once. If reviewers agreed that it is two much, I 
will pruned the spec file to remove the macro, as if the default values were 
hard-coded.

3/ For the upstream source tarball, I do not understand your point. spectool 
(from package fedora-rpmdevtools) can understand the macros and give the full 
URLs.

I know pretty well the packaging guidelines. Please give me pointers to 
paragraphs that I could have missed.


 
20060814221318  Comment #3 From Ed Hill (ed@xxxxxxx)    on 2006-08-14 22:13 
EST  [reply]      
  Hi Laurent, I'm interested in reviewing CGAL but am getting a 404 Not Found 
error when trying to download the SRPM.  Is this submission still active?

 
20060815041708  Comment #4 From Laurent Rineau 
(laurent.rineau__fedora_extras@xxxxxxxxxxxxxx)    on 2006-08-15 04:17 EST  
[reply]      
  Sorry. The url is there:

Spec URL: http://www.di.ens.fr/~rineau/Fedora/CGAL-3.2.1-13.fc5.src.rpm
SRPM URL: http://www.di.ens.fr/~rineau/Fedora/CGAL.spec

I am thinking about simplifying the spec file. Tell me what you think about 
it.

 
20060815105259  Comment #5 From Ed Hill (ed@xxxxxxx)    on 2006-08-15 10:52 
EST  [reply]      
  Hi Laurent, thanks for fixing the URL!  I grabbed the SRPM and am looking 
through it.  I agree with you that it would be a very good idea to cleanup 
the spec file and remove all the bits that are unnecessary for Fedora 
Extras (eg. all the internal_release, prefix, build_doc, etc. macros).  As 
soon as you have a slimmed-down SRPM available, please post it and I'll 
start doing a thorough review.

 
20060815185744  Comment #6 From Laurent Rineau 
(laurent.rineau__fedora_extras@xxxxxxxxxxxxxx)    on 2006-08-15 18:57 EST  
[reply]      
  Update:
  Spec URL: http://www.di.ens.fr/~rineau/Fedora/CGAL-3.2.1-14.fc5.src.rpm
  SRPM URL: http://www.di.ens.fr/~rineau/Fedora/CGAL.spec

I have pruned the spec files from non Fedora stuff. I eventually admit that 
the spec file is complicated enough, even now: the %install stage is quite 
complicated, as the upstream installation process is not yet well adapted to 
Fedora Extras requirements.


 
20060816010647  Comment #7 From Ralf Corsepius (rc040203@xxxxxxxxxx)    on 
2006-08-16 01:06 EST  [reply]      
  I must be missing something very basic:

# rpm -qlp CGAL-3.2.1-14.i386.rpm
/usr/share/doc/CGAL-3.2.1
/usr/share/doc/CGAL-3.2.1/LICENSE
/usr/share/doc/CGAL-3.2.1/LICENSE.FREE_USE
/usr/share/doc/CGAL-3.2.1/LICENSE.LGPL
/usr/share/doc/CGAL-3.2.1/LICENSE.QPL
/usr/share/doc/CGAL-3.2.1/README.Fedora

This doesn't look like a reasonable packaging to me.

Also:

# rpmlint CGAL-*3.2.1-14.i386.rpm
E: CGAL devel-dependency CGAL-devel
E: CGAL no-binary
W: CGAL-devel no-dependency-on CGAL
E: CGAL-devel file-in-usr-marked-as-conffile /usr/share/CGAL/make/makefile
E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.sh
E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.csh

 
20060816025233  Comment #8 From Ralf Corsepius (rc040203@xxxxxxxxxx)    on 
2006-08-16 02:52 EST  [reply]      
  Further issues:

- The *-devel package ships /usr/include/CORE
IMO, this directory name is too general.

- Static libs:
/usr/lib/libCGALQt.a
/usr/lib/libcore++.a

- A more general design problem:
Some headers in /usr/include/CGAL hard-code configuration-time detected
* system features, e.g. the version of zlib and Qt

* compiler characteristics, e.g. endianness.




 
20060816031135  Comment #9 From Laurent Rineau 
(laurent.rineau__fedora_extras@xxxxxxxxxxxxxx)    on 2006-08-16 03:11 EST  
[reply]      
  Actions(In reply to comment #7)
> I must be missing something very basic:
> 
> # rpm -qlp CGAL-3.2.1-14.i386.rpm
> /usr/share/doc/CGAL-3.2.1
> /usr/share/doc/CGAL-3.2.1/LICENSE
> /usr/share/doc/CGAL-3.2.1/LICENSE.FREE_USE
> /usr/share/doc/CGAL-3.2.1/LICENSE.LGPL
> /usr/share/doc/CGAL-3.2.1/LICENSE.QPL
> /usr/share/doc/CGAL-3.2.1/README.Fedora
> 
> This doesn't look like a reasonable packaging to me.
> 
> Also:
> 
> # rpmlint CGAL-*3.2.1-14.i386.rpm
> E: CGAL devel-dependency CGAL-devel
> E: CGAL no-binary
> W: CGAL-devel no-dependency-on CGAL

CGAL is a meta-package that requires CGAL-libs, CGAL-devel, and CGAL-sources. 
The reason is that the CGAL users community is used to get CGAL as a whole.

> E: CGAL-devel file-in-usr-marked-as-conffile /usr/share/CGAL/make/makefile
> E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.sh
> E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.csh

As far as I know, these rpmlint errors should be ignored.

(In reply to comment #8)
> Further issues:
> 
> - The *-devel package ships /usr/include/CORE
> IMO, this directory name is too general.

CGAL-3.2.1 ships CORE-1.7, http://www.cs.nyu.edu/exact/core_pages/intro.html
This directory is from CORE.

> - Static libs:
> /usr/lib/libCGALQt.a
> /usr/lib/libcore++.a

upstream libCGALQt is static only, as indicated in comment #1, as well as 
upstream libcore++. I know that static libraries should be avoided "as far as 
possible", in Fedora. Is the upstream devs choice a sufficient reason?

> - A more general design problem:
> Some headers in /usr/include/CGAL hard-code configuration-time detected
> * system features, e.g. the version of zlib and Qt
> 
> * compiler characteristics, e.g. endianness.

Yes, it should only be /usr/include/CGAL/compiler_config.h. Is it a blocker?


 
20060816032618  Comment #10 From Ralf Corsepius (rc040203@xxxxxxxxxx)    on 
2006-08-16 03:26 EST  [reply]      
  (In reply to comment #9)
> Actions(In reply to comment #7)
> > I must be missing something very basic:
> > 
> > # rpm -qlp CGAL-3.2.1-14.i386.rpm
> > /usr/share/doc/CGAL-3.2.1
> > /usr/share/doc/CGAL-3.2.1/LICENSE
> > /usr/share/doc/CGAL-3.2.1/LICENSE.FREE_USE
> > /usr/share/doc/CGAL-3.2.1/LICENSE.LGPL
> > /usr/share/doc/CGAL-3.2.1/LICENSE.QPL
> > /usr/share/doc/CGAL-3.2.1/README.Fedora
> > 
> > This doesn't look like a reasonable packaging to me.
> > 
> > Also:
> > 
> > # rpmlint CGAL-*3.2.1-14.i386.rpm
> > E: CGAL devel-dependency CGAL-devel
> > E: CGAL no-binary
> > W: CGAL-devel no-dependency-on CGAL
> 
> CGAL is a meta-package that requires CGAL-libs, CGAL-devel, and 
CGAL-sources. 

Contradicts Fedora conventions and IMNSOH, is complete non-sense.
Consider this to be a MUST FIX.

Put the run-time libs into CGAL or CGAL-libs and the devel files into *-devel.

> > E: CGAL-devel file-in-usr-marked-as-conffile /usr/share/CGAL/make/makefile
> > E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.sh
> > E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.csh
> 
> As far as I know, these rpmlint errors should be ignored.
Nope, these scripts are incomplete. MUSTFIX

> (In reply to comment #8)
> > Further issues:
> > 
> > - The *-devel package ships /usr/include/CORE
> > IMO, this directory name is too general.
> 
> CGAL-3.2.1 ships CORE-1.7, http://www.cs.nyu.edu/exact/core_pages/intro.html
> This directory is from CORE.
And? This doesn't answer my remark.

> > - Static libs:
> > /usr/lib/libCGALQt.a
> > /usr/lib/libcore++.a
> 
> upstream libCGALQt is static only, as indicated in comment #1, as well as 
> upstream libcore++. I know that static libraries should be avoided "as far 
as 
> possible", in Fedora. Is the upstream devs choice a sufficient reason?
Formally not, but it's sufficient reason for me not to approve a package and 
to
classify a package's quality as "low" ;)

> > - A more general design problem:
> > Some headers in /usr/include/CGAL hard-code configuration-time detected
> > * system features, e.g. the version of zlib and Qt
> > 
> > * compiler characteristics, e.g. endianness.
> 
> Yes, it should only be /usr/include/CGAL/compiler_config.h. Is it a blocker?
Well, there actually are 2 issues with this.
- Package dependencies. You will have to find a way to handle the hard-coded
version dependencies in rpm.


- Hard-coding compiler characteristics is a common design flaw many packages
suffer from. This should not be much of a problem for current Fedora, but can
easily become one. In many cases, such stuff disqualfies a package from
inclusion in multilib'ed distros. This is an upstream problem, which probably
doesn't affect current Fedora.

[Wrt. endianness: Many people miss that endianness is a compiler feature.
Packages hard-coding endianness break on biendian targets, e.g. for 
multilib'ed
mips and sh distros]




 
20060816035855  Comment #11 From Laurent Rineau 
(laurent.rineau__fedora_extras@xxxxxxxxxxxxxx)    on 2006-08-16 03:58 EST  
[reply]      
  (In reply to comment #10)
> > CGAL is a meta-package that requires CGAL-libs, CGAL-devel, and 
CGAL-sources. 
> 
> Contradicts Fedora conventions and IMNSOH, is complete non-sense.
> Consider this to be a MUST FIX.
> 
> Put the run-time libs into CGAL or CGAL-libs and the devel files into 
*-devel.

For the moment (CGAL-3.2.1-14), libs are in CGAL-libs, and devel files are in 
CGAL-devel. 

> > > E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.sh
> > > E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.csh
> > 
> > As far as I know, these rpmlint errors should be ignored.
> Nope, these scripts are incomplete. MUSTFIX

Ok. I thought it was agreed that shell scripts in /etc/profile.d/ should not 
have shell bangs. If it is really a MUSTFIX, this should be written somewhere, 
and bugs should be reported, against all almost all package that ship 
something in /etc/profile.d/

> > (In reply to comment #8)
> > > Further issues:
> > > 
> > > - The *-devel package ships /usr/include/CORE
> > > IMO, this directory name is too general.
> > 
> > CGAL-3.2.1 ships CORE-1.7, 
http://www.cs.nyu.edu/exact/core_pages/intro.html
> > This directory is from CORE.
> And? This doesn't answer my remark.

I do not see any solution, here. <CORE/...h> is the documented way to include 
CORE headers. If this is a blocker, CORE cannot be into Fedora. That's it.
> > > - A more general design problem:
> > > Some headers in /usr/include/CGAL hard-code configuration-time detected
> > > * system features, e.g. the version of zlib and Qt
> > > 
> > > * compiler characteristics, e.g. endianness.
> > 
> > Yes, it should only be /usr/include/CGAL/compiler_config.h. Is it a 
blocker?
> Well, there actually are 2 issues with this.
> - Package dependencies. You will have to find a way to handle the hard-coded
> version dependencies in rpm.

Actually, these version macros are not used in CGAL. They are not even 
documented. They could be pruned. They are used by the CGAL test suite to 
display dependencies' versions.


 
20060816063508  Comment #12 From Patrice Dumas (pertusus@xxxxxxx)    on 
2006-08-16 06:35 EST  [reply]      
  For computation package like this one I think it is much better 
to keep the static libraries, since the usual reasons for not having
static libraries don't hold, while it is very convenient to be
able to link models statically to run them on any linux.

 
20060816064157  Comment #13 From Patrice Dumas (pertusus@xxxxxxx)    on 
2006-08-16 06:41 EST  [reply]      
  (In reply to comment #10)
> (In reply to comment #9)
> > Actions(In reply to comment #7)

> > > E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.sh
> > > E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.csh
> > 
> > As far as I know, these rpmlint errors should be ignored.
> Nope, these scripts are incomplete. MUSTFIX

This is really an ignorable error. These files are sourced and not
executed. Most of the files in /etc/profile.d don't have a shebang.


> > > - Static libs:

> > upstream libcore++. I know that static libraries should be avoided "as far 
as 
> > possible", in Fedora. Is the upstream devs choice a sufficient reason?
> Formally not, but it's sufficient reason for me not to approve a package and 
to
> classify a package's quality as "low" ;)

Although it would be preferable to have dynamic libraries, it is 
important in my view to have static library for computational 
packages too, in order to be able to build statically compiled
executables to be able to run on any linux.

 
20060816115516  Comment #14 From Ed Hill (ed@xxxxxxx)    on 2006-08-16 11:55 
EST  [reply]      
  >  - Static libs:

My view on static libs is somewhere between Patrice and Ralf.  In 
situations where upstream does not provide a build mechanism for 
shared libs then I think it is acceptable for Fedora Extras packagers 
to ship static libs in -devel (that is, I don't think its necessary to 
force FE packagers to modify the build files to the extent needed to 
produce shared instead of static).  And, this view is entirely 
consistent with the Packaging Guidelines as they are currently 
written.

That said, we can always submit patches to the upstream folks and try 
to help them build and use shared libs.  Helping the upstream projects 
is an honorable thing!  :-)

In any case, using the latest (-14) SRPM:
 + significantly cleaned up spec file (good!)
 + built for me in mock on FC5-i386
 + has an ability to work with LEDA but, since LEDA is now 
   proprietary, its fine that we don't try to use it (OK)
 - has an ability to work with TAUCS and, since TAUCS is LGPL,
   we should seriously consider packaging it for FE and using 
   it as a CGAL build dependency

Anyway, I'll submit a more thorough review later this week...

 
20060816162206  Comment #15 From Laurent Rineau 
(laurent.rineau__fedora_extras@xxxxxxxxxxxxxx)    on 2006-08-16 16:22 EST  
[reply]      
  Taucs suffers the "static-only-libs" problem too. And its build/config 
system 
is really strange.

Let us see if CGAL can be include in FE, then I will handle Taucs as a further 
step (CGAL libraries do not depend on Taucs: only the templates library, in 
headers, do).

 
20060817032959  Comment #16 From Ralf Corsepius (rc040203@xxxxxxxxxx)    on 
2006-08-17 03:29 EST  [reply]      
  (In reply to comment #13)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Actions(In reply to comment #7)
> 
> > > > E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.sh
> > > > E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.csh
> > > 
> > > As far as I know, these rpmlint errors should be ignored.
> > Nope, these scripts are incomplete. MUSTFIX
> 
> This is really an ignorable error.
Well, agreed, it's minor error, nevertheless it's an error and easy to fix.

> These files are sourced and not
> executed. 
Then they should NOT be executable => chmod -x

> Most of the files in /etc/profile.d don't have a shebang.
Just because others are sloppy, doesn't mean I need to be.

 
20060817074840  Comment #17 From Laurent Rineau 
(laurent.rineau__fedora_extras@xxxxxxxxxxxxxx)    on 2006-08-17 07:48 EST  
[reply]      
  (In reply to comment #16)
> > > > > E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.sh
> > > > > E: CGAL-devel script-without-shellbang /etc/profile.d/cgal.csh
> > These files are sourced and not
> > executed. 
> Then they should NOT be executable => chmod -x

Agreed. It is patch in my spec file. Maybe should someone fill bugs about 
other packages that share that issue. I do not know how to use XML-XPC.
  >> TODO-latter

(In reply to comment #8)
> - A more general design problem:
> Some headers in /usr/include/CGAL hard-code configuration-time detected
> * system features, e.g. the version of zlib and Qt

The CGAL_FOOBAR_VERSION macros are not used in CGAL, actually. They are here 
only for internal uses (to be displayed by the testsuite). I can prune that 
from the package.
 
> * compiler characteristics, e.g. endianness.

The endianness detection has been fixed in the upstream SVN repository 
yesterday, from your comment #10. It now uses macros, and no longer hard-code 
endianness. I will backport the patch in the src.rpm package.

To summarize, the issues that remains:
(In reply to comment #8)
> Further issues:
> 
> - The *-devel package ships /usr/include/CORE
> IMO, this directory name is too general.
> 
> - Static libs:
> /usr/lib/libCGALQt.a
> /usr/lib/libcore++.a

These two issue last. And I do not see how to deal with that (especially 
the /usr/include/CORE issue, which cannot be fixed without changing CORE 
documentation and uses).


-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.

_______________________________________________
Fedora-package-review mailing list
Fedora-package-review@xxxxxxxxxx
http://www.redhat.com/mailman/listinfo/fedora-package-review

[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]