FYI, You forgot to block bug "2" (the new review request tracker) but since I'm taking it, don't worry about doing it now.

Spec looks good :)

I'll hopefully have time to work on the full review today or tomorrow.

1. I'm assuming you didn't try to build in mock. I found I needed to add the following BR's to get a successful build:

BuildRequires: libXmu-devel
BuildRequires: freeimage-devel

2. Have you reported the incorrect FSF address upstream?

3. Disclaimer: I'm not a license expert :)

Can you show me what/where in the source made you decide that the license was LGPLv2+? I couldn't find any meaningful occurence of "or later". 

There were a one directory of LGPLv2+ licensed files, "src/Exchange3DS", but all the other files seemed to be "LGPLv2". 

Also, since this links against OCE it should have the same statement I added to "smech". Just append "with exception" to the end of whatever license we figure out.

I thought the spec might look somewhat familiar. ;)

Oops. You're right, did not build in mock (running low on space on my machine). Added those BRs now.

> 2. Have you reported the incorrect FSF address upstream?

http://sourceforge.net/tracker/?func=detail&aid=3477860&group_id=214102&atid=1028339

(Have also opened issues related to the patches.)

> Can you show me what/where in the source made you decide that the license was
> LGPLv2+? I couldn't find any meaningful occurence of "or later".

Ah, was confused myself. Most source files do still contain the word "either" which threw me off, but it looks like "or later..." was removed, so it wasn't clear cut to me at that moment. We could presume the author(s) intentionally removed the "or later..." bit from standard notice [1] but forgot to correct the sentence for grammar? Maybe I should contact the upstream to clarify/fix this as well?

I've put it as "LGPLv2 with exceptions" for now.

SPEC: http://kmilos.fedorapeople.org/geom.spec 
SRPM: http://kmilos.fedorapeople.org/geom-6.3.1.8-2.svn222.fc16.src.rpm

[1] http://www.gnu.org/licenses/lgpl-2.1.html#TOC4

(In reply to comment #3)
> > Can you show me what/where in the source made you decide that the license was
> > LGPLv2+? I couldn't find any meaningful occurence of "or later".
> 
> Ah, was confused myself. Most source files do still contain the word "either"
> which threw me off, but it looks like "or later..." was removed, so it wasn't
> clear cut to me at that moment. We could presume the author(s) intentionally
> removed the "or later..." bit from standard notice [1] but forgot to correct
> the sentence for grammar? Maybe I should contact the upstream to clarify/fix
> this as well?
> 
> I've put it as "LGPLv2 with exceptions" for now.

I would check with the author on what his intentions were but even if he tells you it's LGPLv2 (or later), he still needs to make sure the license and header files reflect this properly. Although he may be stuck with the verbage since this is effectivly a "fork" of the original, right?

Have you been able to get any clarification from upstream on the license?

(In reply to comment #5)
> Have you been able to get any clarification from upstream on the license?

I have included this request for clarification as a comment on

http://sourceforge.net/tracker/?func=detail&aid=3477860&group_id=214102&atid=1028339

So far, no word on any of the bugs filed upstream :(

(In reply to comment #4)
> Although he may be stuck with the verbage since
> this is effectivly a "fork" of the original, right?

Well, the files in geom-6.3.1.8/src/Exchange3DS/lib3ds/ are not a fork, they are a bundled copy of lib3ds-1.3.0 (all files are bit-wise identical).

(In reply to comment #7)
> Well, the files in geom-6.3.1.8/src/Exchange3DS/lib3ds/ are not a fork, they
> are a bundled copy of lib3ds-1.3.0 (all files are bit-wise identical).

Ah, thanks fro checking. That makes things a bit more complicated, as this probably needs to be unbundled and packaged separately? Unless the lib3ds project is being merged into GEOM?

(In reply to comment #8)
> Ah, thanks fro checking. That makes things a bit more complicated, as this
> probably needs to be unbundled and packaged separately? Unless the lib3ds
> project is being merged into GEOM?

Doh, just saw it's already in the main Fedora repo. Will try to work on unbundling and linking that one then.

SPEC: http://kmilos.fedorapeople.org/geom.spec 
SRPM: http://kmilos.fedorapeople.org/geom-6.3.1.8-3.svn222.fc16.src.rpm

$ rpmlint SRPMS/*.rpm 
geom.src: W: invalid-license LGPLv2 with exception
geom.src: W: invalid-url Source0: geom-6.3.1.8-svn222.tar.gz
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

$ rpmlint RPMS/i686/*.rpm 
geom.i686: W: invalid-license LGPLv2 with exception
geom.i686: E: incorrect-fsf-address /usr/share/doc/geom-6.3.1.8/LICENCE.lgpl.txt
geom-debuginfo.i686: W: invalid-license LGPLv2 with exception
geom-devel.i686: W: invalid-license LGPLv2 with exception
geom-devel.i686: W: no-documentation
3 packages and 0 specfiles checked; 1 errors, 4 warnings.

$ rpmlint RPMS/noarch/*.rpm 
geom-doc.noarch: W: invalid-license LGPLv2 with exception
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Latest version unbundles lib3ds and also corrects the preprocessing INCLUDE_PATH in the Doxygen template (Richard, you might want to check SMESH too).

Note: Probably didn't do the proper thing of finding the lib3ds the CMake way. Got lucky with the way lib3ds is packaged and included in GEOM source, so the BuildRequires suffices, hope that's ok for now.

(In reply to comment #10)
> Note: Probably didn't do the proper thing of finding the lib3ds the CMake way.
> Got lucky with the way lib3ds is packaged and included in GEOM source, so the
> BuildRequires suffices, hope that's ok for now.

I can help there. It looks like lib3ds-devel provides a pkgconf config file so something like this should work:

INCLUDE(FindPkgConfig)
IF(USE_EXTERNAL_LIB3DS)
       # The last argument may need to be lib3ds depending on the pkgconf config.
       PKG_CHECK_MODULES(LIB3DS REQUIRED 3ds)
       INCLUDE_DIRECTORIES(SYSTEM ${LIB3DS_INCLUDE_DIRS})
ENDIF()
---

And then the following:
TARGET_LINK_LIBRARIES(Exchange3DS TKernel TKBRep TKMath TKMesh TKV3d TKTopAlgo 3ds)
would become:
TARGET_LINK_LIBRARIES(Exchange3DS TKernel TKBRep TKMath TKMesh TKV3d TKTopAlgo ${LIB3DS_LIBRARIES})
---

Also, if you use the "option" as shown above you'll need to add that to you cmake options:
%cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \
       -DUSE_EXTERNAL_LIB3DS=TRUE \
       ..
---

Sigh. I really hoped I could pull a fast one on that, and not pull in pkgconfig as a BR as well...

Also, just out of curiosity, I don't get why libXmu and freeimage are needed at build time? They're not included anywhere in the code, shouldn't dynamic linking of OCE libs take care of the rest?

(In reply to comment #12)
> Sigh. I really hoped I could pull a fast one on that, and not pull in pkgconfig
> as a BR as well...

Eh... :) Not really a big deal. Another option would be to use FIND_FILE(... and FIND_LIBRARY(... to do things manually but the pkg-config module does make things easy.

 
> Also, just out of curiosity, I don't get why libXmu and freeimage are needed at
> build time? They're not included anywhere in the code, shouldn't dynamic
> linking of OCE libs take care of the rest?

Not sure... I'd have to investigate. Either their cmake config is requiring it even if it's not being used, or it's a requirement of one of the other main requirements?

Ok, did it the pkg-config way.

SPEC: http://kmilos.fedorapeople.org/geom.spec 
SRPM: http://kmilos.fedorapeople.org/geom-6.3.1.8-4.svn222.fc16.src.rpm

$ rpmlint SRPMS/*.rpm RPMS/noarch/*.rpm RPMS/i686/*.rpm
geom.src: W: invalid-license LGPLv2 with exception
geom.src: W: invalid-url Source0: geom-6.3.1.8-svn222.tar.gz
geom-doc.noarch: W: invalid-license LGPLv2 with exception
geom.i686: W: invalid-license LGPLv2 with exception
geom.i686: E: incorrect-fsf-address /usr/share/doc/geom-6.3.1.8/LICENCE.lgpl.txt
geom-debuginfo.i686: W: invalid-license LGPLv2 with exception
geom-devel.i686: W: invalid-license LGPLv2 with exception
geom-devel.i686: W: no-documentation
6 packages and 0 specfiles checked; 1 errors, 9 warnings.

Looks good to me. I'll try to do the review today or tomorrow.

Ok, first the good news. Most everything looks good. 

Now the bad news...

Most people don't both running rpmlint on the installed package and it's not a packaging guideline requirement (but it should be). I found some linkage problems with the library. I'll attach the full rpmlint output because it's too big to post here (which usually isn't a good sign :)

Basically you have two major things going on. Both are fixable.

The first is "undefined-non-weak-symbol" which rpmlint describes as:
$ rpmlint -I undefined-non-weak-symbol
undefined-non-weak-symbol:
The binary contains undefined non-weak symbols.  This may indicate improper
linkage; check that the binary has been linked as expected.

Basically, it has symbols for libraries it's not linked to.

The other, easier one, is: "unused-direct-shlib-dependency"

The most direct method is to add "-Wl,--as-needed" to your linker flags, a la:
LDFLAGS="-Wl,--as-needed";export LDFLAGS

just before your cmake command.

If you have trouble fixing the first, let me know. I'll see if I can help. Also, once fixed, you'll need to send the patch upstream.

SPEC: http://kmilos.fedorapeople.org/geom.spec 
SRPM: http://kmilos.fedorapeople.org/geom-6.3.1.8-5.svn222.fc16.src.rpm

(In reply to comment #16)
> The first is "undefined-non-weak-symbol" which rpmlint describes as:
> $ rpmlint -I undefined-non-weak-symbol
> undefined-non-weak-symbol:
> The binary contains undefined non-weak symbols.  This may indicate improper
> linkage; check that the binary has been linked as expected.
> 
> Basically, it has symbols for libraries it's not linked to.

Ok, managed to find a few missing, but the majority is still there. The symbols seem to come from the GEOM libs themselves - no idea how to handle this, help appreciated.
 
> The other, easier one, is: "unused-direct-shlib-dependency"
> 
> The most direct method is to add "-Wl,--as-needed" to your linker flags, a la:
> LDFLAGS="-Wl,--as-needed";export LDFLAGS
> 
> just before your cmake command.

Had this in already from your SMESH spec file.

Looks lika a build problem, some file are not compiled. Focusing on  Sketcher::GetLabel(), one of the undefined symbols:

Build the thing: 
$ rpmbuild -bc  ~/rpmbuild/SPECS/geom*spec 2>&1 | tee build.log
$ pushd ~/rpmbuild/geom*

There is no defined symbol in the object files:
$ nm -gC $(find . -name \*.o) | grep  'Sketcher::GetLabel'
         U Sketcher::GetLabel()

GetLabel lives in src/Sketcher.cpp:
$ grep -r Sketcher::GetLabel src
src/Sketcher/Sketcher.cpp:TDF_Label Sketcher::GetLabel()

There is no file Sketcher.o, and Sketcher.cpp doesn't seem to be compiled:
$ find . -name Sketcher.o
$ popd
$ grep Sketcher.cpp build.log

just my two cents...

(In reply to comment #18)
> There is no file Sketcher.o, and Sketcher.cpp doesn't seem to be compiled:
> $ find . -name Sketcher.o
> $ popd
> $ grep Sketcher.cpp build.log
> 
> just my two cents...

Thanks Alec, that one was actually down to globbing for Sketcher_*.cpp instead of Sketcher*.cpp in CMakeLists.txt

Just by picking a few other ones at random, it does look like the implementation of those methods really is missing in the source files, although the definitions are there in the header files, so file another bug upstream.

Missing method implementation upstream bug:

https://sourceforge.net/tracker/?func=detail&aid=3489036&group_id=214102&atid=1028339

Is this review still active? I admint I haven't had much time lately but I'm willing to help move it along.

I had no response from Fotis on upstream tickets and patches reported to him on Sourceforge (also pinged on email), so if you're in touch with him, maybe you can help raise some interest?

It also looks he's more focused on the WOK based version [1] going forward, which I'm also not sure how well it'll fit with OCE?

http://salomegeometry.svn.sourceforge.net/viewvc/salomegeometry/

I had a few minutes and looked through open reviews and ran across this one. I hate to admit I had completely forgotten about it! :)

Have you had any luck with upstream?

> Have you had any luck with upstream?

Nope.

Hello Milos, hello Richard,
I am a user of Salome Meca on caelinux (http://www.caelinux.com/CMS/) and I would like to see it in Fedora too. I'm curious why you packaged geom instead of the original salome platform (maybe some licensing/funcionality/personal preference issues?). Can I help speed up the review in some way?

Thanks, 

Mario

(In reply to comment #25)
> Hello Milos, hello Richard,
> I am a user of Salome Meca on caelinux (http://www.caelinux.com/CMS/) and I
> would like to see it in Fedora too. I'm curious why you packaged geom instead
> of the original salome platform (maybe some licensing/funcionality/personal
> preference issues?). Can I help speed up the review in some way?

I'm not sure. Has SALOME always been LGPL licensed? I don't like the fact it requires a login to download the source though...

Hello Richard,
Once registered, I see these links: 
http://www.salome-platform.org/downloads/current-version/DownloadDistr?platform=Sources&version=6.6.0
http://www.salome-platform.org/downloads/current-version/DownloadDistr?platform=Documentation&version=6.6.0
http://git.salome-platform.org/gitweb/

they seem to be static and not related to the login, but I'm not sure.

However, I sent an email to the legal list to ask for advice:
http://lists.fedoraproject.org/pipermail/legal/2013-January/002061.html

and I'll post here whatever they answer to me.

Best,
Mario

(In reply to comment #25)
> I am a user of Salome Meca on caelinux (http://www.caelinux.com/CMS/) and I
> would like to see it in Fedora too. I'm curious why you packaged geom instead
> of the original salome platform (maybe some licensing/funcionality/personal
> preference issues?). Can I help speed up the review in some way?

Thanks for the interest Mario. I admit I have not looked into licensing issues for Salome. My motivation was to get the bare minimum in to support packaging pythonOCC next. (I guess the same was true for Richard packaging SMESH for FreeCAD.)

For this particular package, you can help by trying to generate some interest upstream perhaps?

Now that OCE is in Fedora I assume this bug can be closed?

Since OCE is now in Fedora I assume this can be in Fedora as well. Closing.