Bug 635511 - Review Request: assimp - Library to import various 3D model formats into applications
Review Request: assimp - Library to import various 3D model formats into appl...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tim Niemueller
Fedora Extras Quality Assurance
:
: 689639 (view as bug list)
Depends On:
Blocks: 725752
  Show dependency treegraph
 
Reported: 2010-09-19 23:40 EDT by Rich Mattes
Modified: 2012-07-03 16:51 EDT (History)
8 users (show)

See Also:
Fixed In Version: assimp-2.0.863-5.20110824svn.fc16
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-04-24 10:51:28 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tim: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
fast_atof speed and accuracy test code (4.69 KB, text/plain)
2010-11-17 14:36 EST, Tom "spot" Callaway
no flags Details
Example run of test code against unmodified Irrlicht 1.7.2 (5.79 KB, text/plain)
2010-11-17 14:37 EST, Tom "spot" Callaway
no flags Details

  None (edit)
Description Rich Mattes 2010-09-19 23:40:52 EDT
Spec URL: http://rmattes.fedorapeople.org/RPMS/assimp/assimp.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/assimp/assimp-1.1.700-1.fc13.src.rpm

Description: Assimp, the Open Asset Import Library, is a free library to import 
various well-known 3D model formats into applications.  Assimp aims 
to provide a full asset conversion pipeline for use in game 
engines and real-time rendering systems, but is not limited 
to these applications.

koji-f12: http://koji.fedoraproject.org/koji/taskinfo?taskID=2476590
koji-f13: http://koji.fedoraproject.org/koji/taskinfo?taskID=2476587
koji-f14: http://koji.fedoraproject.org/koji/taskinfo?taskID=2476597

$ rpmlint assimp* ../../SRPMS/assimp*assimp.x86_64: 
W: shared-lib-calls-exit /usr/lib64/libassimp.so.1.0.0 exit@GLIBC_2.2.5
assimp.x86_64: W: no-manual-page-for-binary assimp
assimp-devel.x86_64: W: no-documentation
assimp.src: W: invalid-url Source0: assimp-1.1.700.tar.bz2
4 packages and 0 specfiles checked; 0 errors, 4 warnings.
Comment 1 Ralf Corsepius 2010-09-20 00:43:27 EDT
2 remarks:

1) Your packages uses a repackaged source-package (*.tar.bz2), instead of the original sources (*.zip).

I'd recommend not doing so, because this adds complications in longer term maintenance of packages.

Instead I'd recommend you to use the original *.zip and to remove those files you don't want in %prep.


2) assimp contains doxygen devel docs.

Consider adding them.
Comment 2 Rich Mattes 2010-09-20 01:39:18 EDT
Thanks for taking a look Ralf.

I added the doxygen-generated API reference to the -devel package as documentation.  There are also doxygen docs for the assimp binary, but they don't contain any more information than running "assimp --help" does so I opted to exclude them.

Part of the reason I repackaged the archive was because the .zip file the project provides doesn't unzip cleanly into a single directory.  I've resolved this issue by using %setup -c, so the package now uses the project's .zip file.  Bundled libraries are removed in %prep for safety.

New spec and SRPM are located at:
Spec URL: http://rmattes.fedorapeople.org/RPMS/assimp/assimp.spec
SRPM URL:
http://rmattes.fedorapeople.org/RPMS/assimp/assimp-1.1.700-2.fc13.src.rpm

$ rpmlint ../RPMS/x86_64/assimp-* ../SRPMS/assimp-*
assimp.x86_64: W: shared-lib-calls-exit /usr/lib64/libassimp.so.1.0.0 exit@GLIBC_2.2.5
assimp.x86_64: W: no-manual-page-for-binary assimp
4 packages and 0 specfiles checked; 0 errors, 2 warnings.
Comment 3 Ralf Corsepius 2010-09-20 02:04:58 EDT
(In reply to comment #2)
> Thanks for taking a look Ralf.
> 
> I added the doxygen-generated API reference to the -devel package as
> documentation.  There are also doxygen docs for the assimp binary, but they
> don't contain any more information than running "assimp --help" does so I opted
> to exclude them.

> Part of the reason I repackaged the archive was because the .zip file the
> project provides doesn't unzip cleanly into a single directory. 

[FWIW: I implemented an assimp*spec for local use sometime last week]

My way to overcome this is:

%setup -q -c -T -n %{name}-%{version}
%setup -q -T -D -n %{name}-%{version} -a0

> New spec and SRPM are located at:
I intend to return to them later today.
Comment 4 Ralf Corsepius 2010-09-20 02:19:56 EDT
Partial review:

* Remove
BuildRequires: ccache

assimp does not build-require ccache.


* Remove from *-devel
Requires: pkgconfig cmake

assimp doesn't ship pkgconfig files nor cmake files.


* Consider building the doxygen docs from sources.

Apart of these, the package seems basically fine to me.

One outstanding issue, I haven't checked for yet: 
The *devel package's deps likely are incomplete.
Comment 5 Rich Mattes 2010-09-20 21:10:28 EDT
I've removed the extra BuildRequires and got the documentation building manually.  The new spec and srpm are at:

Spec URL: http://rmattes.fedorapeople.org/RPMS/assimp/assimp.spec
SRPM URL:
http://rmattes.fedorapeople.org/RPMS/assimp/assimp-1.1.700-3.fc13.src.rpm

$ rpmlint ../RPMS/x86_64/assimp-* ../SRPMS/assimp-*
assimp.x86_64: W: shared-lib-calls-exit /usr/lib64/libassimp.so.1.0.0 exit@GLIBC_2.2.5
assimp.x86_64: W: no-manual-page-for-binary assimp
4 packages and 0 specfiles checked; 0 errors, 2 warnings.
Comment 6 Ralf Corsepius 2010-09-24 10:04:16 EDT
Technically, the package is fine. I would approve it, if there weren't a legal detail:

The source *.zip contains data files ("artwork?") under different, some times not really clear licenses. AFAIS, they all qualify as "freely distributable", but some of them do not qualify a as "open source".

As the package doesn't install them, I am not sure if this is allowed in Fedora.

=> Blocking FE-LEGAL for review.
Comment 7 Rich Mattes 2010-10-12 09:02:25 EDT
It looks like you're referring to all of the model files under the "test" directory.I've asked upstream for clarification on the licensing of the test models via their SourceForge mailing list.  If it turns out they're not under a free license, would it be feasible to re-bundle the source without the test directory for the srpm?
Comment 8 Ralf Corsepius 2010-10-12 09:49:14 EDT
(In reply to comment #7)
> It looks like you're referring to all of the model files under the "test"
> directory.
Correct.

> I've asked upstream for clarification on the licensing of the test
> models via their SourceForge mailing list.  If it turns out they're not under a
> free license, would it be feasible to re-bundle the source without the test
> directory for the srpm?

I am very opposed to forking source tarballs away from upstreams and would prefer to avoid doing so if possible.

Also, AFAIS, all of the "tests" are "distributable" (legal to be re-distributed), with their licensing not affecting the "binary" rpms and many of them either qualifying as "art work" or as "trivial" (== non-copyrightable == non-licensable). That said, I don't see much reason to remove any of them. 

All I want is RH-legal to give their "rubber stamp" - i.e. them to formally approve or disapprove the legal status of the sources.
Comment 9 Tom "spot" Callaway 2010-10-12 10:56:59 EDT
Assuming upstream is responsive, I would prefer to wait for them to clarify the licensing on those files.
Comment 10 Rich Mattes 2010-10-13 21:51:39 EDT
I got a response from upstream: http://sourceforge.net/mailarchive/message.php?msg_name=4CB5F509.1070608%40gmx.net

They've separated the models into two directories, models that are BSD/zlib/public domain, and models that have questionable or restrictive licensing.  I could export a snapshot from their SVN and remove the nonbsd folder before creating the archive.

They also brought up the fact that the IrrXML they bundle has many bugfixes and enhancements, and that the copy that's included in Fedora's Irrlicht might not work for some Collada models.  I've asked them what the status of upstreaming their patches is; if we can get them accepted upstream into Irrlicht we could carry the patches until the next Irrlicht release.
Comment 11 Tom "spot" Callaway 2010-10-14 10:34:19 EDT
That seems like the best approach to handle the models.

As to the IrrXML bundling, since I'm also the Irrlicht maintainer, I think that sounds like a solid approach. Please let me know how they respond.
Comment 12 Rich Mattes 2010-10-15 10:34:20 EDT
The response to my question about upstreaming:
http://sourceforge.net/mailarchive/message.php?msg_name=4CB6D753.3020805%40gmx.net

It looks like the assimp guys are concerned that the changes they made to irrXML go against IrrXML's goal of being as fast as possible, but they've never tried getting the code upstream.  They don't seem opposed to it though.  I'll try to create a patch that applies to Irrlicht and show it to the assimp guys so they can make sure I didn't miss anything.
Comment 13 Rich Mattes 2010-11-08 15:22:42 EST
I managed to patch Assimp's changes to IrrXML into Irrlicht[1], and completely remove all references to the bundled IrrXML from Assimp[2].  The bulk of the changes are additions to fast_atof.h, all of which are used inside assimp.  I don't know if it makes much sense pushing them to Irrlicht, it might be easier to just let assimp use its own fast_atof.h, and use Irrlicht for the rest of the IrrXML dependencies (this is how the assimp package in Comment 5 is currently configured.)

The assimp guys were also questioning why I'm bringing in all of Irrlicht to use IrrXML, It does seem a little overkill, but there are really no other options unless IrrXML gets split off into its own package.

[1] http://rmattes.fedorapeople.org/irrlicht-1.7.1-fastatof-assimp.patch
[2] http://rmattes.fedorapeople.org/assimp-1.1.700.stripbundledlibs.patch
Comment 14 Tom "spot" Callaway 2010-11-17 14:36:23 EST
Created attachment 461140 [details]
fast_atof speed and accuracy test code

So, I finally found some time to look into this, and I discovered that Irrlicht has a regression test for the fast_atof functions, that tests speed and accuracy. I've fixed it so that it runs on Linux, and attached it to this bug.
Comment 15 Tom "spot" Callaway 2010-11-17 14:37:22 EST
Created attachment 461141 [details]
Example run of test code against unmodified Irrlicht 1.7.2

Example run of test code against unmodified Irrlicht 1.7.2
Comment 16 Tom "spot" Callaway 2010-11-17 14:43:04 EST
Once I'd fixed up the test code (and built/installed a local 1.7.2 package), I compiled it like this:

gcc -m64 -I/usr/include/irrlicht fast_atof.cpp -o fast_atof-172 -lIrrlicht -lstdc++ -lm

(I haven't yet separated IrrXML from Irrlicht yet.)

The results in Comment 15 are from an example run on my Fedora 14 x86_64 laptop.

Then, I applied your patch to fast_atof.cpp, rebuilt and reinstalled, then built a new copy of the test code. The results show that the new code is not performing accurate calculations on some tests:

 String '340282346638528859811704183484516925440.000000'
 New fast 15581490439004356608.0000000000000000000000000000000000000000
 Old fast 0.0000000000000000000000000000000000000000
     atof 340282346638528859811704183484516925440.0000000000000000000000000000000000000000

 String '3.402823466e+38F'
 New fast 340282306073709652508363335590014353408.0000000000000000000000000000000000000000
 Old fast 3.4028234481811523437500000000000000000000
     atof 340282346638528859811704183484516925440.0000000000000000000000000000000000000000

 String '3402823466e+29F'
 New fast 340282346638528859811704183484516925440.0000000000000000000000000000000000000000
 Old fast 3402823424.0000000000000000000000000000000000000000
     atof 340282346638528859811704183484516925440.0000000000000000000000000000000000000000

 String '-340282346638528859811704183484516925440.000000'
 New fast -15581490439004356608.0000000000000000000000000000000000000000
 Old fast -0.0000000000000000000000000000000000000000
     atof -340282346638528859811704183484516925440.0000000000000000000000000000000000000000

 String '-3.402823466e+38F'
 New fast -340282306073709652508363335590014353408.0000000000000000000000000000000000000000
 Old fast -3.4028234481811523437500000000000000000000
     atof -340282346638528859811704183484516925440.0000000000000000000000000000000000000000

 String '-3402823466e+29F'
 New fast -340282346638528859811704183484516925440.0000000000000000000000000000000000000000
 Old fast -3402823424.0000000000000000000000000000000000000000
     atof -340282346638528859811704183484516925440.0000000000000000000000000000000000000000

 String '34028234663852885981170418348451692544.000000'
 New fast 15581490439004356608.0000000000000000000000000000000000000000
 Old fast 0.0000000000000000000000000000000000000000
     atof 34028234663852885981170418348451692544.0000000000000000000000000000000000000000

 String '3.402823466e+37F'
 New fast 34028229593250485068252812361638871040.0000000000000000000000000000000000000000
 Old fast 3.4028234481811523437500000000000000000000
     atof 34028234663852885981170418348451692544.0000000000000000000000000000000000000000

 String '3402823466e+28F'
 New fast 34028234663852885981170418348451692544.0000000000000000000000000000000000000000
 Old fast 3402823424.0000000000000000000000000000000000000000
     atof 34028234663852885981170418348451692544.0000000000000000000000000000000000000000

 String '-34028234663852885981170418348451692544.000000'
 New fast -15581490439004356608.0000000000000000000000000000000000000000
 Old fast -0.0000000000000000000000000000000000000000
     atof -34028234663852885981170418348451692544.0000000000000000000000000000000000000000

 String '-3.402823466e+37F'
 New fast -34028229593250485068252812361638871040.0000000000000000000000000000000000000000
 Old fast -3.4028234481811523437500000000000000000000
     atof -34028234663852885981170418348451692544.0000000000000000000000000000000000000000

 String '-3402823466e+28F'
 New fast -34028234663852885981170418348451692544.0000000000000000000000000000000000000000
 Old fast -3402823424.0000000000000000000000000000000000000000
     atof -34028234663852885981170418348451692544.0000000000000000000000000000000000000000

 String '.00234567'
 New fast 0.0023449999280273914337158203125000000000
 Old fast 0.0023456700146198272705078125000000000000
     atof 0.0023456700146198272705078125000000000000
*** ERROR - less accurate than old method ***


 String '-.00234567'
 New fast -0.0023449999280273914337158203125000000000
 Old fast -0.0023456700146198272705078125000000000000
     atof -0.0023456700146198272705078125000000000000
*** ERROR - less accurate than old method ***


 String '0.00234567'
 New fast 0.0023449999280273914337158203125000000000
 Old fast 0.0023456700146198272705078125000000000000
     atof 0.0023456700146198272705078125000000000000
*** ERROR - less accurate than old method ***


 String '-0.00234567'
 New fast -0.0023449999280273914337158203125000000000
 Old fast -0.0023456700146198272705078125000000000000
     atof -0.0023456700146198272705078125000000000000
*** ERROR - less accurate than old method ***


 String '1.175494351e-38F'
 New fast 0.0000000000000000000000000000000000000118
 Old fast 0.0000000000000000000000000000000000000118
     atof 0.0000000000000000000000000000000000000118
*** ERROR - less accurate than old method ***


 String '1175494351e-47F'
 New fast 0.0000000000000000000000000000000000000118
 Old fast 1175494400.0000000000000000000000000000000000000000
     atof 0.0000000000000000000000000000000000000118

 String '1.175494351e-37F'
 New fast 0.0000000000000000000000000000000000001175
 Old fast 0.0000000000000000000000000000000000001175
     atof 0.0000000000000000000000000000000000001175
*** ERROR - less accurate than old method ***


 String '1.175494351e-36F'
 New fast 0.0000000000000000000000000000000000011755
 Old fast 0.0000000000000000000000000000000000011755
     atof 0.0000000000000000000000000000000000011755
*** ERROR - less accurate than old method ***


 String '-1.175494351e-36F'
 New fast -0.0000000000000000000000000000000000011755
 Old fast -0.0000000000000000000000000000000000011755
     atof -0.0000000000000000000000000000000000011755
*** ERROR - less accurate than old method ***


 String '123456.789'
 New fast 123456.7890625000000000000000000000000000000000
 Old fast 123456.7890625000000000000000000000000000000000
     atof 123456.7890625000000000000000000000000000000000

 String '-123456.789'
 New fast -123456.7890625000000000000000000000000000000000
 Old fast -123456.7890625000000000000000000000000000000000
     atof -123456.7890625000000000000000000000000000000000

 String '0000123456.789'
 New fast 123456.7890625000000000000000000000000000000000
 Old fast 123456.7890625000000000000000000000000000000000
     atof 123456.7890625000000000000000000000000000000000

 String '-0000123456.789'
 New fast -123456.7890625000000000000000000000000000000000
 Old fast -123456.7890625000000000000000000000000000000000
     atof -123456.7890625000000000000000000000000000000000
Calculation is not accurate, so the speed is irrelevant
Comment 17 Tom "spot" Callaway 2010-11-17 14:44:41 EST
So, either the GetTickCount() equivalent I added to the test code is not correct, or these fast_atof changes are not likely to be accepted upstream. Either way, I think it would be useful to share this data with the Assimp upstream and see what they think about this.
Comment 18 Rich Mattes 2010-11-17 15:02:47 EST
If I'm reading the results from comment 15 right, glibc's atof() is faster than both Irrlicht's and Assimp's anyway?  Maybe I should just patch Assimp to use the system atof in Linux...

I think the errors might result from a #define hidden in the Assimp version of the fast_atof, namely #define AI_FAST_ATOF_RELAVANT_DECIMALS 6.  It looks like that dictates how many decimal points to pay attention to before giving up and returning.  Setting it to something like 10 or 15 (the max value) might improve the results.
Comment 19 Tom "spot" Callaway 2010-11-17 15:55:58 EST
Yeah, you're dead on about the errors. Pushing it to 15 makes the errors go away.

At 15, these are the times from three runs:

Run 1:
         atof time = 478
    fast_atof Time = 268
old fast_atof time = 486
The fast method isn't at least three times as fast as atof()

Run 2:
        atof time = 446
    fast_atof Time = 267
old fast_atof time = 461
The fast method isn't at least three times as fast as atof()

Run 3:
         atof time = 399
    fast_atof Time = 295
old fast_atof time = 462
The fast method isn't at least three times as fast as atof()

Averaged:

        atof time = 441
    fast_atof Time = 276
old fast_atof time = 469

The fast_atof time with the assimp patch and AI_FAST_ATOF_RELAVANT_DECIMALS 15 is approx 1.6x faster than glibc atof.

Just for reference, the average fast_atof time in stock 1.7.2 across three runs was: 504. Which is slower than the glibc atof.

I'm doing runs with AI_FAST_ATOF_RELAVANT_DECIMALS 10 now.
Comment 20 Tom "spot" Callaway 2010-11-17 16:08:01 EST
At 10, these are the times from three runs:

         atof time = 451
    fast_atof Time = 273
old fast_atof time = 476
The fast method isn't at least three times as fast as atof()

         atof time = 428
    fast_atof Time = 310
old fast_atof time = 531
The fast method isn't at least three times as fast as atof()

         atof time = 406
    fast_atof Time = 309
old fast_atof time = 490
The fast method isn't at least three times as fast as atof()

Averaged:
         atof time = 428.3
    fast_atof Time = 297.3
old fast_atof time = 499

The fast_atof time with the assimp patch and AI_FAST_ATOF_RELAVANT_DECIMALS 10
is approx 1.4x faster than glibc atof. Approx 1.7x faster than stock 1.7.2 fast_atof.

Does assimp have a preferred default? I think we'd need to be higher than 6 to get it merged, but these numbers look pretty good to me.
Comment 21 Tom "spot" Callaway 2010-12-15 13:42:53 EST
Have you talked to assimp upstream about this? If they have a preferred default, it would be good to know it before I propose this change to irrlicht.
Comment 22 Rich Mattes 2010-12-15 17:11:10 EST
They just got back to me[1], it looks like they are going to investigate moving it to 10 on their side as well.  6 was apparently picked because most file formats don't encode floating point numbers past that, so there was no point in processing any more than that.

Assimp 2.0 was released a couple of weeks ago, which includes the separation of models with/without BSD licenses.  I will rebase my changes to that release and post the updated srpm later tonight.

[1] http://sourceforge.net/mailarchive/message.php?msg_id=26762291
Comment 23 Rich Mattes 2010-12-18 15:48:26 EST
I was able to port my changes forward to the 2.0 release and delete the non-bsd models from the source distribution.  It requires the patch in comment 13 to be applied to Irrlicht before it will build.

Spec URL: http://rmattes.fedorapeople.org/RPMS/assimp/assimp.spec
SRPM URL: http://rmattes.fedorapeople.org/RPMS/assimp/assimp-2.0.863-1.fc14.src.rpm
Comment 24 Rich Mattes 2011-02-02 23:56:47 EST
Any updates with the Irrlicht devs?  And I've removed the non-free models from the tarball, I think that satisfies the FE-Legal blocker?
Comment 25 Tom "spot" Callaway 2011-02-03 16:44:31 EST
Post here, sorry for the delay, this fell off my radar:
http://irrlicht.sourceforge.net/phpBB2/viewtopic.php?p=243079#243079
Comment 26 Jason Tibbitts 2011-03-21 21:04:37 EDT
*** Bug 689639 has been marked as a duplicate of this bug. ***
Comment 27 Tim Niemueller 2011-03-22 05:21:56 EDT
Is there news about this package? I packaged it myself because I needed it for another package (Thanks Jason for noting the duplicate, I just rushed through it and forgot to check).
Comment 28 Tom "spot" Callaway 2011-03-22 13:25:11 EDT
I'm working with the irrlicht upstream to merge the fast_atof improvements that assimp needs. They're on board in principle, but they didn't like some of the function naming (the original patch changed the existing function strtol10 from returning a signed int to an unsigned int), so I reworked the code so that strtol10 returns a signed int, and there are new "u" functions (e.g. strtoul10) that return the unsigned ints.

I had to patch assimp for the new names, here's that patch:
http://spot.fedorapeople.org/assimp-2.0.863-new-fastatof-typenames.patch

For reference, the irrlicht patch is here:
http://spot.fedorapeople.org/irrlicht-1.7.2-fastatof-improvements-typefixes.patch

I've done test compiles with both patches in play, and things look good. Now, we just need more feedback from irrlicht upstream as to whether they will take this change or not. I'm happy to carry the irrlicht patch in the short term. It would probably be a good idea to let assimp know about this, and get their feedback on the name changes.
Comment 29 Tim Niemueller 2011-03-22 18:49:33 EDT
I'd vote for speeding this up by including the patches in the Fedora packages until they are upstreamed.
Comment 30 Rich Mattes 2011-03-23 00:02:17 EDT
I've emailed the assimp upstream about possibly changing the names of their internal functions (applying spot's assimp patch) so that we can more easily build against the external irrXML.  The thread should show up soon at https://sourceforge.net/mailarchive/forum.php?forum_name=assimp-discussions
Comment 31 Rich Mattes 2011-03-23 07:06:45 EDT
Assimp upstream made the following changes[1] in their SVN trunk:

strtol10 -> strtoul10
strtol16 -> strtoul16
strtol8  -> strtoul8
strtol10_64 -> strtoul10_64
strtol_cppstyle -> strtoul_cppstyle
strtol10_s -> strtol10

I can update my package with an SVN snapshot and the irrxml removal patch within a day or so

[1] http://sourceforge.net/mailarchive/forum.php?thread_name=4D89C9C6.4050308%40gmx.net&forum_name=assimp-discussions
Comment 32 Rich Mattes 2011-03-24 02:46:44 EDT
I made sure the latest version of irrXML-devel was installed on my machine, with the fast_atof patches, and regenerated my assimp patches based on the latest svn snapshot.

Spec URL: http://rmattes.fedorapeople.org/RPMS/assimp/assimp.spec
SRPM URL:
http://rmattes.fedorapeople.org/RPMS/assimp/assimp-2.0.863-2.20110324svn.fc15.src.rpm

$ rpmlint assimp.spec ../RPMS/x86_64/assimp*
assimp.spec: W: invalid-url Source0: assimp-924.tar.bz2
assimp.x86_64: W: shared-lib-calls-exit /usr/lib64/libassimp.so.2.0.0 exit@GLIBC_2.2.5
assimp.x86_64: W: no-manual-page-for-binary assimp
3 packages and 1 specfiles checked; 0 errors, 3 warnings.
Comment 33 Tim Niemueller 2011-03-24 04:36:35 EDT
Excellent, I'll try with OpenRAVE later.

For the shared-lib-calls-exit thing: please consider asking upstream to either have a release mode which is silent about the error (and a debug mode where this exits) or to throw a std::exception which can be caught on software embedding assimp simply reporting a load error. I'd assume assertions are used for "static" errors, i.e. not-so-dynamic errors which either happen or don't and if then they appear in the development phase, rather than using them for simple value range checks etc., which should be reported as soft errors to the caller. Is that the case?
Comment 34 Tim Niemueller 2011-04-06 06:34:36 EDT
Seems to work with OpenRave without problems, but I can't tell how to do specific testing of the assimp integration. Does this still block FE-Legal or can we continue with the review now?

CC'ing Rosen on this. Rosen, can you give instructions how to test the assimp integration in OpenRave?
Comment 35 Rich Mattes 2011-04-06 08:31:46 EDT
The latest updates to the package have addressed the legal concerns by removing the non-free model files from the tarball.  The remaining model files should all have bsd-like licenses, and are OK for redistribution.  I do not think I can lift the FE-LEGAL flag on my own though.
Comment 36 Tom "spot" Callaway 2011-04-06 10:29:43 EDT
I think at this point, I'm fine to carry the patch in irrlicht, upstream seems interested in adding it in a future release.

Also, I see no remaining legal issues, so I'm lifting FE-Legal. Go ahead and finish this review, and let me know if you need any more irrlicht changes.
Comment 37 Rosen Diankov 2011-04-07 03:40:26 EDT
To test with openrave, just do:

openrave anyfile.ext

where ext is an extension supported by assimp.
Comment 38 Simon Thompson 2011-08-11 03:41:57 EDT
I just tried building this in mock on my machine and it fell over:

DEBUG: In file included from /builddir/build/BUILD/assimp/code/irrXMLWrapper.h:45In file included from /builddir/build/BUILD/assimp/code/DXFLoader.cpp:52:
DEBUG: /usr/include/irrlicht/fast_atof.h:8:21: error: irrMath.h: No such file or directory
DEBUG: In file included from /builddir/build/BUILD/assimp/code/CSMLoader.cpp:53:
DEBUG: /usr/include/irrlicht/fast_atof.h:8:21: error: irrMath.h: No such file or directory

Was building against irrlicht-1.7.2-8.fc17 built locally.

Adding:
BuildRequires:  irrlicht-devel
to assimp results in a successful build.
Comment 39 Rich Mattes 2011-08-24 21:47:25 EDT
Thanks, I've added a buildrequires on irrlicht-devel.  While I was at it, I upgraded the package to the latest SVN snapshot, since some of my unbundling changes were accepted upstream over the past few months.  You can find the new srpm and spec at:

Spec URL: http://rmattes.fedorapeople.org/RPMS/assimp/assimp.spec
SRPM URL:
http://rmattes.fedorapeople.org/RPMS/assimp/assimp-2.0.863-3.20110824svn.fc15.src.rpm

Scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=3299994

rpmlint output:
$ rpmlint assimp.spec ../RPMS/x86_64/assimp*
assimp.spec: W: invalid-url Source0: assimp-1071.tar.bz2
assimp.x86_64: W: no-manual-page-for-binary assimp
3 packages and 1 specfiles checked; 0 errors, 2 warnings.
Comment 40 Tim Niemueller 2011-12-04 19:02:20 EST
While testing the new package with OpenRAVE I found that the pkg-config file is broken. The includedir and libdir have the prefix twice (e.g. /usr//usr/include/assimp).
Comment 41 Rich Mattes 2011-12-16 20:19:16 EST
Thanks Tim, I've patched it and uploaded the result.

Spec URL: http://rmattes.fedorapeople.org/RPMS/assimp/assimp.spec
SRPM URL:
http://rmattes.fedorapeople.org/RPMS/assimp/assimp-2.0.863-4.20110824svn.fc16.src.rpm

$ rpmlint ../SRPMS/assimp-2.0.863-4.20110824svn.fc16.src.rpm ../RPMS/x86_64/assimp-*
assimp.src: W: invalid-url Source0: assimp-1071.tar.bz2
assimp.x86_64: W: no-manual-page-for-binary assimp
Comment 42 Tim Niemueller 2012-04-03 10:45:29 EDT
I'll take it since I need it.
Comment 43 Tim Niemueller 2012-04-03 11:53:09 EDT
REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

- rpmlint is not silent and some messages may not be ignored
  assimp.src: W: invalid-url Source0: assimp-1071.tar.bz2
  Is ok, source is created with script that comes with the package from svn and deleting dll files
  assimp.x86_64: W: no-manual-page-for-binary assimp
  4 packages and 0 specfiles checked; 0 errors, 2 warnings.

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format
%{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the
+ The License field in the package spec file matches the actual license ().
+ The file, containing the text of the license(s) for the package, is included
in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
(+) The sources used to build the package, match the upstream source, as provided in the spec URL.
  Source is downloaded from svn and modified to not ship windows DLLs. I have done an own checkout and compared my version and the package version with diff. No changes have been shown.

+ The package successfully compiles and builds into binary rpms on at least one
primary architecture.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
+ Spec file calls ldconfig
+ The package does NOT bundle copies of system libraries.
+ The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files
listings.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
(+) The package consistently uses macros.
  You use macros most of the time, but $RPM_BUILD_ROOT. Please consider using %{buildroot} instead.

+ The package contains code, or permissible content.
+ No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the
application.
+ Header files in devel sub-package
0 No static libraries.
+ pkgconfig(.pc) files in devel package
+ Non-suffix so file is in devel sub-package
+ Devel sub-package properly depends on main package
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other
packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
+ All filenames in rpm packages are valid UTF-8.

Looks good. Please fix macro usage before importing. Please consider providing it for EPEL 6 if feasible (e.g requirements are met).

APPROVED
Comment 44 Rich Mattes 2012-04-04 19:48:07 EDT
New Package SCM Request
=======================
Package Name: assimp
Short Description: Library to import various 3D model formats into applications
Owners: rmattes
Branches: f16 f17 el6
InitialCC:
Comment 45 Jon Ciesla 2012-04-05 08:09:05 EDT
Git done (by process-git-requests).
Comment 46 Fedora Update System 2012-04-15 13:42:09 EDT
assimp-2.0.863-5.20110824svn.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/assimp-2.0.863-5.20110824svn.fc16
Comment 47 Fedora Update System 2012-04-15 19:54:46 EDT
assimp-2.0.863-5.20110824svn.fc16 has been pushed to the Fedora 16 testing repository.
Comment 48 Fedora Update System 2012-04-24 10:51:28 EDT
assimp-2.0.863-5.20110824svn.fc16 has been pushed to the Fedora 16 stable repository.
Comment 49 Orion Poplawski 2012-07-03 16:51:05 EDT
I've built and submitted an update for el6 since the dependencies are there now.

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