Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: openCOLLADA - 3D import and export libraries
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Summary: Review Request: openCOLLADA - 3D import and export libraries Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: unspecified Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: hobbes1069@gmail.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora Story Points: ---
Spec URL: https://docs.google.com/leaf?id=0B4A6of0Rl4nUZWVlOWE3ZGEtNDdiMi00MWZmLWE5NmQ... SRPM URL: https://docs.google.com/leaf?id=0B4A6of0Rl4nUMzczZmZmOGEtZGRlOC00NDdmLWFhOTU... Description: COLLADA is a royalty-free XML schema that enables digital asset exchange within the interactive 3D industry. OpenCOLLADA is a Google summer of code opensource project providing libraries for 3D file interchange between applications like blender.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Richard Shaw hobbes1069@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841(FE-NEEDSPONSOR)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Nicolas Chauvet (kwizart) kwizart@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |kwizart@gmail.com
--- Comment #1 from Nicolas Chauvet (kwizart) kwizart@gmail.com 2011-04-06 18:18:08 EDT --- This is an informal review since I cannot sponsor you.
1/ Can you show the output of rpmlint on the src.rpm and the installed package. Please fix the empty spaces in the middle of the lines for the %description
2/ Please remove Everything that is not usefull in fedora. This reduce readability. (specially everything before the 'Name' field as this is not usefull in fedora).
3/ The version field is wrong, please follow: https://fedoraproject.org/wiki/Packaging:Guidelines#Version_and_Release
4/ Uneeded BR: gcc-c++ - It is added by default, please remove.
5/ Buildroot is undeed nowdays: https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
6/ Remove spurious 'echo %{buildir}'
7/ rpm -E %{?jobsflag} is empty. fedora uses %{?_smp_mflags} instead. Please remove the previous rpm macro.
8/ Where does CHANGES.TXT come from ? Please add an url if possible ----------- # copy CHANGES.txt cp %{S:1} ./ ----------- It's better to use install -p
9/ About: # Add some docs, need to fix eol encoding with dos2unix in some files -> Please move this in %prep section
10/# Manual install ... Are you sure this cannot be installed with scons ? You should fix it within scons files then.
Here a preliminary pass for the review.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #2 from Richard Shaw hobbes1069@gmail.com 2011-04-07 09:58:13 EDT --- (In reply to comment #1)
1/ Can you show the output of rpmlint on the src.rpm and the installed package.
rpmlint for SRPM: openCOLLADA.src: W: spelling-error Summary(en_US) Collada -> Collard, Collate, Collage openCOLLADA.src: W: spelling-error %description -l en_US gtempaccount -> unaccountable, accountable, accounting openCOLLADA.src:118: W: mixed-use-of-spaces-and-tabs (spaces: line 8, tab: line 118) openCOLLADA.src: W: invalid-url Source0: openCOLLADA-svn836.tar.bz2 1 packages and 0 specfiles checked; 0 errors, 4 warnings.
The installed package created a lot of warning that I didn't understand but no errors. I'll attach the output instead of pasting it here.
Please fix the empty spaces in the middle of the lines for the %description
Empty lines? Fixed.
2/ Please remove Everything that is not usefull in fedora. This reduce readability. (specially everything before the 'Name' field as this is not usefull in fedora).
Those macros are used to build the fake lib version. I can't remove them since they are used later. What should we do here?
3/ The version field is wrong, please follow: https://fedoraproject.org/wiki/Packaging:Guidelines#Version_and_Release
I was not sure how to handle this since it has no "version" in the traditional sense. If I understand the guidelines correctly then this package will always be at version "1.0" with the svn tag in the release?
4/ Uneeded BR: gcc-c++ - It is added by default, please remove.
Fixed.
5/ Buildroot is undeed nowdays: https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
I saw this before but the spec templates still have it in there so the guidelines conflict here. Now I know it's truly save to remove...
Isn't one of the other sections, %clean if I remember correctly also now not needed?
6/ Remove spurious 'echo %{buildir}'
Fixed
7/ rpm -E %{?jobsflag} is empty. fedora uses %{?_smp_mflags} instead. Please remove the previous rpm macro.
Fixed
8/ Where does CHANGES.TXT come from ? Please add an url if possible
# copy CHANGES.txt cp %{S:1} ./
It's better to use install -p
It's the svn log which I guess the Suse creator/maintainer of this package either likes or Suse requires. Do we need this? It appears to only be added to the -devel package...
9/ About: # Add some docs, need to fix eol encoding with dos2unix in some files -> Please move this in %prep section
10/# Manual install ... Are you sure this cannot be installed with scons ? You should fix it within scons files then.
Not sure. I'll ask the Suse maintainer why he chose this route.
Here a preliminary pass for the review.
Thanks!
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #3 from Richard Shaw hobbes1069@gmail.com 2011-04-07 10:00:52 EDT --- Created attachment 490553 --> https://bugzilla.redhat.com/attachment.cgi?id=490553 output of "rpmlint libOpenCOLLADA" for installed package
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #4 from Richard Shaw hobbes1069@gmail.com 2011-04-07 15:31:37 EDT --- (In reply to comment #2)
(In reply to comment #1)
10/# Manual install ... Are you sure this cannot be installed with scons ? You should fix it within scons files then.
Not sure. I'll ask the Suse maintainer why he chose this route.
I got a response, here's my email and his response: --- 04/07/2011 04:03 PM, Richard Shaw wrote:
Dave,
While trying to get blender accepted for Fedora I got a request that I submit the openCOLLADA package for review as well.
One of the questions I couldn't answer is why %install is so manual and if it's possible to get scons to do the install even if it requires patching.
Not patching writing from scratch. IMHO it's better to have control over installation from the spec file rather than a badly written make install that puts everything in the wrong places. If you're interested in scons and libraries have a look at ffado that's the only library I know of that uses scons to build most use autotools. When I finally have time to switch from scons to cmake, I've already patched SConstruct and the SConscripts heavily, I can get the library versioning and assembly correct. Scons doesn't cater for libs, you have to write everything yourself, whereas cmake does. Collada has SConstruct and CMakeLists.txt, when I originally built it there was only scons and of course vcproj2cmake.rb for visual studio. ---
Does this make sense? It's a little over my head.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #5 from Nicolas Chauvet (kwizart) kwizart@gmail.com 2011-04-12 06:06:07 EDT --- Hi,
What is good it to have an accurate make install script that install things in the right place. If it's not done by the developpers, that's move the "responsability" of the location onto the packager shoulders. So that's worth for a given release, and that might be right in this case; but it should also be keep right for others releases and others changes done in consciousness by the developper team.
Can you upload an updated spec file somewhere ?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #6 from Richard Shaw hobbes1069@gmail.com 2011-04-12 09:44:47 EDT --- (In reply to comment #5)
Hi,
What is good it to have an accurate make install script that install things in the right place. If it's not done by the developpers, that's move the "responsability" of the location onto the packager shoulders. So that's worth for a given release, and that might be right in this case; but it should also be keep right for others releases and others changes done in consciousness by the developper team.
I had some further communications with the Suse maintainer and I found out he was responsible for getting the scons build to work. The developers mainly cater to Windows so I don't see them trampling on any of the work we do for linux. In fact, whatever improvements we (and the Suse maintainer) make will probably get accepted upstream without issue.
Can you upload an updated spec file somewhere ?
Sure. I just wanted to get your response first.
I still need your input for the "version" of the package, but I'll assume I was correct for now since you didn't say otherwise.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #7 from Nicolas Chauvet (kwizart) kwizart@gmail.com 2011-04-12 10:03:54 EDT --- (In reply to comment #6) ...
I still need your input for the "version" of the package,
This still doesn't fit the versioning guideline in the case of svn snapshot. https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning If upstream doesn't use a version, then the Version field is "0".
Note that I will be in vacation https://fedoraproject.org /wiki/Vacation If a sponsors wants to continue de review, please do so...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #8 from Richard Shaw hobbes1069@gmail.com 2011-04-12 10:33:10 EDT --- Uploaded. The existing link should work.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #9 from Nicolas Chauvet (kwizart) kwizart@gmail.com 2011-04-12 11:04:47 EDT --- The output of rpmlint you have attached mention a lot of undefined-non-weak-symbol. It mean most library are missing the right library link flags at compile time. This is usually something that can be fixed within the build tool (SCons). This have to be reported upstream and is probably rather trivial to fix.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #10 from Richard Shaw hobbes1069@gmail.com 2011-04-12 11:34:29 EDT --- I checked into it but I can't find a solution. When it do a direct "ldd /usr/lib64/<library name>" I don't see the error.
Is it possible these are loaded on demand and these are false positives by rpmlint?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #11 from Nicolas Chauvet (kwizart) kwizart@gmail.com 2011-04-12 11:43:24 EDT --- you can show it with ldd -r <library name>
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #12 from Richard Shaw hobbes1069@gmail.com 2011-04-12 11:55:35 EDT --- Output:
# ldd -r /usr/lib64/libbuffer.so linux-vdso.so.1 => (0x00007fff8f6d2000) libstdc++.so.6 => /usr/lib64/libstdc++.so.6 (0x0000003e4ee00000) libm.so.6 => /lib64/libm.so.6 (0x0000003e45600000) libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x0000003e45e00000) libc.so.6 => /lib64/libc.so.6 (0x0000003e44a00000) /lib64/ld-linux-x86-64.so.2 (0x0000003e44600000) undefined symbol: _ZN6Common4ftoaEfPc (/usr/lib64/libbuffer.so) undefined symbol: _ZN6Common4dtoaEdPcb (/usr/lib64/libbuffer.so)
Ok, now how do I know what's missing?
I'm googling everything I can think of and found some previous bug reports and saw solutions like, "Oh, your missing linking against <library>." But I can't tell how they knew which one was missing!
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Ralf Corsepius rc040203@freenet.de changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |rc040203@freenet.de
--- Comment #13 from Ralf Corsepius rc040203@freenet.de 2011-04-12 13:43:21 EDT --- # c++filt _ZN6Common4ftoaEfPc _ZN6Common4dtoaEdPcb Common::ftoa(float, char*) Common::dtoa(double, char*, bool)
Check for a library or header providing these two symbols.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #14 from Richard Shaw hobbes1069@gmail.com 2011-04-12 14:25:34 EDT --- Ok, strange. The SConscript for libbuffer appears to include the right directory which has header files for Common::ftoa and Common::dtoa.
Here's the SConscript file for libbuffer:
#!/usr/bin/env python Import('env')
libName = 'buffer'
srcDirs = [ 'src/' ]
variantDir = env['objDir'] + env['configurationBaseName'] + '/' outputDir = env['libDir'] + env['configurationBaseName'] + '/' targetPath = outputDir + libName
incDirs = ['include/', '../libftoa/include']
src = [] for srcDir in srcDirs: src += [ variantDir + str(p) for p in Glob(srcDir + '*.cpp')] VariantDir(variant_dir=variantDir + srcDir, src_dir=srcDir, duplicate=False)
if env['SHAREDLIB']: SharedLibrary(target=targetPath, source=src, LINKFLAGS = '-Wl,--soname=libbuffer.so.0', CPPPATH=incDirs, CCFLAGS=env['CPPFLAGS']) else: StaticLibrary(target=targetPath, source=src, CPPPATH=incDirs, CCFLAGS=env['CPPFLAGS']) ---
SHAREDLIB=yes is specified on the scons command in the spec file...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Hans de Goede hdegoede@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |hdegoede@redhat.com Blocks|177841(FE-NEEDSPONSOR) | AssignedTo|nobody@fedoraproject.org |hdegoede@redhat.com
--- Comment #15 from Hans de Goede hdegoede@redhat.com 2011-04-13 10:04:07 EDT --- Hi,
As discussed by email, I'm willing to sponsor Richard. Nicolas will finish the review he has started as far as possible before he goes on a 2 week vacation this friday, and then I'll take over, (re)review, and if all goes well sponsor Richard.
Taking this bug, clearing the needsponsor blocker and changing status to assigned.
Regards,
Hans
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #16 from Richard Shaw hobbes1069@gmail.com 2011-04-13 10:07:53 EDT --- (In reply to comment #9)
The output of rpmlint you have attached mention a lot of undefined-non-weak-symbol. It mean most library are missing the right library link flags at compile time. This is usually something that can be fixed within the build tool (SCons). This have to be reported upstream and is probably rather trivial to fix.
I got an answer from the Suse maintainer.
Basically opencollada is windows only and he did all the work to turn it into a linux library. It's still a work in progress and he's going to see if he can get the non-weak-symbol errors fixed upstream.
I tried looking at the SConscript files but it's over my head.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #17 from Ralf Corsepius rc040203@freenet.de 2011-04-13 13:23:43 EDT --- 2 issues:
* The tarball contains a lot of bundled libraries (cf, Externals/).
This is problematic twice: - In general, the Fedora package should not not use them.
- These packages' licenses need to be checked for whether they are properly licensed and whether these package's licenses are compatible to openCOLLADA's license.
From a coarse glance into tarball, I'd suspect Externals/MathMLSolver not to be
properly licensed (I can't find any licence). Googling however directed me to http://mathmlsolver.svn.sourceforge.net/viewvc/mathmlsolver/trunk, but I haven't checked details, yet.
* The package naming seems inconsistent to me: libOpenCOLLADA vs. OpenCOLLADA-devel
The FPG would recommend using the tarball name, which would mean to name the packages openCOLLADA and openCOLLADA-devel
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #18 from Richard Shaw hobbes1069@gmail.com 2011-04-14 22:29:39 EDT --- (In reply to comment #13)
# c++filt _ZN6Common4ftoaEfPc _ZN6Common4dtoaEdPcb Common::ftoa(float, char*) Common::dtoa(double, char*, bool)
Check for a library or header providing these two symbols.
Thanks for the hint! I actually created a patch (albeit an UGLY patch) for rpmlint to do this for me. Upstream cleaned it up so it will be included in a future release.
https://bugzilla.redhat.com/show_bug.cgi?id=696749
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #19 from Richard Shaw hobbes1069@gmail.com 2011-04-14 22:40:40 EDT --- (In reply to comment #17)
2 issues:
- The tarball contains a lot of bundled libraries (cf, Externals/).
cf?
This is problematic twice:
In general, the Fedora package should not not use them.
These packages' licenses need to be checked for whether they are properly
licensed and whether these package's licenses are compatible to openCOLLADA's license.
From a coarse glance into tarball, I'd suspect Externals/MathMLSolver not to be properly licensed (I can't find any licence). Googling however directed me to http://mathmlsolver.svn.sourceforge.net/viewvc/mathmlsolver/trunk, but I haven't checked details, yet.
Drilled down to the actual SF page at it says it is MIT licensed. How do we handle that? I ran into this problem on RPMFusion and the decision was to put comments above the License: tag explaining which parts had what license.
- The package naming seems inconsistent to me:
libOpenCOLLADA vs. OpenCOLLADA-devel
The FPG would recommend using the tarball name, which would mean to name the packages openCOLLADA and openCOLLADA-devel
The current naming was how the Suse maintainer set it up and I'm sure their rules differ in many areas. For the purpose of the Fedora package I'll change the name.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #20 from Ralf Corsepius rc040203@freenet.de 2011-04-14 23:08:43 EDT --- (In reply to comment #19)
(In reply to comment #17)
2 issues:
- The tarball contains a lot of bundled libraries (cf, Externals/).
cf?
Compare for the contents of the directory called Externals/ inside of the tarball. It contains zlib, Cg,, libxml, lib3ds and other libraries.
A more detailed look into the package tells that openCOLLADA currently only uses MathMLSolver/ and UTF/.
=> Make sure the other directories are not being used when building for Fedora. Brute-force way to do so would be to remove them in %prep (This is what a recent change to the FPG recommends).
This is problematic twice:
In general, the Fedora package should not not use them.
These packages' licenses need to be checked for whether they are properly
licensed and whether these package's licenses are compatible to openCOLLADA's license.
From a coarse glance into tarball, I'd suspect Externals/MathMLSolver not to be properly licensed (I can't find any licence). Googling however directed me to http://mathmlsolver.svn.sourceforge.net/viewvc/mathmlsolver/trunk, but I haven't checked details, yet.
Drilled down to the actual SF page at it says it is MIT licensed.
Yes. I meanwhile also found some copyright notices in Externals/MathMLSolver's headers and found openCOLLADA/Externals/MathMLSolver to be a hacked up version of the code on sourceforge.
I don't know why openCOLLADA is doing so - Could be they are "just hacking" and don't care about proper integration/packaging, could be the sourceforge project is dead. AFAICT, googling indicates openCOLLADA is the only user of MathMLSolver while the sourceforge project might be dead.
How do we handle that?
The formal way would be to ask upstream to add the license file.
I ran into this problem on RPMFusion and the decision was to put comments above the License: tag explaining which parts had what license.
Yes, this is one option to handle such cases.
- The package naming seems inconsistent to me:
libOpenCOLLADA vs. OpenCOLLADA-devel
The FPG would recommend using the tarball name, which would mean to name the packages openCOLLADA and openCOLLADA-devel
The current naming was how the Suse maintainer set it up and I'm sure their rules differ in many areas. For the purpose of the Fedora package I'll change the name.
If you want to add "SUSE compatibility" you can add corresponding "Provides: ...".
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #21 from Richard Shaw hobbes1069@gmail.com 2011-04-20 15:13:16 EDT --- (In reply to comment #20)
(In reply to comment #19)
(In reply to comment #17)
2 issues:
- The tarball contains a lot of bundled libraries (cf, Externals/).
cf?
Compare for the contents of the directory called Externals/ inside of the tarball. It contains zlib, Cg,, libxml, lib3ds and other libraries.
A more detailed look into the package tells that openCOLLADA currently only uses MathMLSolver/ and UTF/.
=> Make sure the other directories are not being used when building for Fedora. Brute-force way to do so would be to remove them in %prep (This is what a recent change to the FPG recommends).
I have done this and it still builds.
This is problematic twice:
In general, the Fedora package should not not use them.
These packages' licenses need to be checked for whether they are properly
licensed and whether these package's licenses are compatible to openCOLLADA's license.
From a coarse glance into tarball, I'd suspect Externals/MathMLSolver not to be properly licensed (I can't find any licence). Googling however directed me to http://mathmlsolver.svn.sourceforge.net/viewvc/mathmlsolver/trunk, but I haven't checked details, yet.
Drilled down to the actual SF page at it says it is MIT licensed.
Yes. I meanwhile also found some copyright notices in Externals/MathMLSolver's headers and found openCOLLADA/Externals/MathMLSolver to be a hacked up version of the code on sourceforge.
I don't know why openCOLLADA is doing so - Could be they are "just hacking" and don't care about proper integration/packaging, could be the sourceforge project is dead. AFAICT, googling indicates openCOLLADA is the only user of MathMLSolver while the sourceforge project might be dead.
Yes, I haven't seen much activity on their SF page so I would assume it's dead.
As far as UTF goes. It looks to be a copy of ConvertUTF which also appears to be defunct. I found a ftp link for the code on unicode.org through:
http://gears.googlecode.com/svn/trunk/third_party/convert_utf/README.google
If there is nothing happening upstream for these two libraries, are they OK being bundled in openCOLLADA?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #22 from Hans de Goede hdegoede@redhat.com 2011-04-21 04:55:52 EDT --- Hi,
(In reply to comment #21)
If there is nothing happening upstream for these two libraries, are they OK being bundled in openCOLLADA?
As long as openCOLLADA is the only user of them, keeping them bundled is fine in this specific case. If however other users turn up in Fedora (unlikely). Things should be coordinated with the package owner of those other users and they should be unbundled.
Regards,
Hans
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #23 from Richard Shaw hobbes1069@gmail.com 2011-04-21 10:32:21 EDT --- Sounds good. A couple of questions:
1. Should we add virtual provides for the two bundled libraries? We'll have to figure out what their version are or fake something.
2. Where do we go from here?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #24 from Hans de Goede hdegoede@redhat.com 2011-04-21 11:06:38 EDT --- Hi,
(In reply to comment #23)
Sounds good. A couple of questions:
- Should we add virtual provides for the two bundled libraries? We'll have to
figure out what their version are or fake something.
No need for virtual provides, so also no need to worry about versions :)
- Where do we go from here?
You do a new spec + srpm fixing as much mentioned issues, then I do a full review and then we go from there.
Regards,
Hans
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #25 from Richard Shaw hobbes1069@gmail.com 2011-04-21 14:14:27 EDT --- (In reply to comment #24)
No need for virtual provides, so also no need to worry about versions :)
Yeah, I just thought about it. Since the libraries are not in Fedora separately, there's nothing to track.
You do a new spec + srpm fixing as much mentioned issues, then I do a full review and then we go from there.
I can post what I have now, but I think the only major issue left is all the undefined-non-weak-symbol warnings from rpmlint.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #26 from Hans de Goede hdegoede@redhat.com 2011-04-21 14:20:11 EDT --- (In reply to comment #25)
You do a new spec + srpm fixing as much mentioned issues, then I do a full review and then we go from there.
I can post what I have now
Yes please.
but I think the only major issue left is all the undefined-non-weak-symbol warnings from rpmlint.
I can probably help with those, once I've an srpm of your latest work to play around with :)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #27 from Richard Shaw hobbes1069@gmail.com 2011-04-21 14:44:50 EDT --- NEW SPEC URL: https://docs.google.com/leaf?id=0B4A6of0Rl4nUZWVlOWE3ZGEtNDdiMi00MWZmLWE5NmQ...
NEW SRC RPM URL: https://docs.google.com/leaf?id=0B4A6of0Rl4nUM2IwMzQyOTMtZWZjZS00ZTNmLWEzZTc...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Hans de Goede hdegoede@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-review?
--- Comment #28 from Hans de Goede hdegoede@redhat.com 2011-04-23 11:42:50 EDT --- Full review done, results below:
Good: ===== - rpmlint checks return: openCOLLADA.src: W: spelling-error Summary(en_US) Collada -> Collard openCOLLADA.src: W: spelling-error %description -l en_US opensource -> open source, open-source, outsource openCOLLADA.src: W: spelling-error %description -l en_US validator -> lavatorial openCOLLADA.src: W: spelling-error %description -l en_US xml -> XML, ml, x ml openCOLLADA.src:21: W: macro-in-comment %{name} openCOLLADA.src:21: W: macro-in-comment %{AGE} openCOLLADA.src: W: invalid-url Source0: openCOLLADA-svn838.tar.xz openCOLLADA.x86_64: W: spelling-error Summary(en_US) Collada -> Collard openCOLLADA.x86_64: W: spelling-error %description -l en_US opensource -> open source, open-source, outsource openCOLLADA.x86_64: W: spelling-error %description -l en_US validator -> lavatorial openCOLLADA.x86_64: W: spelling-error %description -l en_US xml -> XML, ml, x ml openCOLLADA.x86_64: W: incoherent-version-in-changelog 0.0-3 ['0-3.svn838.fc15', '0-3.svn838'] openCOLLADA-devel.x86_64: W: no-documentation These can all be ignored. - package meets naming guidelines - package meets packaging guidelines - license (MIT) OK, text in %doc, matches source - spec file legible, in am. english - source matches upstream - package compiles on devel (x86) - no missing BR - no unnecessary BR - no locales - not relocatable - owns all directories that it creates - no duplicate files - permissions ok - %clean ok - macro use consistent - code, not content - no need for -docs - nothing in %doc affects runtime - no need for .desktop file - devel package ok - no .la files - post/postun ldconfig ok
Needs work: ===========
Ok, I'm going to divide this into 2 lists, one with small things and one with large things. First the small things: - rpmlint checks return: openCOLLADA.src:108: W: mixed-use-of-spaces-and-tabs (spaces: line 9, tab: line 108) Please don't use tabs in spec files.
openCOLLADA.x86_64: W: file-not-utf8 /usr/share/doc/openCOLLADA-0/AUTHORS You will need to convert this to utf8, add the following to %prep: iconv -f ISO_8859-1 -t utf-8 COLLADAStreamWriter/AUTHORS > \ COLLADAStreamWriter/AUTHORS.tmp touch -r COLLADAStreamWriter/AUTHORS COLLADAStreamWriter/AUTHORS.tmp mv COLLADAStreamWriter/AUTHORS.tmp COLLADAStreamWriter/AUTHORS
- please drop the following comment (it is the same as Source0, so I see no use for it): #Source0: %{name}-svn%{AGE}.tar.xz
- please drop the following (already commented) command from the spec file: #strip --strip-debug *.so.* Stripping should *never* be done inside the spec file.
- please add -p to the invocation of cp for the doc files, to keep the original timestamps. Also why do you cp LICENSE, where as for AUTHORS you reference it with its full path ? This seems inconsistent.
- please add -k to the invocation of dos2unix to keep the original timestamps on the files
- Group: Applications/Productivity Is not really appropriate, please use: Group: System Environment/Libraries For the main package instead.
- # copy CHANGES.txt install -p -m 0644 %{S:1} ./
This belongs in %prep IMHO
- Add -p to invocation of cp for installation of header files to preserve the timestamps on the files
- devel does not requires base package n-v-r Please change the: Requires: %{name} = %{version} For the -devel sub-package into: Requires: %{name} = %{version}-%{release}
- I don't like the way the changelog is formatted, here is a copy paste from another review with similar issues: "Please put a blank line in the changelog before each data-name-version line (except for the first). And prefix / itemize items below the data-name-version line with "- ". IE change: * Sun Sep 26 2010 Elad Alfassa el.il@doom.co.il - 0.6-3 Fix typo. * Sun Sep 26 2010 Elad Alfassa el.il@doom.co.il - 0.6-2 Mark schemas file as config file * Sun Sep 26 2010 Elad Alfassa el.il@doom.co.il - 0.6-1 More spec file fixes * Thu Sep 23 2010 Elad Alfassa elad@macron.co.il - 0.6-0 Version update, some general spec file fixes. * Sun Jul 25 2010 Elad Alfassa elad@macron.co.il - 0.3-1 initial build to: * Sun Sep 26 2010 Elad Alfassa el.il@doom.co.il - 0.6-3 - Fix typo.
* Sun Sep 26 2010 Elad Alfassa el.il@doom.co.il - 0.6-2 - Mark schemas file as config file
* Sun Sep 26 2010 Elad Alfassa el.il@doom.co.il - 0.6-1 - More spec file fixes
* Thu Sep 23 2010 Elad Alfassa elad@macron.co.il - 0.6-0 - Version update, some general spec file fixes.
* Sun Jul 25 2010 Elad Alfassa elad@macron.co.il - 0.3-1 -initial build"
***
And then now the large things.
1) You're using a soname of lib*.so.0 for all the libs, given that this is all c++ code and not with official releases from upstream, I think it would be good to embed the svn snapshot nr into the soname. This means that any packages using openCOLLADA will need to be rebuilt each time you rebase to a new svn snapshot, but that seems the safest as I don't believe upstream will keep a stable ABI. So instead of adding: -Wl,--soname=libOpenCOLLADAFoo.so.0 As LINKFLAGS you should add (and install the files as): -Wl,--soname=libOpenCOLLADAFoo-$(svn-version).so (This is using libtool's convention for embedding an exact version into the soname)
2) openCOLLADA-buildflags.patch is an unacceptable hack, the reason it is unacceptable is that the RPM_OPT_FLAGS can differ per arch, so you cannot hardcode them like this. Take a look at this spec file for another way to deal with this: http://pkgs.fedoraproject.org/gitweb/?p=rafkill.git;a=blob_plain;f=rafkill.s... Note their might be an easier way ...
Also I wonder why exactly libOpenCOLLADASaxFrameworkLoader cannot build with -O2 only -O0 ?
3) libUTF, libbuffer and libftoa are very generic names, I would like to see these renamed to libopenCOLLADAFoo names.
4) And then there is the issue with the unresolved symbols I've run out of steam (and time) for now to look into that (blergh scons), maybe I'll look into it later today. If not feel free to do a new version in the mean time addressing all the other concerns.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #29 from Richard Shaw hobbes1069@gmail.com 2011-04-24 12:39:42 EDT --- (In reply to comment #28)
- And then there is the issue with the unresolved symbols I've run out of
steam (and time) for now to look into that (blergh scons), maybe I'll look into it later today. If not feel free to do a new version in the mean time addressing all the other concerns.
I've been working with Dave Platter, the more or less creator of the *nix version of this library and Suse package maintainer, and he's started working on the undefined-non-weak-symbol issues.
Apparently Suse doesn't use rpmlint so he wasn't aware of the problem. He fixed two of the errors by updating the SConscript files. I'll attach the patch as a template.
Two down, 243 to go.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #30 from Richard Shaw hobbes1069@gmail.com 2011-04-24 12:40:22 EDT --- Created attachment 494541 --> https://bugzilla.redhat.com/attachment.cgi?id=494541 Patch to fix undefined-non-weak-symbol issues in libbuffer.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #31 from Hans de Goede hdegoede@redhat.com 2011-04-26 15:30:29 EDT --- Created attachment 495019 --> https://bugzilla.redhat.com/attachment.cgi?id=495019 PATCH: add soname to cmake build files, hide util libs
Hi,
So I've been working on the weak non resolved symbols issue, and I noticed there were CMakeList files already present, and they already contained all the bits needed to properly link the libs to each other to avoid the weak non resolved symbols issue.
So I gave the cmake buildsys a spin and it works well, working with cmake is so much easier then scons :)
So I did a small patch fixing 2 things with the cmake buildsys: 1) Add a proper soname to the build libraries 2) the libBuffer libftoa and libUTF8 libs were only being used in one other openCOLLADA lib each, and have very generic names. So instead of building them as libs, I've changed the buildsys to embed them into the libs using them.
If you use the attached patch (and drop the sconstruct patches) and replace the %build section with: %cmake -DUSE_STATIC=OFF -DUSE_SHARED=ON -Dsoversion=svn%{AGE} # Note building with _smp_mflags does not work for some reason ... make
Then you'll find properly build libs (including soname + symlinks) under the lib subdir, make install does not work, it looks like the cmake files need some work to get this to work, but we can just do it manually for now.
Using cmake with this patch seems to fix all "large things" mentioned in my full review comment :) I hope you're ok with moving to cmake for building ...
Regards,
Hans
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #32 from Richard Shaw hobbes1069@gmail.com 2011-04-26 15:39:49 EDT --- (In reply to comment #31)
Using cmake with this patch seems to fix all "large things" mentioned in my full review comment :) I hope you're ok with moving to cmake for building ...
Not at all! In fact Dave and I both prefer it but he hadn't had time to learn more cmake so he could figure out what was missing and I don't know it near well enough to take it on.
I'll try it out tonight!
Richard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #33 from Richard Shaw hobbes1069@gmail.com 2011-04-26 16:09:25 EDT --- (In reply to comment #28)
Needs work:
Ok, I'm going to divide this into 2 lists, one with small things and one with large things. First the small things:
- rpmlint checks return: openCOLLADA.src:108: W: mixed-use-of-spaces-and-tabs (spaces: line 9, tab:
line 108) Please don't use tabs in spec files.
I couldn't find the tab but I ran across 'expand' which seemed to work quite nicely to fix the problem.
openCOLLADA.x86_64: W: file-not-utf8 /usr/share/doc/openCOLLADA-0/AUTHORS You will need to convert this to utf8, add the following to %prep: iconv -f ISO_8859-1 -t utf-8 COLLADAStreamWriter/AUTHORS > \ COLLADAStreamWriter/AUTHORS.tmp touch -r COLLADAStreamWriter/AUTHORS COLLADAStreamWriter/AUTHORS.tmp mv COLLADAStreamWriter/AUTHORS.tmp COLLADAStreamWriter/AUTHORS
Done.
- please drop the following comment (it is the same as Source0, so I see no use for it): #Source0: %{name}-svn%{AGE}.tar.xz
Already gone. Sorry, I left that in there when I was playing with different compression (gz, xz, bz2)
- please drop the following (already commented) command from the spec file: #strip --strip-debug *.so.* Stripping should *never* be done inside the spec file.
That's left over from Suse. It was always commented out for me. Fixed.
- please add -p to the invocation of cp for the doc files, to keep the original
Fixed.
timestamps. Also why do you cp LICENSE, where as for AUTHORS you reference it with its full path ? This seems inconsistent.
Not sure, unless Dave just wanted AUTHORS in the subdir and LICENSE in the root. I don't see why AUTHORS couldn't be moved to the install root (is that the right terminology?)
- please add -k to the invocation of dos2unix to keep the original timestamps on the files
Done.
- Group: Applications/Productivity Is not really appropriate, please use: Group: System Environment/Libraries For the main package instead.
Apparently Suse has A LOT MORE groups than we do. I guessed on this one but that was before I really knew what I was doing :)
- # copy CHANGES.txt install -p -m 0644 %{S:1} ./ This belongs in %prep IMHO
Done.
- Add -p to invocation of cp for installation of header files to preserve the timestamps on the files
This is already recursive. Would it be appropriate to just use -a here?
- devel does not requires base package n-v-r Please change the: Requires: %{name} = %{version} For the -devel sub-package into: Requires: %{name} = %{version}-%{release}
Done.
- I don't like the way the changelog is formatted, here is a copy paste from another review with similar issues:
Done.
Richard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #34 from Richard Shaw hobbes1069@gmail.com 2011-04-26 17:40:03 EDT --- Ok, small update.
Apparently cmake installs all the libs a ./lib directory in the build dir so the find command was no longer necessary. Took that out and it built fine.
New problem. I'm getting rpath's in the resultant libraries. I added CMAKE_SKIP_RPATH to the cmake command but don't have time to build the package just yet to see if it works. Hopefully it's that simple.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #35 from Richard Shaw hobbes1069@gmail.com 2011-04-26 18:48:00 EDT --- Rebuilt now that I'm home from work. Here's the new rpmlint output from the installed package: (MUCH SMALLER!)
# rpmlint openCOLLADA openCOLLADA.x86_64: W: spelling-error Summary(en_US) Collada -> Collard, Collate, Collage openCOLLADA.x86_64: W: spelling-error %description -l en_US opensource -> open source, open-source, outsource openCOLLADA.x86_64: W: spelling-error %description -l en_US validator -> invalidator, validation, validate openCOLLADA.x86_64: W: spelling-error %description -l en_US xml -> XML, cml, ml openCOLLADA.x86_64: W: incoherent-version-in-changelog 0.0-3 ['0-4.svn838.fc14', '0-4.svn838'] openCOLLADA.x86_64: E: invalid-soname /usr/lib64/libOpenCOLLADABaseUtils.so.0.838.0 libOpenCOLLADABaseUtils.so.svn838 openCOLLADA.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libOpenCOLLADABaseUtils.so.0.838.0 /usr/lib64/libpcreposix.so.0 openCOLLADA.x86_64: E: invalid-soname /usr/lib64/libOpenCOLLADAStreamWriter.so.0.838.0 libOpenCOLLADAStreamWriter.so.svn838 openCOLLADA.x86_64: E: invalid-soname /usr/lib64/libMathMLSolver.so.0.838.0 libMathMLSolver.so.svn838 openCOLLADA.x86_64: E: invalid-soname /usr/lib64/libGeneratedSaxParser.so.0.838.0 libGeneratedSaxParser.so.svn838 openCOLLADA.x86_64: E: invalid-soname /usr/lib64/libOpenCOLLADASaxFrameworkLoader.so.0.838.0 libOpenCOLLADASaxFrameworkLoader.so.svn838 openCOLLADA.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libOpenCOLLADASaxFrameworkLoader.so.0.838.0 /usr/lib64/libpcreposix.so.0 openCOLLADA.x86_64: E: invalid-soname /usr/lib64/libOpenCOLLADAFramework.so.0.838.0 libOpenCOLLADAFramework.so.svn838 openCOLLADA.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libOpenCOLLADAFramework.so.0.838.0 /usr/lib64/libMathMLSolver.so.svn838 1 packages and 0 specfiles checked; 6 errors, 8 warnings.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #36 from Hans de Goede hdegoede@redhat.com 2011-04-27 01:38:38 EDT --- (In reply to comment #33)
(In reply to comment #28)
- Add -p to invocation of cp for installation of header files to preserve the timestamps on the files
This is already recursive. Would it be appropriate to just use -a here?
Yes
(In reply to comment #34)
New problem. I'm getting rpath's in the resultant libraries. I added CMAKE_SKIP_RPATH to the cmake command but don't have time to build the package just yet to see if it works. Hopefully it's that simple.
I woke up thinking this morning if we don't use cmake for make install, we need to add this magic value to disable rpath, I'm happy to see you already figured this out yourself :)
(In reply to comment #35) <snip>
openCOLLADA.x86_64: E: invalid-soname /usr/lib64/libOpenCOLLADABaseUtils.so.0.838.0 libOpenCOLLADABaseUtils.so.svn838
The lib files should be installed under the same name as you can find them under the ./lib dir, actually you can just do: mkdir -p $RPM_BUILD_ROOT%{_libdir} cp -a lib/* $RPM_BUILD_ROOT%{_libdir}
For the so files (still need to also cp the include dirs) from %install since cmake also puts the needed symlinks, etc. under ./lib. Talking about the include dirs, you should now no longer install the include dirs for libBuffer, libftoa and libUTF.
The %files entry for the so files in the main package then becomes:
%{_libdir}/lib*.so.svn%{AGE}
Oh, one other small thing I just noticed, instead of: %post -n openCOLLADA -p /sbin/ldconfig
%postun -n openCOLLADA -p /sbin/ldconfig
You can just write: %post -p /sbin/ldconfig %postun -p /sbin/ldconfig
There is no need for the -n openCOLLADA since that is the main package's name.
With all previous fixes + the just cp -a lib thingie I think we're 99% there, if you can provide a new spec file + srpm I'll check it out and either approve it right away or make a hopefully small list of things left to fix :)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #37 from Richard Shaw hobbes1069@gmail.com 2011-04-27 09:28:34 EDT --- (In reply to comment #36)
(In reply to comment #33)
(In reply to comment #28)
- Add -p to invocation of cp for installation of header files to preserve the timestamps on the files
This is already recursive. Would it be appropriate to just use -a here?
Yes
Done.
(In reply to comment #35)
<snip>
openCOLLADA.x86_64: E: invalid-soname /usr/lib64/libOpenCOLLADABaseUtils.so.0.838.0 libOpenCOLLADABaseUtils.so.svn838
The lib files should be installed under the same name as you can find them under the ./lib dir, actually you can just do: mkdir -p $RPM_BUILD_ROOT%{_libdir} cp -a lib/* $RPM_BUILD_ROOT%{_libdir}
For the so files (still need to also cp the include dirs) from %install since cmake also puts the needed symlinks, etc. under ./lib. Talking about the include dirs, you should now no longer install the include dirs for libBuffer, libftoa and libUTF.
The %files entry for the so files in the main package then becomes:
%{_libdir}/lib*.so.svn%{AGE}
Shouldn't I just make this "%{_libdir}/lib*"? Otherwise it doesn't pick up the symbolic link .so files.
Oh, one other small thing I just noticed, instead of: %post -n openCOLLADA -p /sbin/ldconfig
%postun -n openCOLLADA -p /sbin/ldconfig
You can just write: %post -p /sbin/ldconfig %postun -p /sbin/ldconfig
There is no need for the -n openCOLLADA since that is the main package's name.
With all previous fixes + the just cp -a lib thingie I think we're 99% there, if you can provide a new spec file + srpm I'll check it out and either approve it right away or make a hopefully small list of things left to fix :)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #38 from Richard Shaw hobbes1069@gmail.com 2011-04-27 09:31:08 EDT --- (In reply to comment #37)
(In reply to comment #36)
The %files entry for the so files in the main package then becomes:
%{_libdir}/lib*.so.svn%{AGE}
Shouldn't I just make this "%{_libdir}/lib*"? Otherwise it doesn't pick up the symbolic link .so files.
Nevermind. I guess those are only used for the -devel package?
Richard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #39 from Hans de Goede hdegoede@redhat.com 2011-04-27 09:33:28 EDT --- Hi,
(In reply to comment #37)
The lib files should be installed under the same name as you can find them under the ./lib dir, actually you can just do: mkdir -p $RPM_BUILD_ROOT%{_libdir} cp -a lib/* $RPM_BUILD_ROOT%{_libdir}
For the so files (still need to also cp the include dirs) from %install since cmake also puts the needed symlinks, etc. under ./lib. Talking about the include dirs, you should now no longer install the include dirs for libBuffer, libftoa and libUTF.
The %files entry for the so files in the main package then becomes:
%{_libdir}/lib*.so.svn%{AGE}
Shouldn't I just make this "%{_libdir}/lib*"? Otherwise it doesn't pick up the symbolic link .so files.
The .so files belong in the -devel sub package, where as the main package gets the lib*.so.svn%{AGE} files, the .so symlinks are used to determine which lib to link to when building only, they are not used runtime (the dynamic linker searches for the soname runtime, which is the lib*.so.svn%{AGE} file).
Regards,
Hans
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #40 from Hans de Goede hdegoede@redhat.com 2011-04-27 09:33:44 EDT --- Right (comments crossed).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #41 from Richard Shaw hobbes1069@gmail.com 2011-04-27 09:47:01 EDT --- Ok. Getting close.
1. Right now I'm getting the wrong library permissions. 775 instead of 755. I'm guessing I could fix this with install instead of cp -a but is there an easy way to tell cmake to do the right thing?
2. Ran across this[1] page saying we shouldn't use CMAKE_SKIP_RPATH but I didn't understand the work around.
Richard
[1] http://fedoraproject.org/wiki/Packaging:Cmake
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #42 from Richard Shaw hobbes1069@gmail.com 2011-04-27 09:48:15 EDT --- (In reply to comment #41)
- Ran across this[1] page saying we shouldn't use CMAKE_SKIP_RPATH but I
didn't understand the work around. [1] http://fedoraproject.org/wiki/Packaging:Cmake
Nevermind. Read it again and noticed it's supposed to be handled in by make install but we're not using it.
Richard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #43 from Richard Shaw hobbes1069@gmail.com 2011-04-27 10:18:15 EDT --- Ok, fixed the permissions on the libraries with "install -p -m 0755..." left the symlinks as "cp -a" since they need 777, right?
Rpmlint output pretty clean now on everything.
NEW SRPMS URL: https://docs.google.com/leaf?id=0B4A6of0Rl4nUMjJhZDNkNzAtNzI2MC00MmIyLTkyMTk...
SPEC URL Not affected.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #44 from Hans de Goede hdegoede@redhat.com 2011-04-27 10:58:17 EDT --- Hi,
(In reply to comment #41)
- Right now I'm getting the wrong library permissions. 775 instead of 755. I'm
guessing I could fix this with install instead of cp -a but is there an easy way to tell cmake to do the right thing?
Weird, it does not do that for me, maybe a difference in the cmake version we have (I'm on Fedora 15).
(In reply to comment #43)
Ok, fixed the permissions on the libraries with "install -p -m 0755..."
Good
left the symlinks as "cp -a" since they need 777, right?
symlinks are always 777, but anything but cp -a will probably copy / install the files they point to rather then the links, so keeping cp -a for them is the right thing to do.
NEW SRPMS URL: https://docs.google.com/leaf?id=0B4A6of0Rl4nUMjJhZDNkNzAtNzI2MC00MmIyLTkyMTk...
Almost there, but still needs some work:
* Please drop everything in the spec file above Name: except for the %define of AGE, the rest is no longer needed
* Please drop Patch1 and Patch2 they are not needed, they patch SConscript files which are only used by scons ...
* Please drop "License: MIT" from the -devel subpackage, since it is the same as for the main package (I missed that before)
* Remove %{?_smp_mflags} from your make invocation, otherwise the build fails at least it does so consistently on my quad core. Funny how you kept the comment from me snippet saying that the build breaks, but re-added the %{?_smp_mflags} :)
* You're still installing header files from the common dir, resulting in unittest and performance test headers ending up under /usr/include, but see the next item for a more radical suggestion for re-arranging the headers.
* And last, one slightly larger issue (which I should have checked before). I'm not really happy with putting a bunch of the .h files directly under /usr/include. Ideally (IMHO) COLLADAfoo/include/* should end up as /usr/include/COLLADAfoo/* for all variants of foo
Regards,
Hans
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #45 from Richard Shaw hobbes1069@gmail.com 2011-04-27 11:17:40 EDT --- (In reply to comment #44)
Almost there, but still needs some work:
- Please drop everything in the spec file above Name: except for the %define of AGE, the rest is no longer needed
Done.
- Please drop Patch1 and Patch2 they are not needed, they patch SConscript files which are only used by scons ...
I think you mean patch 0 and patch1 since patch2 is your patch. Done.
- Please drop "License: MIT" from the -devel subpackage, since it is the same as for the main package (I missed that before)
Done.
- Remove %{?_smp_mflags} from your make invocation, otherwise the build fails at least it does so consistently on my quad core. Funny how you kept the comment from me snippet saying that the build breaks, but re-added the %{?_smp_mflags} :)
When you said it didn't work I didn't think you meant failed to build, just that it didn't do anything. It works for me on my dual core with -j4. Maybe we should spec a max number of threads conditional?
- You're still installing header files from the common dir, resulting in unittest and performance test headers ending up under /usr/include, but see the next item for a more radical suggestion for re-arranging the headers.
Should I remove common in addition to the below suggestion? What about Externals/MathMLSolver?
- And last, one slightly larger issue (which I should have checked before). I'm not really happy with putting a bunch of the .h files directly under /usr/include. Ideally (IMHO) COLLADAfoo/include/* should end up as /usr/include/COLLADAfoo/* for all variants of foo
Hmm. This sounds like a job for bash and my bashfoo is not that strong :)
I'm thinking some sort of for loop that either inverts foo/include to include/foo or that strips the ./include off before copying the files.
Another option would be to create a /usr/include/openCOLLADA and then just dump all the files and/or directories under that...
What do you think?
Richard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #46 from Hans de Goede hdegoede@redhat.com 2011-04-27 11:47:37 EDT --- Hi again,
(In reply to comment #45)
(In reply to comment #44)
<snip>
- Remove %{?_smp_mflags} from your make invocation, otherwise the build fails at least it does so consistently on my quad core. Funny how you kept the comment from me snippet saying that the build breaks, but re-added the %{?_smp_mflags} :)
When you said it didn't work I didn't think you meant failed to build, just that it didn't do anything. It works for me on my dual core with -j4. Maybe we should spec a max number of threads conditional?
No, there is some race condition in there somehow, so anything > 1 is too much. Maybe it only gets triggered with a newer cmake version, who knows ?
Anyways it is best to just not use %{?_smp_mflags} in this case to avoid build failures on for example the buildsystem.
- You're still installing header files from the common dir, resulting in unittest and performance test headers ending up under /usr/include, but see the next item for a more radical suggestion for re-arranging the headers.
Should I remove common in addition to the below suggestion?
s/remove/not install headers from/ -> yes
What about Externals/MathMLSolver?
I think it is ok to include the headers for that.
- And last, one slightly larger issue (which I should have checked before). I'm not really happy with putting a bunch of the .h files directly under /usr/include. Ideally (IMHO) COLLADAfoo/include/* should end up as /usr/include/COLLADAfoo/* for all variants of foo
Hmm. This sounds like a job for bash and my bashfoo is not that strong :)
I'm thinking some sort of for loop that either inverts foo/include to include/foo or that strips the ./include off before copying the files.
Something like this should do the trick:
for i in COLLADABaseUtils COLLADAFramework COLLADAMax COLLADAMaya \ COLLADASaxFrameworkLoader COLLADAStreamWriter GeneratedSaxParser; do mkdir -p $RPM_BUILD_ROOT%{_includedir}/$i cp -a $i/include/* $RPM_BUILD_ROOT%{_includedir}/$i done
And then "manually" do Externals/MathMLSolver since that is a subdir of a subdir
Another option would be to create a /usr/include/openCOLLADA and then just dump all the files and/or directories under that... What do you think?
I think it is good to keep the headers grouped in the way they are grouped already in the sources. You could replace %{_includedir} with %{_includedir}/openCOLLADA in the above script, but I don't think that is necessary.
Regards,
Hans
p.s.
One other thing, it might be good to also package the COLLADAValidator package in a -utils subpackage, as that may be a useful utility to have.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #47 from Richard Shaw hobbes1069@gmail.com 2011-04-27 14:17:46 EDT --- (In reply to comment #46)
Anyways it is best to just not use %{?_smp_mflags} in this case to avoid build failures on for example the buildsystem.
Removed.
- You're still installing header files from the common dir, resulting in unittest and performance test headers ending up under /usr/include, but see the next item for a more radical suggestion for re-arranging the headers.
Should I remove common in addition to the below suggestion?
s/remove/not install headers from/ -> yes
Done.
Something like this should do the trick:
for i in COLLADABaseUtils COLLADAFramework COLLADAMax COLLADAMaya \ COLLADASaxFrameworkLoader COLLADAStreamWriter GeneratedSaxParser; do mkdir -p $RPM_BUILD_ROOT%{_includedir}/$i cp -a $i/include/* $RPM_BUILD_ROOT%{_includedir}/$i done
And then "manually" do Externals/MathMLSolver since that is a subdir of a subdir
I got it all in one by doing this: for dir in COLLADABaseUtils COLLADAFramework COLLADASaxFrameworkLoader \ COLLADAStreamWriter COLLADAValidator Externals/MathMLSolver \ GeneratedSaxParser; do mkdir -p %{buildroot}%{_includedir}/$(basename $dir) cp -a $dir/include/* %{buildroot}%{_includedir}/$(basename $dir) done
p.s.
One other thing, it might be good to also package the COLLADAValidator package in a -utils subpackage, as that may be a useful utility to have.
Done. While we on the subject of sub-packages. Should we also make a separate -doc package?
Richard
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #48 from Richard Shaw hobbes1069@gmail.com 2011-04-27 14:34:52 EDT --- Went ahead and created a -doc package.
Also, I ran rpmlint on all the resulting packages and saw something new:
openCOLLADA-debuginfo.x86_64: E: debuginfo-without-sources
Maybe it's always been there since I haven't checked the debuginfo package before.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #49 from Hans de Goede hdegoede@redhat.com 2011-04-27 15:36:58 EDT --- (In reply to comment #47) <snip>
Done. While we on the subject of sub-packages. Should we also make a separate -doc package?
No I don't think that is necessary, given that the docs are not all that large.
(In reply to comment #48)
Went ahead and created a -doc package.
Hehe, well if you prefer having the htdocs in a separate sub package that is fine. note that things like README and LICENSE must be in the main package though.
Also, I ran rpmlint on all the resulting packages and saw something new:
openCOLLADA-debuginfo.x86_64: E: debuginfo-without-sources
Ah, yes this is caused by you using cmake rather then %cmake when invoking cmake, I missed that. This causes rpm_opt_flags to not be used, causing this amongst other issues.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #50 from Richard Shaw hobbes1069@gmail.com 2011-04-27 16:18:10 EDT --- (In reply to comment #49)
(In reply to comment #47)
<snip>
Done. While we on the subject of sub-packages. Should we also make a separate -doc package?
No I don't think that is necessary, given that the docs are not all that large. (In reply to comment #48)
Went ahead and created a -doc package.
Hehe, well if you prefer having the htdocs in a separate sub package that is fine. note that things like README and LICENSE must be in the main package though.
I figured since most people will not be direct users (this package will just be a dependency of the program they are actually using), that it would be good to separate the documentation.
Yup, I was careful to only move htdocs to the sub-package.
Also, I ran rpmlint on all the resulting packages and saw something new:
openCOLLADA-debuginfo.x86_64: E: debuginfo-without-sources
Ah, yes this is caused by you using cmake rather then %cmake when invoking cmake, I missed that. This causes rpm_opt_flags to not be used, causing this amongst other issues.
Fixed and rebuilding now. Between taking the smp_flags off and adding whatever the cmake macro adds it's taking MUCH longer to build.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #51 from Richard Shaw hobbes1069@gmail.com 2011-04-27 17:03:10 EDT --- Everything looks good. The debuginfo file now has some size to it :)
New spec and SRC RPM uploaded.
SRC RPM: https://docs.google.com/leaf?id=0B4A6of0Rl4nUY2UxZGQ4ZDEtY2Q5MC00MTNmLWExOTE...
SPEC: https://docs.google.com/leaf?id=0B4A6of0Rl4nUZWVlOWE3ZGEtNDdiMi00MWZmLWE5NmQ...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Hans de Goede hdegoede@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #52 from Hans de Goede hdegoede@redhat.com 2011-04-28 04:31:25 EDT --- Hi,
(In reply to comment #51)
Everything looks good. The debuginfo file now has some size to it :)
New spec and SRC RPM uploaded.
SRC RPM: https://docs.google.com/leaf?id=0B4A6of0Rl4nUY2UxZGQ4ZDEtY2Q5MC00MTNmLWExOTE...
SPEC: https://docs.google.com/leaf?id=0B4A6of0Rl4nUZWVlOWE3ZGEtNDdiMi00MWZmLWE5NmQ...
Looks good now, approved!
One last small thing, but that can be fixed before / after import into git, you copy pasted the Summary for the doc subpackage from the devel doc package and did not adjust it to describe the doc subpackage.
So since this package is approved now it is time to sponsor you, next I'll need to know your FAS account username to create one if needed, see: http://fedoraproject.org/wiki/PackageMaintainers/Join#Get_a_Fedora_Account
Then I'll add you to the packagers group and sponsor you.
Regards,
Hans
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Richard Shaw hobbes1069@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review+ |fedora-review?
--- Comment #53 from Richard Shaw hobbes1069@gmail.com 2011-04-28 08:52:41 EDT --- Woops. Fixed the description, forgot the summary.
FAS account name is hobbes1069.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #54 from Hans de Goede hdegoede@redhat.com 2011-04-28 09:03:52 EDT --- (In reply to comment #53)
Woops. Fixed the description, forgot the summary.
FAS account name is hobbes1069.
Ok, you've been added to the packagers group and sponsored, next step is to do a scm admin request to get a module created in git and bugzilla for openCOLLADA, as described here: http://fedoraproject.org/wiki/Package_SCM_admin_requests
Note that it may take up to an hour for your new packager right to propagate, so if you get an error when you try to set the cvs flag, wait a bit :)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #55 from Richard Shaw hobbes1069@gmail.com 2011-04-28 09:17:04 EDT --- I'm assuming we are not worried about branching for F13, just F14 and F15?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #56 from Hans de Goede hdegoede@redhat.com 2011-04-28 09:29:08 EDT --- (In reply to comment #55)
I'm assuming we are not worried about branching for F13, just F14 and F15?
Yes, I don't think it is likely we will see blender update using this for F13, so no need for an F13 branch.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Richard Shaw hobbes1069@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
--- Comment #57 from Richard Shaw hobbes1069@gmail.com 2011-04-28 09:30:58 EDT --- New Package SCM Request ======================= Package Name: openCOLLADA Short Description: Collada 3D import and export libraries Owners: hobbes1069 Branches: f14 f15 InitialCC:
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #58 from Dennis Gilmore dennis@ausil.us 2011-04-28 12:42:53 EDT --- Git done (by process-git-requests).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #59 from Richard Shaw hobbes1069@gmail.com 2011-04-28 15:48:11 EDT --- Hans,
I'm geting an error even though i've uploaded my public SSH key:
$ fedpkg clone -B openCOLLADA Cloning into bare repository /home/richard/fedora-scm/openCOLLADA/fedpkg.git... Permission denied (publickey). fatal: The remote end hung up unexpectedly Could not clone: Command '['git', 'clone', '--bare', 'ssh://hobbes1069@pkgs.fedoraproject.org/openCOLLADA', '/home/richard/fedora-scm/openCOLLADA/fedpkg.git']' returned non-zero exit status 128
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #60 from Richard Shaw hobbes1069@gmail.com 2011-04-28 15:50:13 EDT --- Nevermind... changed perms to 0400...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #61 from Fedora Update System updates@fedoraproject.org 2011-04-28 19:42:03 EDT --- openCOLLADA-0-5.svn838.fc14 has been submitted as an update for Fedora 14. https://admin.fedoraproject.org/updates/openCOLLADA-0-5.svn838.fc14
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #62 from Fedora Update System updates@fedoraproject.org 2011-04-28 19:42:10 EDT --- openCOLLADA-0-5.svn838.fc15 has been submitted as an update for Fedora 15. https://admin.fedoraproject.org/updates/openCOLLADA-0-5.svn838.fc15
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #63 from Fedora Update System updates@fedoraproject.org 2011-04-29 00:32:09 EDT --- openCOLLADA-0-5.svn838.fc15 has been pushed to the Fedora 15 testing repository.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #64 from Fedora Update System updates@fedoraproject.org 2011-05-03 00:45:07 EDT --- openCOLLADA-0-5.svn838.fc15 has been pushed to the Fedora 15 stable repository.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Fixed In Version| |openCOLLADA-0-5.svn838.fc15 Resolution| |ERRATA Last Closed| |2011-05-03 00:45:13
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #65 from Fedora Update System updates@fedoraproject.org 2011-06-02 06:56:02 EDT --- openCOLLADA-0-5.svn838.fc14 has been pushed to the Fedora 14 stable repository.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version|openCOLLADA-0-5.svn838.fc15 |openCOLLADA-0-5.svn838.fc14
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=694287
--- Comment #66 from Fedora Update System updates@fedoraproject.org 2011-06-02 15:04:11 EDT --- openCOLLADA-0-5.svn838.fc14 has been pushed to the Fedora 14 stable repository.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla