Bug 165666 - Review Request: Graph Visualization Tools
Review Request: Graph Visualization Tools
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Greg DeKoenigsberg
David Lawrence
http://filelister.linux-kernel.at/mod...
:
Depends On: 163840
Blocks:
  Show dependency treegraph
 
Reported: 2005-08-11 05:28 EDT by Oliver Falk
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-06-27 11:40:25 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
proposed graphviz-cairo.spec for Extras CVS (1.80 KB, text/plain)
2006-01-05 11:47 EST, John Ellson
no flags Details

  None (edit)
Description Oliver Falk 2005-08-11 05:28:57 EDT
Spec Name or Url: http://filelister.linux-kernel.at/downloads/packages/FC_EXTRAS_APPROVAL/graphviz-cairo/graphviz-cairo.spec
SRPM Name or Url: http://filelister.linux-kernel.at/downloads/packages/FC_EXTRAS_APPROVAL/graphviz-cairo/graphviz-cairo-2.4-1.src.rpm
Description:
A renderer plugin for graphviz that uses cairo and provides output formats
such as png, x11.
Comment 1 Ralf Corsepius 2005-08-11 06:00:41 EDT
Not a formal review, just some remarks on the spec files:

- BuildRoot not conforming to FE conventions:
BuildRoot:		%{_tmppath}/%{name}-root

- Questionable:
%{expand: %%define optflags %{optflags} -ffast-math}

- Probably wrong:
%configure ....
  transform='s,x,x,' \
[If this is required, the configure script is utterly broken.]

This is twice questionable:
1. Using -ffast-math is questionable by itself. I recommend against it.
2. The expand. Pass CFLAGS="$RPM_OPT_FLAGS <more>" to configure instead.

- Missing
Requires(post): %{_bindir}/dot
Requires(postun): %{_bindir}/dot
Comment 2 Oliver Falk 2005-08-11 06:52:29 EDT
* BuildRoot fixed
* Added comment to the expand, as in graphviz.spec:
  # XXX ix86 only used to have -ffast-math, let's use everywhere
* configure switched to the one provided within the source tarball specfile
* the transform stuff is from the the source tarball specfile

Updated spec and srpm.
Comment 3 Oliver Falk 2005-08-22 07:06:11 EDT
Please check build 3.(In reply to comment #1)
> Not a formal review, just some remarks on the spec files:
> 
> - BuildRoot not conforming to FE conventions:
> BuildRoot:		%{_tmppath}/%{name}-root

Fixed.
 
> - Questionable:
> %{expand: %%define optflags %{optflags} -ffast-math}

We also have this for graphviz itself. It comes from the original specfile
provided within the tarball.

> - Probably wrong:
> %configure ....
>   transform='s,x,x,' \
> [If this is required, the configure script is utterly broken.]

Stays as we also use it for graphviz package.

> This is twice questionable:
> 1. Using -ffast-math is questionable by itself. I recommend against it.
> 2. The expand. Pass CFLAGS="$RPM_OPT_FLAGS <more>" to configure instead.
> 
> - Missing
> Requires(post): %{_bindir}/dot
> Requires(postun): %{_bindir}/dot

Fixed.


Please see build 3. Same URL.
Comment 4 Ralf Corsepius 2005-08-25 05:12:13 EDT
(In reply to comment #3)
> Please check build 3.(In reply to comment #1)

> > - Probably wrong:
> > %configure ....
> >   transform='s,x,x,' \
> > [If this is required, the configure script is utterly broken.]
> 
> Stays as we also use it for graphviz package.
If it is required for graphviz, graphviz is broken, too.
Comment 5 Oliver Falk 2005-08-29 07:30:29 EDT
Specfile cleanup and update. Please have a look again.
Comment 6 Oliver Falk 2005-10-18 02:41:26 EDT
Ralf, you you have a look again plz.
Comment 7 Oliver Falk 2005-10-18 02:41:59 EDT
Ralf, could you have a look again plz.
Comment 8 Orion Poplawski 2006-01-03 16:50:08 EST
Oliver - I can't find any of your src rpms.  Please repost a URL.
Comment 10 John Ellson 2006-01-05 09:23:17 EST
Excuse me if there is pending progress, but I'm sensing a deadlock here?

Is the review not happening because graphviz-cairo in not in Fedora CVS, or is
graphviz-cairo not in Fedora CVS because of problems found in the review of the
upstream code?
Comment 11 Ralf Corsepius 2006-01-05 09:46:46 EST
(In reply to comment #10)
> .. or is
> graphviz-cairo not in Fedora CVS because of problems found in the review of the
> upstream code?

As far as my comment in
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=165666#c4 is concerned,  it
still applies to the current package. 

I regret having to say this, but *I* am not going to approve this package
without this issue having been fixed. This doesn't mean I will block others from
approving this package.
Comment 12 Michael Schwendt 2006-01-05 09:57:09 EST
You should _at least_ try to convince Ralf as why something he points
out as being "questionable" or "broken" is found in the packaging and/or
in upstream's tarball.
Comment 13 John Ellson 2006-01-05 10:07:38 EST
That bug in comment#4 was in the graphviz package, not graphviz-cairo, and it
was fixed long ago in the devel and FC-* spec - although perhaps it was not
fixed properly in the RHL-8, RHL-9 specs ??  Oliver?

Fedora Extras CVS currently contains:
ellson@ontap:graphviz> grep transform */*spec
FC-1/graphviz.spec:- Removed "transform='s,x,x,'" configure arg
FC-2/graphviz.spec:- Removed "transform='s,x,x,'" configure arg
FC-3/graphviz.spec:- Removed "transform='s,x,x,'" configure arg
FC-4/graphviz.spec:- Removed "transform='s,x,x,'" configure arg
RHL-8/graphviz.spec:    transform='s,x,x,' \
RHL-8/graphviz.spec:- Removed "transform='s,x,x,'" configure arg
RHL-9/graphviz.spec:    transform='s,x,x,' \
RHL-9/graphviz.spec:- Removed "transform='s,x,x,'" configure arg
devel/graphviz.spec:- Removed "transform='s,x,x,'" configure arg


I suppose the bug may still exist in the .spec in the upstream src.rpm
for graphviz-2.6, but that spec isn't used in Fedora, and the bug has definitely
been fixed in the HEAD sources.

graphviz-2.6 is already in extras.  This discussion was supposed to be about
graphviz-cairo which is not yet in extras.
Comment 14 Ralf Corsepius 2006-01-05 11:00:10 EST
(In reply to comment #13)
> That bug in comment#4 was in the graphviz package, not graphviz-cairo, 
Well, let me cite graphiz-cairo.spec I d/l'ed from the URLs you gave in comment
#9, before posting comment #11:
...
make \
    DESTDIR=$RPM_BUILD_ROOT \
    docdir=$RPM_BUILD_ROOT%{_docdir}/%{name} \
    transform='s,x,x,' \
    install
...

All you are doing here is to move the "transform" from configuration-time to
install-time.

The actual problem is this package not applying GNU canonicialization standards
correctly.

Comment 15 John Ellson 2006-01-05 11:27:43 EST
I see, so I apologise, I didn't realize it was in that version of
graphviz-cairo's .spec too.

graphviz-cairo-2.6 is what it is and we're not quite ready for the next stable
release upstream yet.

Can this problem be fixed in the same way that it was for graphviz-2.6?  i.e by 
fixing it in the Extras CVS version of graphviz-cairo.spec (which is supercedes
the .spec in the package anyway)?

How can graphviz-cairo be loaded into Extras CVS so that we can get past this?
Comment 16 John Ellson 2006-01-05 11:47:37 EST
Created attachment 122835 [details]
proposed graphviz-cairo.spec for Extras CVS
Comment 17 John Ellson 2006-03-07 12:34:10 EST
graphviz-cairo-2.8 is now available:
    http://www.graphviz.org/pub/graphviz/ARCHIVE/graphviz-cairo-2.8-1.src.rpm

Comment 18 Orion Poplawski 2006-05-19 13:08:22 EDT
Is (In reply to comment #17)
> graphviz-cairo-2.8 is now available:
>     http://www.graphviz.org/pub/graphviz/ARCHIVE/graphviz-cairo-2.8-1.src.rpm

Is this the package intended to be submitted?  Oliver, are you still going to be
the maintainer?  At first glance:

- Non standard BuildRoot
- No changelog entries.
- What's up with __doc?
- spec file perm is 600
- no ldconfig


Comment 19 Jason Tibbitts 2006-06-23 19:10:27 EDT
Is anyone still interesting in keeping this submission going?  It's been over a
month since the last ping with no response.  If Oliver no longer wants to keep
this going but John wishes to submit his package to extras and be the
maintainer, then this bug should be closed and a new one opened.

I'll close in one week if there's no further activity.
Comment 20 John Ellson 2006-06-27 10:54:47 EDT
Go ahead and close this one.

I should have a graphviz-2.10 release in the next few weeks, and that
will include the graphviz-cairo features instead of shipping them in a separate
source rpm.

I've not heard anything from Oliver recently, but I still appeciate an
independent set of eyes on graphviz during the release process, so I hope he is
still willing?

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