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.
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
* 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.
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.
(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.
Specfile cleanup and update. Please have a look again.
Ralf, you you have a look again plz.
Ralf, could you have a look again plz.
Oliver - I can't find any of your src rpms. Please repost a URL.
The upstream sources are here. Graphviz-2.6 is already in Extras. I think graphviz-cairo is not yet in Extras CVS. http://www.graphviz.org/pub/graphviz/ARCHIVE/graphviz-2.6.tar.gz http://www.graphviz.org/pub/graphviz/ARCHIVE/graphviz-2.6.tar.gz.md5 http://www.graphviz.org/pub/graphviz/ARCHIVE/graphviz-2.6-1.src.rpm http://www.graphviz.org/pub/graphviz/ARCHIVE/graphviz-cairo-2.6.tar.gz http://www.graphviz.org/pub/graphviz/ARCHIVE/graphviz-cairo-2.6.tar.gz.md5 http://www.graphviz.org/pub/graphviz/ARCHIVE/graphviz-cairo-2.6-1.src.rpm
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?
(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.
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.
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.
(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.
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?
Created attachment 122835 [details] proposed graphviz-cairo.spec for Extras CVS
graphviz-cairo-2.8 is now available: http://www.graphviz.org/pub/graphviz/ARCHIVE/graphviz-cairo-2.8-1.src.rpm
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
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.
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?