Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 206814
Summary: | Review Request: hugin - Frontend for Panorama Tools, similar to PTAssembler, PTGui or Open for Windows | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Bruno Postle <bruno> |
Component: | Package Review | Assignee: | Jef Spaleta <jspaleta> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | jspaleta, peter, ron, tjb |
Target Milestone: | --- | Flags: | jspaleta:
fedora-review+
petersen: fedora-cvs+ |
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | Bug Fix | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2007-03-21 13:05:11 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Bruno Postle
2006-09-16 22:42:40 UTC
BuildRequires: libpano12-devel > 2.8.3 zlib-devel libtiff-devel libjpeg-devel libpng-devel gettext-devel gcc-c++ BuildRequires: libstdc++-devel wxGTK-devel >= 2.6.0 boost-devel gtk+-devel glib2-devel You don't need gcc-c++ or libstdc++-devel really. Is there a reason for using the old gtk instead of gtk2? # autopanog.exe is a mono app In which case you need the BRs for mono - at least mono-devel and remember, if a mono app is being included, you're limited to FC5 and FC6. Currently, you also need a hack as FC6 mono is architecture agnostic (it get's things right!) and will probably need to fix things in the source. Let me know if you need a hand with it as mono can be a pain at times! %build %configure make Any reason for not using smp_mflags? If it fails, you need to include a comment as to why %{_datadir}/locale/*/LC_MESSAGES/hugin.mo %{_datadir}/locale/*/LC_MESSAGES/nona_gui.mo These are handled incorrectly. Some of the time with findlang, you need to use something other than %{name}. For example, in gnome-build, I use %find_lang gbf-1 Definite NO-NO - you're building static libraries and not shared ones. rpmlint is throwing up a pile of errors, all of which are the same sort of thing E hugin version-control-internal-file /usr/share/doc/hugin-0.6.1/help_en_EN/CVS/Repository (or Entries) You need to sort out the translation problems as well the debuginfo looks likes it needs work on it as well (E: hugin-debuginfo script-without-shellbang /usr/src/debug/hugin-0.6.1/src/vigra_ext/MultiThreadOperations.cpp for example) Not yet made it in mock, will say what I get when I have. Built cleanly in mock (x86) (In reply to comment #1) > > You don't need gcc-c++ or libstdc++-devel really. Is there a reason for using > the old gtk instead of gtk2? I've removed the c++ build dependencies and the gtk+ stuff was left over from when this library did build against gtk+, gone. > # autopanog.exe is a mono app > > In which case you need the BRs for mono Hugin is a front-end for lots of other tools. Autopano is one, but it is peripheral and not a hugin requirement. It is a mono app, so this fix would let it run if a user did decide to install it. > Any reason for not using smp_mflags? None, fixed. > Some of the time with findlang, you need to use something other than %{name}. I've reread the find_lang docs a few more times and I think I have it working, fixed. > E hugin version-control-internal-file Oops, very embarassing typo I added at the last minute, fixed. > E: hugin-debuginfo script-without-shellbang I don't get debuginfo RPMs on x86_64, should be fixed. > Definite NO-NO - you're building static libraries and not shared ones. The hugin sources contain a modified version of the vigra impex library, is that what you mean? Upstream intends to pass the changes further upstream, but until then they are linking it in statically. If it was separated it would conflict with the real vigra library. That's my understanding anyway. Spec URL: http://bugbear.blackfish.org.uk/~bruno/apt/SPECS/hugin.spec SRPM URL: http://bugbear.blackfish.org.uk/~bruno/apt/fedora/linux/5/x86_64/SRPMS.panorama/hugin-0.6.1-3.fc5.src.rpm static linking should not be used http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7 I'm not that happy in this case to use statically linked libs. I'll have a word higher up the food chain... Okay, there are problems. 1. The mono stuff needs to be a subpackage, call it hugin-mono or something 2. You have a desktop icon in %{datadir}/applications but nothing in the spec 3. You have icons and the such as well as mime information in %datadir which is not handled. Have a look at http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets for more information on these 4. ldconfigs not required - no libs being installed Also even on x86_64, you will have had a debuginfo package generated. (In reply to comment #6) > > 1. The mono stuff needs to be a subpackage, call it hugin-mono or something There isn't any mono code in hugin. autopanog is in another package which I won't be submitting as the basic algorithm is US patented and hugin runs fine without it. The fix in the spec is a convenience thing for users who do want to install it. > 2. You have a desktop icon in %{datadir}/applications but nothing in the spec > 3. You have icons and the such as well as mime information in %datadir which is > not handled. Thanks, hadn't found this stuff before. Should be fixed. > 4. ldconfigs not required - no libs being installed Fixed. > Also even on x86_64, you will have had a debuginfo package generated. Nope. I guess there is something wrong with my system as x86_64 RPMs never get stripped and I get no debuginfo packages. I'll ask upstream what the situation is with all the .la libraries that get statically linked. I think this is just a case of programming style, they are not intended to be exported. SPEC URL: http://bugbear.blackfish.org.uk/~bruno/apt/SPECS/hugin.spec SRPM URL: http://bugbear.blackfish.org.uk/~bruno/apt/fedora/linux/5/x86_64/SRPMS.panorama/hugin-0.6.1-4.fc5.src.rpm (In reply to comment #7) > (In reply to comment #6) > > Also even on x86_64, you will have had a debuginfo package generated. > > Nope. I guess there is something wrong with my system as x86_64 RPMs never get > stripped and I get no debuginfo packages. Do you have the redhat-rpm-config package installed? (In reply to comment #8) > > Do you have the redhat-rpm-config package installed? I do now ;-) I had it on my old i386 box, but not here. I missed it because it isn't mentioned in the Packaging/Guidelines. (In reply to comment #7) > > I'll ask upstream what the situation is with all the .la libraries that get > statically linked. Upstream says that the static libs are just the way the code is structured internally. They are not intended to be exported, which is why shared libraries are not built. The exception is the the vigra code, this is a modified fork. Upstream has submitted these changes to the vigra maintainer and expects that they will be in the next release, at which point this code will be removed from hugin. Note that I already built vigra for extras but vigra has a very long release cycle. PING! Where do the packaging issues stand at this point? I ran across hugin recently and I'd very much like if we can get hugin into the package collection. It's been 4 months since the last comment, so I'd appreciate a recap as to what the outstanding blocker issues are, and I'd like to know if the original submitter is still willing to maintain the package. -jef I'd very much like to get the hugin/enblend pair into fedora, not least because I have a hundred or so people using my third party repo and I need to get them moved over. This review has stalled because the hugin sources include a modified version of vigra rather than using the vigra already in extras. Upstream doesn't see changing this as a priority, so we are stuck with a package that lots of people want to use in fedora which can't get in. Note that the related enblend tool also uses includes a modified vigra, so this will have the same issues if and when I submit that. As one of those users... I'm KEEN on getting this in. Is the issue of the vigra library really a blocker? The 'thou shall not build statically' rule has room for exceptions and its not as strict as the 'do not ship static libraries for use by others' rule. Or to ask it another way... how is this case different from what is done with the applications shipped in the netcdf package, which build statically against netcdf libraries...until the next upstream release is available which will make it possible to build dynamically? There is already precedent in the software repository for statically linking against libraries, as special cases. Has upstream for the vigra library reject the modifications? Or is this strictly a matter of slow development timescales for the vigra library project to incorporate changes. If there is a way forward with upstream, so that we can link dynamically in a future version, I don't see this a blocker. But if the upstream project has rejected the modifications, then the library will need to be forked into a seperate project and shipped as a dynamic library in the hugin package. If we are going to special case this, then we need to have some confidence that the issue will become irrelevant as part of forseeable upstream development. Again I refer to the netcdf package as an example, and the development work being done to on netcdf v4 which should fix the special case static linking that the netcdf v3 package has to do. Have has this been taken to the extras or maintainers list for a larger discussion, concerning this can be considered a special case allowance? -jef (In reply to comment #13) > Or to ask it another way... how is this case different from what is done with > the applications shipped in the netcdf package, which build statically against > netcdf libraries...until the next upstream release is available which will make > it possible to build dynamically? There is already precedent in the software > repository for statically linking against libraries, as special cases. > As another example (though perhaps a bit tangential), viaideinfo builds against the static library from pciutils, since it [pciutils-devel] provides no shared library at this time. This should really be brought up on the maintainers list, methinks. (Would this be a good potential topic fot the next FESCo meeting?) Thanks. Before taking it to the fedora-bar-brawl mailinglist, I'd first feel better knowing what the state of the modifications are in terms of upstream adoption. Since the underlying reason we have the static linking here is some sort of library modification... i want to know which resolution path we will be travelling. Are these in the pipe to be included in future upstream library versions.. or are they rejected modifications which will require a library fork, and thus more downstream work to resolve long term. We have a stronger argument for inclusion if there is a clear picture of how this will be worked out. -jef It seems vigra did a release (1.5 from 1.4) in late December. Has this incorporated the Hugin fixes? There is credit to the Hugin team in the changelog. http://kogs-www.informatik.uni-hamburg.de/~koethe/vigra/doc/vigra/CreditsChangelog.html The modifications in the hugin version of vigra have been submitted upstream and some of them are already in vigra-1.5.0 (which went into the fedora build system last night coincidentally). This is definitely not a permanent fork, just some stuff that isn't yet clean enough to go further upstream. Note that the vigra 'library' is mostly c++ headers. The linkable part 'vigraimpex' is actually just a small bit of it, so in reality you are statically including it at build time whichever way you look at it. (In reply to comment #17) > The modifications in the hugin version of vigra have been submitted upstream and > some of them are already in vigra-1.5.0 (which went into the fedora build system > last night coincidentally). This is definitely not a permanent fork, just some > stuff that isn't yet clean enough to go further upstream. Sounds to me like this is an acceptable path forward in terms of long term development interests nullifying the underlying issue here. Clearly the necessary upstream development communication and interaction is happening as evidenced by the latest vigra release. I think we its perfectly appropriate to let this package in with an internal library statically linking. I'll even volunteer to co-maintain the package and watch the progress of the upstream vigra library development, and help transition the package to using the standard vigra library when its appropriate. Now we need to hear from the currently assigned reviewer. Paul, is this an acceptable special case situation? Or would you like to open this up to a larger discussion on the mailinglists? -jef One more thought, Now I haven't dug into the build process yet to look for myself... but for the static linking into the executable...are there specific compiler options which need to be used to ensure position independant-ness? Does -fPIE or -fPIC or something similiar need to be used? Does this matter at all? I bring it up, because it came up in a list discussion concerning the netcdf static libs. Not exactly the same situation, since you aren't shipping the static libs, but it made me wonder. -jef Just another thought from reading this review request: Would it be possible to get the changes that are needed for this package added to the fedora vigra package? ie, patch in our version until upstream finishes merging them in and then we can drop the patch. Then, this package wouldn't need to use a local vigra copy... Or possibly get the upstream vigra folks to merge in their VCS system the patches, and we can move our vigra version to a snapshot? Would it be worth mailing some vigra/hugin maintainers to comment here on which way forward they would prefer? Hi all, I'm the primary author and maintainer of hugin. Frankly I don't see the problem with the inclusion of the vigra source in hugin. Except for a very small pieces of code related to calling libtiff/jpeg/png, all code in vigra is in the headers and compiled statically into all programs anyway. I would favor just including hugin as it is. Once upstream vigra supports all features required by the current hugin, I will switch hugin to using the upstream vigra release. Also all other static libraries included in hugin might change anytime and are not suitable for linking by any other applications. They are just there to help keeping the code organized. Therefore hugin does not install any header or lib*.a/so files in the system diretories. ciao Pablo Okay, Just to let everyone know, since the original reviewer assigned to this ticket has not responsed to me within a week of my post, I will be taking over this review. I hope to have a formal review done in the next couple of hours, hopefully I won't find any more blockers. I don't see the inclusion of the vigra source in hugin as a blocker issue. We have heard from the primary hugin author and there is a clear path forward concerning merger of code back into vigra upstream. Please bare with me, while I respin a formal review as quickly as I am able. I am assuming that http://bugbear.blackfish.org.uk/~bruno/apt/fedora/linux/5/x86_64/SRPMS.panorama/hugin-0.6.1-4.fc5.src.rpm is the latest version up for review. -jef Okay small item in the build log. checking for wxWindows version >= 2.4.2 (--unicode=no)... Warning: No config found to match: /usr/bin/wx-config --static --libs in /usr/lib/wx/config If you require this configuration, please install the desired library build. If this is part of an automated configuration test and no other errors occur, you may safely ignore it. You may use wx-config --list to see all configs available in the default prefix. I assume this is a bogus warning since the hugin application does appear to function. But I thought I'd point it out for comments from the submitter and possible the upstream author. Okay here's my formal review. There are two blockers that need to be address. A directory ownership issue and the desktop file install process. Get these fixed and I can approve. -jef Full review: + Good - BAD ? Questionable N/A Not Applicable Items that need to be addressed - A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. Problems: /usr/share/mime/packages/hugin.xml /usr/share/mime/packages/ is owned by shared-mime-info, hugin should require shared-mime-info /usr/share/icons/gnome/48x48/mimetypes/gnome-mime-application-x-ptoptimizer-script.png /usr/share/icons/gnome/48x48/mimetypes/ is owned by openoffice.org-core It's very difficult to say that hugin should require openoffice.org.core. This directory ownership appears to be in error and thus I think its okay to make an exception and have hugin NOT require openoffice.org.core just for the directory ownership chain. - includes a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. This is described in detail in the desktop files section of Packaging Guidelines. http://fedoraproject.org/wiki/Packaging/Guidelines#desktop Items which pass review. + rpmlint hugin-0.6.1-4.fc7.i386.rpm clean + The package named according to the Package Naming Guidelines. + The spec file name matches the base package %{name}, in the format %{name}.spec + The package is licensed with an open-source compatible license and meet other legal requirements as defined in the legal section of Packaging Guidelines. + The License field in the package spec file must match the actual license. Note: there are multiple codebases in the package and the License tag has the most reasonable license for the collection. + If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc. Note: Contains COPYING for the hugin codebase Contains LICENCE_JHEAD for the public domain jhead codebase Contains LICENCE_VIGRA for the MIT licensed vigra codebase + The spec file is written in American English-ese. + The spec file for the package is legible. + The sources used to build the package match the upstream source, md5sum 46bc3136d42acbabab837128ff471507 hugin-0.6.1.tar.bz2 + The package builds on x86 + BuildRequires look good, assuming the warning concerning wx-config is bogus.. as i believe it is. + The spec file MUST handle locales properly. + no shared libs + not designed to be relocatable + No duplicate files in the %files listing. + Permissions on files look okay. + %clean section, which contains rm -rf %{buildroot} + package consistently uses macros. + The package contains code, or permissable content. + no large docs + %doc files do not affect the runtime of the application. + no Header files + no static libraries in payload. + no pkgconfig(.pc) + no library files + no devel package + no .la libtool archives + Package does not own files or directories already owned by other packages. In reply to comment #22) > I am assuming that > http://bugbear.blackfish.org.uk/~bruno/apt/fedora/linux/5/x86_64/SRPMS.panorama/hugin-0.6.1-4.fc5.src.rpm > is the latest version up for review. Yes, though at some point soon I would like to add a dependency on enblend, though this hasn't been submitted yet as it is waiting on 229419. (In reply to comment #23) > checking for wxWindows version >= 2.4.2 (--unicode=no)... > Warning: No config found to match: /usr/bin/wx-config --static --libs > in /usr/lib/wx/config This warning has appeared since about the time of the switch from wx-2.4 to wx-2.6, it doesn't appear to cause any problems so I always ignored it. (In reply to comment #24) > /usr/share/mime/packages/ is owned by shared-mime-info, hugin should require > shared-mime-info Ok, done > - includes a %{name}.desktop file, and that file must be properly installed > with desktop-file-install in the %install section. Ok I'm a little confused, hugin already installs /usr/share/applications/hugin.desktop Should I delete this and reinstall it with something like this in %install?: rm %{buildroot}/%{_datadir}/applications/%{name}.desktop desktop-file-install --vendor="fedora" \ --dir=${buildroot}/%{_datadir}/applications \ src/hugin/hugin.desktop (In reply to comment #26) > > - includes a %{name}.desktop file, and that file must be properly installed > > with desktop-file-install in the %install section. > > Ok I'm a little confused, hugin already installs > /usr/share/applications/hugin.desktop > > Should I delete this and reinstall it with something like this in %install?: > > rm %{buildroot}/%{_datadir}/applications/%{name}.desktop > desktop-file-install --vendor="fedora" \ > --dir=${buildroot}/%{_datadir}/applications \ > src/hugin/hugin.desktop You don't have too. Call desktop-file-install in %install, passing it the in-buildroot .desktop file as the source, and use the "--delete-original" option. Something like the following should do it (adapt as needed): %install # ... desktop-file-install --vendor fedora \ --dir %{buildroot}%{_datadir}/applications \ --delete-original \ %{buildroot}/%{_datadir}/applications/%{name}.desktop Hope that helps. Ok latest updated specfile and SRPM: http://bugbear.blackfish.org.uk/~bruno/apt/fedora/linux/6/x86_64/SRPMS.panorama/hugin-0.6.1-5.fc6.src.rpm http://bugbear.blackfish.org.uk/~bruno/apt/SPECS/hugin.spec looks good APPROVED ping me when you submit enblend. Note the submission/review process has changed since this was filed. So you'll want to review the process when submitting enblend. New Package CVS Request ======================= Package Name: hugin Short Description: Frontend for Panorama Tools, similar to PTAssembler, PTGui or Open for Windows Owners: bruno Branches: FC-5 FC-6 InitialCC: jspaleta done |