Bugzilla will be upgraded to version 5.0 on a still to be determined date in the near future. The original upgrade date has been delayed.

Bug 463767 (cloog)

Summary: Review Request: cloog - The Chunky Loop Generator
Product: [Fedora] Fedora Reporter: Dodji Seketeli <dodji>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: dvlasenk, fedora-package-review, itamar, mtasaka, notting, pmachata
Target Milestone: ---Flags: mtasaka: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-02-18 12:42:53 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On: 463742    
Bug Blocks:    
Attachments:
Description Flags
My version of srpm
none
Adds a cloog-config.h file to the distribution
none
pass make distcheck none

Description Dodji Seketeli 2008-09-24 12:10:36 EDT
Spec URL: http://www.seketeli.org/dodji/rpms/cloog/cloog.spec
SRPM URL: http://www.seketeli.org/dodji/rpms/cloog/cloog-0.15-0.1.git95753.fc9.src.rpm

Description:

CLooG is a software which generates loops for scanning Z-polyhedra. That is,
CLooG finds the code or pseudo-code where each integral point of one or more
parametrized polyhedron or parametrized polyhedra union is reached. CLooG is
designed to avoid control overhead and to produce a very efficient code.
Comment 1 Denys Vlasenko 2008-09-24 13:35:58 EDT
Created attachment 317609 [details]
My version of srpm

I and Dodji made a srpm each. Lack of communication :(

I did not look at Dodji's, so do not take it as a "fixed" or "better" version.
Just uploading mine so that all work in one place.
Comment 2 Dodji Seketeli 2008-09-28 14:42:14 EDT
To rebuild the srpm, you need the ppl library package, version 10pre30 at least. The problem is that it's not packaged in Fedora yet. I have updated the spec and srpm of the ppl package and filed a request in bug #463742. So you can go to that bug to get the srpm and build it so that you can build the cloog package.
Comment 3 Mamoru TASAKA 2008-10-03 12:13:45 EDT
Just noting that the maintainer of ppl on Fedora is also the upstream developer
of ppl, so he actually knows pre31 (currently) is released. It may be that
he has some reason he doesn't want to import pre tarball into Fedora.
Comment 4 Dodji Seketeli 2008-10-03 12:19:54 EDT
@Mamoru: Yes, I know that. I just needed to have a packaged version of ppl 10pre<something> to actually be able to _build_ cloog, and maybe ease the process of review of cloog for those who might be willing to review of me. Cloog needs those versions of ppl. And I need a clood to be able to build/package gcc from trunk.

It was not my intent to rush the ppl maintainer.
Comment 5 Mamoru TASAKA 2008-11-08 03:15:44 EST
Some notes for 0.15-0.1:

* SourceURL
  - When the source is created from git repository, please
    write in the spec file as comments how you created
    the source tarball:
    https://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control

* BuildRequires
  - As ppl-devel 0.10 is available on dist-f11, please modify BuildRequires.

* Requires
  - Please check if all required packages are to be correctly installed
    for -devel subpackage.
    ! For example, %_includedir/%name/ppl_backend.h contains:
--------------------------------------------------------------
    33  
    34  #include <ppl_c.h>
    35  #include <gmp.h>
    36  #include <stdlib.h>
    37  #include <string.h>
--------------------------------------------------------------
      This means that -devel subpackage must have 
      "Requires: ppl-devel gmp-devel" (here I am not saying about BuildRequires)

* Group
  - Usually the main packge has "Group: System Environment/Libraries"
    and -devel subpackage has "Group: Development/Libraries".
    ! You can make your spec file based on the skeleton spec file
      created by
      $ rpmdev-newspec -t lib cloog

* Dependency between main/subpackages
  - Usually the dependency between main package and subpackages
    must be EVR (Epoch:Version:Release) specific
    (i.e. -devel subpackage must have "Requires: %{name} = %{version}-%{release})

* Shipping static archives
  - Please explain why you want to ship static archives even if
    shared library is available. This must be avoided unless there is
    some specific reason. c.f.
    https://fedoraproject.org/wiki/Packaging/Guidelines#Exclusion_of_Static_Libraries

* cflags
  - When compiling this software the option "-fomit-frame-pointer"
    is used. This option makes debugging very difficult, so this option
    must be removed.

* Timestamps
  - Please consider to use
--------------------------------------------------------------------
make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
--------------------------------------------------------------------
    to keep timestamps on installed files as much as possible.
    This method usually works for Makefiles generated by recent
    autotools.

* %_infodir/dir file
  - Rebuild fails without
--------------------------------------------------------------------
rm -f $RPM_BUILD_ROOT%{_infodir}/dir
--------------------------------------------------------------------
    http://koji.fedoraproject.org/koji/taskinfo?taskID=921939

* Info files
  - Files under %_infodir are automatically regarded as %doc.
  - Please follow
    https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Texinfo

* %changelog format
  - For how to write %changelog, please follow
    https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs
    
* Documents
  - Please add the files below to %doc of main package.
--------------------------------------------------------------------
README
--------------------------------------------------------------------

! Note
  Please change the release number every time you modify your spec/srpm
  to avoid confusion.
Comment 6 Mamoru TASAKA 2008-11-16 10:23:07 EST
ping?
Comment 7 Dodji Seketeli 2008-11-19 15:21:13 EST
Pong,

Thank you very much for the review. I am going to go through it and fix things as requested.

Cheers.
Comment 8 Dodji Seketeli 2008-11-19 17:35:03 EST
So I have updated the spec file and srpm following your recommendations.

The new spec file is at http://www.seketeli.org/dodji/rpms/cloog/cloog.spec-0.15-0.2.git

The new srpm is at http://www.seketeli.org/dodji/rpms/cloog/cloog-0.15-0.2.git57a0bc.fc10.src.rpm.

Thanks again for your review.
Comment 9 Mamoru TASAKA 2008-11-20 13:32:03 EST
For -0.2

! First of all, you can try to check your package by rpmlint
  (in rpmlint package). rpmlint detects some general packaging
  issues.

Then:
* Requires
-----------------------------------------------------------
Requires: ppl-devel >= 0.10, gmp-devel >= 4.1.3
-----------------------------------------------------------
  - This should be for cloog-devel package, not for cloog package

* Group
  - Group for -devel subpackage should be "Development/Libraries",
    not "Development/Library"

* %_infodir/dir
  - This file must not be installed. Overwriting this file
    by this rpm breaks system "info" information.

    * Please remove this file at %install (not %clean)
      and remove this file from %files
    * And restore the previous %clean

* Shipping static archives
  - What I meant is that unless some specific reason static
    archive libfoo.a must be removed (as well as libtool .la
    file)

* calling ldconfig on scriptlet
  - When using "%post -p /sbin/ldconfig" (i.e. using /sbin/ldconfig
    directly instead of calling bash and executing ldconfig in the
    shell script), no other additional scriptlets are allowed.

    i.e. if there is some other scriptlets than /sbin/ldconfig,
    you have to write like:
----------------------------------------------------------
%post
/sbin/ldconfig
/sbin/install-info %{_infodir}/%{name}.info %{_infodir}/dir || :
----------------------------------------------------------

* %changelog
  - git revision differs between %release and %change.log

? Some header files design flaw
  - Well, for example the head of %_includedir/cloog/cloog.h
    says:
----------------------------------------------------------
    40  #ifndef CLOOG_H
    41  #define CLOOG_H
    42  
    43  #ifdef CLOOG_PPL_BACKEND
    44  # define GNUMP
    45  # include<cloog/ppl_backend.h>
    46  #else
    47  # include <polylib/missing.h>
    48  # include<cloog/polylib_backend.h>
    49  #endif
----------------------------------------------------------
    However, where can we tell if CLOOG_PPL_BACKEND is defined
    or not (when this package was built) (i.e whether this header
    file includes ppl_backend.h or polylib_backend.h)?
    build.log shows that when rebuilding this package -DCLOOG_PPL_BACKEND
    is used, however installed header files does not save such
    information....
Comment 10 Mamoru TASAKA 2008-12-01 08:23:52 EST
ping?
Comment 11 Mamoru TASAKA 2008-12-10 10:29:04 EST
ping again?
Comment 12 Dodji Seketeli 2008-12-15 15:33:43 EST
Sorry for my late reply.

The updated spec file is at http://www.seketeli.org/dodji/rpms/cloog/cloog.spec.0.15-0.2.git57a0bc.
and the updated srpm is at http://www.seketeli.org/dodji/rpms/cloog/cloog-0.15-0.2.git57a0bc.fc10.src.rpm.

Please find below my answers to your review.

Thanks.

>For -0.2
>
>! First of all, you can try to check your package by rpmlint
>  (in rpmlint package). rpmlint detects some general packaging
>  issues.

Done. I am still getting those two errors, but I am not sure how to fix them:

cloog.x86_64: W: shared-lib-calls-exit /usr/lib64/libcloog.so.0.0.0 exit@GLIBC_2.2.5
cloog-devel.x86_64: W: no-documentation

I'd appreciate any help there.

>
>Then:
>* Requires
>-----------------------------------------------------------
>Requires: ppl-devel >= 0.10, gmp-devel >= 4.1.3
>-----------------------------------------------------------
>  - This should be for cloog-devel package, not for cloog package

Fixed.

>
>* Group
>  - Group for -devel subpackage should be "Development/Libraries",
>    not "Development/Library"

Fixed.

>* %_infodir/dir
>  - This file must not be installed. Overwriting this file
>    by this rpm breaks system "info" information.
>
>    * Please remove this file at %install (not %clean)
>      and remove this file from %files

Done.

>    * And restore the previous %clean

Done.

>* Shipping static archives
>  - What I meant is that unless some specific reason static
>    archive libfoo.a must be removed (as well as libtool .la
>    file)

I guess you meant that for the -devel package.
I added an exclude for the .a library as well as for the .la files.

>
>* calling ldconfig on scriptlet
>  - When using "%post -p /sbin/ldconfig" (i.e. using /sbin/ldconfig
>    directly instead of calling bash and executing ldconfig in the
>    shell script), no other additional scriptlets are allowed.
>
>    i.e. if there is some other scriptlets than /sbin/ldconfig,
>    you have to write like:
>----------------------------------------------------------
>%post
>/sbin/ldconfig
>/sbin/install-info %{_infodir}/%{name}.info %{_infodir}/dir || :
>----------------------------------------------------------

Done.

>
>* %changelog
>  - git revision differs between %release and %change.log

Fixed.

>
>? Some header files design flaw
>  - Well, for example the head of %_includedir/cloog/cloog.h
>    says:
>----------------------------------------------------------
>    40  #ifndef CLOOG_H
>    41  #define CLOOG_H
>    42  
>    43  #ifdef CLOOG_PPL_BACKEND
>    44  # define GNUMP
>    45  # include<cloog/ppl_backend.h>
>    46  #else
>    47  # include <polylib/missing.h>
>    48  # include<cloog/polylib_backend.h>
>    49  #endif
>----------------------------------------------------------
>    However, where can we tell if CLOOG_PPL_BACKEND is defined
>    or not (when this package was built) (i.e whether this header
>    file includes ppl_backend.h or polylib_backend.h)?
>    build.log shows that when rebuilding this package -DCLOOG_PPL_BACKEND
>    is used, however installed header files does not save such
>    information....

How can I fix this ? I mean this is an upstream problem. Do you mean I should append a patch to the package ?
I could as well ship the package as is, and submit a patch upstream to fix it ?
Comment 13 Mamoru TASAKA 2008-12-16 11:13:28 EST
Well, would you change the release number every time you modify your
spec file?

(In reply to comment #12)
> cloog.x86_64: W: shared-lib-calls-exit /usr/lib64/libcloog.so.0.0.0
> exit@GLIBC_2.2.5

- This library actually calls exit() in the library (e.g.
  source/names.c). This is unusual situation. Usually when some unexpected
  behavior happens in a function in a library, the function should
  return a value which tells the error or so and should call
  exit() ($ rpmlint -I shared-lib-calls-exit shows the explanation).

  Would you contact upstream?

> cloog-devel.x86_64: W: no-documentation
- This warning can be ignored.
> I'd appreciate any help there.

> >? Some header files design flaw
> >  - Well, for example the head of %_includedir/cloog/cloog.h
> >    says:
> >----------------------------------------------------------
> >    40  #ifndef CLOOG_H
> >    41  #define CLOOG_H
> >    42  
> >    43  #ifdef CLOOG_PPL_BACKEND
> >    44  # define GNUMP
> >    45  # include<cloog/ppl_backend.h>
> >    46  #else
> >    47  # include <polylib/missing.h>
> >    48  # include<cloog/polylib_backend.h>
> >    49  #endif
> >----------------------------------------------------------
> >    However, where can we tell if CLOOG_PPL_BACKEND is defined
> >    or not (when this package was built) (i.e whether this header
> >    file includes ppl_backend.h or polylib_backend.h)?
> >    build.log shows that when rebuilding this package -DCLOOG_PPL_BACKEND
> >    is used, however installed header files does not save such
> >    information....
> 
> How can I fix this ? I mean this is an upstream problem. Do you mean I should
> append a patch to the package ?
> I could as well ship the package as is, and submit a patch upstream to fix it ?

- For this issue, I don't think this issue can be unresolved (I won't
  approve this package unless this is fixed). At least a patch should be appended
  or so and also this must be fixed upstream.
Comment 14 Mamoru TASAKA 2009-01-05 12:03:24 EST
ping?
Comment 15 Mamoru TASAKA 2009-01-18 09:29:25 EST
ping again?
Comment 16 Mamoru TASAKA 2009-01-30 10:05:38 EST
Again ping?
Comment 17 Dodji Seketeli 2009-02-05 16:01:51 EST
Created attachment 331051 [details]
Adds a cloog-config.h file to the distribution

This patch creates a cloog-config.h file that contains the configuration macros for this package. It then removes the need to have -DFOO configuration macros passed at compile time. I guess it should fix the problem you raised with the cloog/cloog.h header as the user can know how the cloog library was configured.

If this approach is okay with you I will forward the patch to upstream.
Comment 18 Dodji Seketeli 2009-02-05 16:04:23 EST
Created attachment 331052 [details]
pass make distcheck

This patch removes some files left in builddir after make clean. Those where preventing make distcheck to pass. I am stacking this here because I'd like to forward it to upstream. At least it won't be lost if it's here.
Comment 19 Dodji Seketeli 2009-02-05 16:10:21 EST
> This library actually calls exit() in the library (e.g.
> source/names.c). This is unusual situation. Usually when some unexpected
> behavior happens in a function in a library, the function should
> return a value which tells the error or so and should call
> exit() ($ rpmlint -I shared-lib-calls-exit shows the explanation).

> Would you contact upstream?

Yes, sure. Though is this a blocker for the package to be sponsored ? I mean this is typically something that can be fixed in a subsequent upstream release. The reason why I am being a bit dense on this is that there are lots of exit() calls in the library and changing those would require some real API design changes. Otherwise, I would have provided a patch myself already.

I will batch forward this issue to upstream with the patch https://bugzilla.redhat.com/attachment.cgi?id=331052 when you approve it.

Thanks for your time.
Comment 20 Mamoru TASAKA 2009-02-06 10:59:15 EST
(In reply to comment #17)
> Created an attachment (id=331051) [details]
> Adds a cloog-config.h file to the distribution

Seems good.

(In reply to comment #19)
> > This library actually calls exit() in the library (e.g.
> > source/names.c). This is unusual situation. Usually when some unexpected
> > behavior happens in a function in a library, the function should
> > return a value which tells the error or so and should call
> > exit() ($ rpmlint -I shared-lib-calls-exit shows the explanation).
> 
> > Would you contact upstream?
> 
> Yes, sure. Though is this a blocker for the package to be sponsored ? 

Not a blocker.
Comment 21 Dodji Seketeli 2009-02-09 06:24:07 EST
I sent the patches to upstream. I am waiting for comments.
Comment 22 Dodji Seketeli 2009-02-10 05:57:12 EST
Upstream accepted the patches.They should properly appear in the upstream git soon.
Comment 23 Dodji Seketeli 2009-02-10 06:35:36 EST
Hey Mamoru,

I am thinking that maybe I should change the name of the package from cloog to cloog-ppl, as this branch of the package is really intended to be compiled with the ppl backend only.

When compiled with say the polylib backend, it becomes another library as include/cloog/polylib_backend.h is then included by cloog.h and induces a new API.
In that case a new package cloog-polylib would then be generated.

Also, cloog-ppl (that is at http://repo.or.cz/w/cloog-ppl.git) really is a special branch of the cloog tree (that is at http://repo.or.cz/w/cloog.git) where the ppl backend branch work is happening. It will be later merged back into the cloog main tree, after gcc 4.4 is released. So gcc 4.4 will be based on cloog-ppl, whereas gcc 4.5 is likely to be based on cloog, after cloog-ppl is merged back into the main tree.

What do you think about this ?
Comment 24 Mamoru TASAKA 2009-02-10 07:35:33 EST
Well, do you want to "re"submit a new review request for ppl
once cloog-ppl is merged to ppl main tree? When the srpm name is
to be renamed, we have to submit another review request for it.

If not, I think the less troublesome way is to create
cloog-ppl(-devel) binary rpms from ppl srpm.
Comment 25 Mamoru TASAKA 2009-02-10 07:41:23 EST
(In reply to comment #24)
> If not, I think the less troublesome way is to create
> cloog-ppl(-devel) binary rpms from ppl srpm.

Of course "from cloog srpm", sorry...
Comment 26 Dodji Seketeli 2009-02-10 17:32:08 EST
Okay,

So I tried to do what you proposed. I now generate cloog-ppl and cloog-ppl-devel packages from the ppl srpm.

spec: http://people.redhat.com/dseketel/rpms/cloog/cloog.spec.0.15-0.3.gitad322
srpm: http://people.redhat.com/dseketel/rpms/cloog/cloog-0.15-0.3.gitad322.fc10.src.rpm
Comment 27 Mamoru TASAKA 2009-02-13 10:42:01 EST
Well:

* Not a blocker, however "rpm -ivv cloog-ppl" bears some warning on
  info file:
---------------------------------------------------------
D: adding 4 entries to Filedigests index.
D:   install: %post(cloog-ppl-0.15-0.3.gitad322.fc11.i386) synchronous scriptlet start
D:   install: %post(cloog-ppl-0.15-0.3.gitad322.fc11.i386)      execv(/bin/sh) pid 5361
+ /sbin/ldconfig
+ /sbin/install-info /usr/share/info/cloog.info /usr/share/info/dir
install-info: warning: no info dir entry in `/usr/share/info/cloog.info'
---------------------------------------------------------

* Usually for a packager needing sponsorship, I request him/her to
  either
  - submit another review request
  - or do a (at least one) pre-review of other person's review
    request
  as written on
  http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored .

  Do you have another review request of yours or review requests
  on which you did some pre-review?

Note:
Fedora package collection review requests which are waiting for someone to
review can be checked on my wiki page:
http://fedoraproject.org/wiki/User:Mtasaka#B._Review_request_tickets
(Check "No one is reviewing")
Comment 28 Dodji Seketeli 2009-02-13 14:40:33 EST
For the info problem, I will submit a bug report to upstream.
As for the second review request, I have just filed a new one at https://bugzilla.redhat.com/show_bug.cgi?id=485496.
Comment 29 Mamoru TASAKA 2009-02-15 11:12:20 EST
Now approving.

--------------------------------------------------------
   This package (cloog) is APPROVED by mtasaka
--------------------------------------------------------
Comment 30 Dodji Seketeli 2009-02-15 12:18:03 EST
Thanks.
Comment 31 Dodji Seketeli 2009-02-15 12:21:20 EST
New Package CVS Request
=======================
Package Name: cloog
Short Description: The Chunky Loop Generator
Owners: dodji
Branches: F-10
InitialCC: dodji
Comment 32 Kevin Fenzi 2009-02-16 16:21:44 EST
cvs done.
Comment 33 Mamoru TASAKA 2009-02-18 12:12:03 EST
By the way, can this package be rebuilt on F-10?
F-10 ppl is still 0.9-25.fc10.

If you won't build this package on F-10, please close this
bug as NEXTRELEASE.
Comment 34 Dodji Seketeli 2009-02-18 12:42:53 EST
No, it can't. It depends on ppl 0.10 that is available on the coming F-11 only. The only envisionned consumer of cloog-ppl is gcc 4.4 for now anyway.