Bug 184331 - Review Request: k3d - 3D modeling and rendering system
Review Request: k3d - 3D modeling and rendering system
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Paul F. Johnson
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-03-07 18:56 EST by Denis Leroy
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-08-11 04:53:06 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)
rpmlint output once installed (26.01 KB, text/plain)
2006-07-28 16:17 EDT, Paul F. Johnson
no flags Details

  None (edit)
Description Denis Leroy 2006-03-07 18:56:31 EST
Spec Name or Url: http://www.poolshark.org/src/k3d.spec
SRPM Name or Url: http://www.poolshark.org/src/k3d-0.5.0.39-1.src.rpm

Description: 

K-3D is a complete 3D modeling, animation and rendering system. K-3D
features a robust, object oriented plugin architecture, designed to
scale to the needs of professional artists. It is designed from the
ground up to generate motion picture quality animation using RenderMan
compliant render engines.

Notes:

Look at this beautiful list of BuildRequires. This is living on the edge: I'm told by a K-3D developer that this might be the first attempt at running this monster with g++ 4.1, so there might be some rough edges.

rpmlint will complain about header files in the main RPM (in /usr/share): as far as i understand this is a false positive, though if a K3D power-user could confirm i'd appreciate.
Comment 1 Gérard Milmeister 2006-03-16 13:51:18 EST
(In reply to comment #0)
> rpmlint will complain about header files in the main RPM (in /usr/share): as
far as i understand this is a false positive, though if a K3D power-user could
confirm i'd appreciate.
These are included by for shaders and thus and integral part of the package,
I would say.
I did not look closely at your spec file, but here is the spec file I used
for building the package. May be you find something useful.
http://math.ifi.unizh.ch/fedora/spec/k3d.spec
Comment 2 Gérard Milmeister 2006-05-19 03:56:50 EDT
* Could you update to 0.5.10.0?
* gts can be used, but is not yet in Extras
  I suggest you submit gts for review and make it a dependency for k-3d
  The review process of this package will be quick, I think.
* There is also a dependency on SuperLU, I don't know how
  crucial it is.
Comment 3 Dan Horák 2006-05-19 04:20:43 EDT
gts was just submited a few hours ago as
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=192363
Comment 4 Gérard Milmeister 2006-05-23 05:23:18 EDT
gts is now released in Extras.
Please update the srpm to the new version with gts included.
Comment 5 Dan Horák 2006-07-18 11:29:05 EDT
Did you see Fedora packages at http://www.planetsaphire.com/rpms/k3d/?
Comment 6 Denis Leroy 2006-07-18 12:03:41 EDT
Will look at it. My development system is in a container in a boat in the middle
of the atlantic, so I won't be able  to work on this package for another month
or so. K3D would take about 24 hours to compile on this old laptop :-(
Comment 7 Paul F. Johnson 2006-07-25 18:53:15 EDT
Can you post the URLs of both the spec and srpm here so I can just download,
test, review and we can get this one off into FE?
Comment 8 Denis Leroy 2006-07-26 12:23:45 EDT
Spec: http://www.poolshark.org/src/k3d.spec
SRPM: http://www.poolshark.org/src/k3d-0.5.15.0-1.src.rpm

- Updated to latest version
- I folded the devel package into the main package since there's not much sense
in having the split (since otherwise the main package would have to Require it).
- Added gts support
Comment 9 Paul F. Johnson 2006-07-26 17:09:38 EDT
Comments:

%post
/sbin/ldconfig

You don't have ldconfig in the requires. You can get around this with 

%post -p /sbin/ldconfig

%{_libdir}/*.so.*

Not overly happy with this. If the package is generating some libs which all
start with a common name (such a libfoo), then the libdir entry can be changed
to libfoo*.so.*

The same applies for the regex. Having %bindir/k3d* is enough (though you will
need %exclude %{_bindir}/k3d-config

I'm slightly confused by

%{_libdir}/*.so.*
%{_libdir}/k3d

Is the application putting libraries into both libdir and libdir/k3d?

In the -devel you have

%{_libdir}/*.so

As before, can you alter this to be a real name?
Comment 10 Ralf Corsepius 2006-07-27 00:00:16 EDT
(In reply to comment #8)
> - I folded the devel package into the main package since there's not much sense
> in having the split (since otherwise the main package would have to Require it).

Very bad move. Please revert this change and split into *-devel and *non-devel,
again.
Comment 11 Paul Howarth 2006-07-27 03:38:33 EDT
(In reply to comment #10)
> (In reply to comment #8)
> > - I folded the devel package into the main package since there's not much sense
> > in having the split (since otherwise the main package would have to Require it).
> 
> Very bad move. Please revert this change and split into *-devel and *non-devel,
> again.

If the main package would have to require it, it sounds like the files in the
devel package (at least some of them) aren't really devel files.

What was in the devel package that's needed by the main package? Is there
anything that was in the devel package that's *not* needed by the main package?
Comment 12 Ralf Corsepius 2006-07-27 16:26:13 EDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > - I folded the devel package into the main package since there's not much
sense
> > > in having the split (since otherwise the main package would have to
Require it).
> > 
> > Very bad move. Please revert this change and split into *-devel and *non-devel,
> > again.
> 
> If the main package would have to require it, it sounds like the files in the
> devel package (at least some of them) aren't really devel files.
This package seems to contain shared libs, dll-modules/plugins and headers.

> What was in the devel package that's needed by the main package? Is there
> anything that was in the devel package that's *not* needed by the main
> package?
I presume the OP had mixed up *.so symlinks and dll-modules/plugins.
Comment 13 Paul F. Johnson 2006-07-28 15:36:47 EDT
Apologies for the delay - my buildsys is somewhat elderly! It built fine in mock
(x86).

I'll go over everything else tonight.
Comment 14 Paul F. Johnson 2006-07-28 16:17:37 EDT
Created attachment 133264 [details]
rpmlint output once installed
Comment 15 Paul F. Johnson 2006-07-28 16:18:27 EDT
You need to fix the problems highlighted in #14 before the review goes any further
Comment 16 Denis Leroy 2006-08-01 12:02:33 EDT
SPEC: http://www.poolshark.org/src/k3d.spec
SRPM: http://www.poolshark.org/src/k3d-0.5.15.0-2.src.rpm

- I cleaned up the %files section a bit, as you requested. The package does
indeed have both libraries in /usr/lib and /usr/lib/k3d (those are plugins).

- For the undefined-non-weak-symbol warnings. I believe this is intentional. See
here: http://www.redhat.com/archives/fedora-extras-list/2006-July/msg00569.html
Essentially you are getting the warnings because the package dependencies are
not installed. As for the weak linking, I believe it is intentional to allow k3d
libs to link to either libGL.so from Xorg or from Nv***a.

- For the devel vs non-devel. Part of the package can be considered a devel
package, but I dont think it does what a traditional devel package would do. The
reasons has to do with what K-3D does: it's merely a modeler, the first step
before calling the main rendering engine (e.g. aqsis, which I'll try to package
next), and that step involves using the devel-type files. So the devel files are
required by the main package. This is explicitely mentioned in the project page
at http://www.k-3d.org/wiki/GettingStarted , as well as in comment #1 above. I
brought up the issue on a fedora mailing list a while ago, but I can't seem to
track it down in the archives. I remember the consensus was that it made little
sense to split a package if both parts require each other. OTOH I can't say I
have a strong opinion on the matter...
Comment 17 Paul F. Johnson 2006-08-01 17:11:06 EDT
The devel package are files required to build or develop the package. These are
headers, .so files and compiling documentation.

This package requires a -devel package
Comment 18 Denis Leroy 2006-08-02 09:03:21 EDT
SPEC: http://www.poolshark.org/src/k3d.spec
SRPM: http://www.poolshark.org/src/k3d-0.5.15.0-3.src.rpm

Okay then, I resplit the package, and added 'Requires: k3d-devel'.

There are still some header files in the main package, under /usr/share/, but
those are false positives (see comment #1).
Comment 19 Paul F. Johnson 2006-08-05 15:10:31 EDT
  --mode 644 \

Is this required for the desktop icon?

Have you asked upstream if these "false positives" are exactly that or if they
should be held in the -devel package?
Comment 20 Paul F. Johnson 2006-08-05 16:48:39 EDT
rpmlint is giving in the main package

E: k3d devel-dependency k3d-devel

This will cause a lot of problems. How can the binary require the devel?

Also has the devel-file-in-non-devel warnings

The devel package contains a pile of warnings about dangling-relative-symlinks
and more importantly

E: k3d-devel only-non-binary-in-usr-lib

The debuginfo package has lots of errors

E k3d-debuginfo script-without-shellbang
/usr/src/debug/k3d-0.5.15.0/k3dsdk/nodes.cpp (as an example)

These errors are normally a permissions issue, they need to be chmod to 0644 in
the after the archive has been dearchived.

rpmlint reports the SRPM to be clean.

I've not built it in mock yet due to these errors.
Comment 21 Denis Leroy 2006-08-07 17:46:39 EDT
Spec: http://www.poolshark.org/src/k3d.spec
SRPM: http://www.poolshark.org/src/k3d-0.5.16.0-1.src.rpm

Paul, thanks for your patience and for your comments. I was able to borrow a
build system and do a little more testing, and I also exchanged a few emails
with upstream developers. Comments embedded below.

> --mode 644  : Is this required for the desktop icon?

No, about 5% of extras packages specify it explicitely. I removed it.

> E: k3d devel-dependency k3d-devel

Ok, I checked with upstream and the good news is that this requirement can be
dropped (they updated the wiki to clarify). So this is fixed.

> Also has the devel-file-in-non-devel warnings

Those are false positives, as confirmed by upstream:

Timothy M. Shead <tshead@k-3d.com> Wrote:
"The .h files in /share/shaders are part of the shader source code, not
the K-3D source code.  They are there so that shaders can be compiled
*at runtime* by the user's RenderMan engine.  So they belong in the
runtime package."

> E debuginfo script-without-shellbang
> /usr/src/debug/k3d-0.5.15.0/k3dsdk/nodes.cpp (as an example)

Fixed.

> The devel package contains a pile of warnings about 
> dangling-relative-symlinks

Right, this occurs when a  package uses library symlinks in a subdirectory of
/usr/lib (those in /usr/lib are ignored by rpmlint). For example, 'graphviz' is
another package that has the same problem. Anyways, this actually was an error
in the spec as the run-time package requires the .so files, so I had to move
them into the main package. K-3D will not load the plugins correctly if only the
.so.0.0.0 files are present. There are a couple of libs in /usr/lib that also
need their .so files in the main package, because they are dlopened directly by
k3d-bin. I think those really belong in /usr/lib/k3d and I've reported the
problem upstream, a fix would require a pretty intrusive autoconf/automake patch
which i think is not worth the effort.

> E: k3d-devel only-non-binary-in-usr-lib

Should no longer occur, as /usr/lib/k3d is in the main package now.

Should build cleanly in mock, for devel and FC-5. FC-4 requires a small BR
change, but otherwise also builds cleanly.
Comment 22 Paul F. Johnson 2006-08-09 16:35:37 EDT
Okay

Good

Builds fine and actually works
rpmlint is clean on all packages (with the exceptions listed above which I'm
willing to accept)
No dupes in the BRs
Includes documentation
No issues with directory ownership

Bad

Needs pkgconfig adding to the R: on the devel
You've packaged the same README in the main and devel package

Unsure
find -type f -regex '.*\.\(cpp\|h\|svg\)' -perm +111 -exec chmod -x {} ';' -
seems like overkill to me.

Fix the two bads (which aren't that bad!), and it's good to go
Comment 23 Denis Leroy 2006-08-10 03:53:44 EDT
SPEC: http://www.poolshark.org/src/k3d.spec
SRPM: http://www.poolshark.org/src/k3d-0.5.16.0-2.src.rpm

> Needs pkgconfig adding to the R: on the devel

Actually, k3d doesn't provide any pkgconfig .pc file, it provides its own
'k3d-config' executable.

> You've packaged the same README in the main and devel package

Fixed.

> find -type f -regex '.*\.\(cpp\|h\|svg\)' -perm +111 -exec chmod -x {} ';' -

Ah the lovely find command. Well I'd still rather do that than do a massive
chmod -x over the whole tree since it contains all sorts of executable scripts
and whatnot. Or did you have something even simpler in mind ?
Comment 24 Paul F. Johnson 2006-08-10 05:21:46 EDT
Okay, I'm happy with that. 

Is it worth adding the tests directory into the -devel directory as well as the
docs/xml/sample_document.k3d file? Might be a useful addition.
Comment 25 Denis Leroy 2006-08-10 06:47:03 EDT
Not a bad idea but the test directory is not easily packagable, we'd have to
provide custom Makefiles... We could add the gzip'ed sample file to the devel
package, or even to the main package, i think it's a good idea.

Is this a must item or did you approve the package ?
Comment 26 Paul F. Johnson 2006-08-10 13:37:37 EDT
Add it to the devel package and it's good to go.
Comment 27 Denis Leroy 2006-08-11 02:36:20 EDT
SPEC: http://www.poolshark.org/src/k3d.spec
SRPM: http://www.poolshark.org/src/k3d-0.5.16.0-3.src.rpm

Paul, thanks for your review!
Comment 28 Paul F. Johnson 2006-08-11 03:11:58 EDT
Thanks for making the addition

APPROVED
Comment 29 Denis Leroy 2006-08-11 04:53:06 EDT
Built :-)
Comment 30 Kevin Fenzi 2006-12-21 22:07:21 EST
Changing the Summary for tracking purposes. 

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