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