ps. I noticed that the source is rather large; most of it is test TNEF files that I assume the test process will use and compare results against. Is it worth either getting rid of the tests, or activating them (if so any pointers on how that is done).

I believe ConvertUTF.c is potentially non-free, as it only permits use "in the creation of products supporting the Unicode Standard" which sounds like a use restriction.  Blocking FE-Legal for an opinion.

Do note that ytnef is already in the distro.  Perhaps it doesn't do what you need, but if this turns out to be non-free then it may have to do.

Complete text of the ConvertUTF.c license:

/*
 * Copyright 2001-2004 Unicode, Inc.
 *
 * Disclaimer
 *
 * This source code is provided as is by Unicode, Inc. No claims are
 * made as to fitness for any particular purpose. No warranties of any
 * kind are expressed or implied. The recipient agrees to determine
 * applicability of information provided. If this file has been
 * purchased on magnetic or optical media from Unicode, Inc., the
 * sole remedy for any claim will be exchange of defective media
 * within 90 days of receipt.
 *
 * Limitations on Rights to Redistribute This Code
 *
 * Unicode, Inc. hereby grants the right to freely use the information
 * supplied in this file in the creation of products supporting the
 * Unicode Standard, and to make copies of this file in any form
 * for internal or external distribution as long as this notice
 * remains attached.
 */

/* ---------------------------------------------------------------------

    Conversions between UTF32, UTF-16, and UTF-8. Source code file.
    Author: Mark E. Davis, 1994.
    Rev History: Rick McGowan, fixes & updates May 2001.
    Sept 2001: fixed const & error conditions per
        mods suggested by S. Parent & A. Lillich.
    June 2002: Tim Dodd added detection and handling of incomplete
        source sequences, enhanced error detection, added casts
        to eliminate compiler warnings.
    July 2003: slight mods to back out aggressive FFFE detection.
    Jan 2004: updated switches in from-UTF8 conversions.
    Oct 2004: updated to use UNI_MAX_LEGAL_UTF32 in UTF-32 conversions.

    See the header file "ConvertUTF.h" for complete documentation.

------------------------------------------------------------------------ */

This is: https://fedoraproject.org/wiki/Licensing/UCD

Free and GPL compatible.

This is a bit tricky to interpret, but note that they grant the right to "freely use the information supplied in this file in the creation of products supporting the Unicode Standard", but do not restrict use in other ways. Use is a special case (as compared to other rights, such as copying/distribution/modification), where the "owner" of the software automatically has the right to use the software on their computer under Section 117 of the US Copyright Act, unless otherwise restricted by the copyright holder.

Lifting FE-Legal.

I could review this, but unfortunately I can't test the nautilus integration as I do not use gnome.  I'm not sure if it's expected, but it does not seem to work for KDE; the  desktop file causes an application/vnd.ms-tnef type to appear, but it has no associated application.

(In reply to comment #4)
> I'm not sure if it's expected, but it does not seem to
> work for KDE; the  desktop file causes an application/vnd.ms-tnef type to
> appear, but it has no associated application.  
OK, with various people's help and lots'o'messing around, the package now also includes a kde4 servicemenu .desktop file (which is different, and lives in a different location to gnome).

Also, added running of the make test, which seems to succeed according to the build log, but not really sure if the spec command is right/suitable.

http://members.iinet.net.au/~timmsy/tnef/tnef.spec
http://members.iinet.net.au/~timmsy/tnef/tnef-1.4.6-2.fc11.src.rpm

Hopefully the servicemenu desktop file works on typical kde user's systems.

Not sure about how to own the .desktop file; I don't want to pull in kde (kde-filesystem) or gnome by installing a command line program, so what is the best approach ?

Hi,

This is my first pre-review so I might be wrong. And please note that this is an
informal review.
Since I want to become a packager, I'm pre-reviewing this package. I also think
this package is useful.

# MUST: rpmlint must be run on every package. The output should be posted in the
review.
=> FAIL(1 warning)
  $ rpmlint -i SPECS/tnef-1.4.6-2.fc11.src.rpm
  tnef.src: W: strange-permission tnef.sh 0444
  A file that you listed to include in your package has strange permissions.
  Usually, a file should have 0644 permissions.

  1 packages and 0 specfiles checked; 0 errors, 1 warnings.

  $ rpmlint RPMS/i586/tnef-1.4.6-2.fc11.i586.rpm
  1 packages and 0 specfiles checked; 0 errors, 0 warnings.

  $ rpmlint RPMS/i586/tnef-debuginfo-1.4.6-2.fc11.i586.rpm
  1 packages and 0 specfiles checked; 0 errors, 0 warnings.

tnef.sh should have 0644 permissions.

# MUST: The package must be named according to the  Package Naming Guidelines .
=> OK

# MUST: The spec file name must match the base package %{name}, in the format
%{name}.spec unless your package has an exemption.
=> OK

# MUST: The package must meet the  Packaging Guidelines .
=> seems OK

# MUST: The package must be licensed with a Fedora approved license and meet
the  Licensing Guidelines .
=> seems OK (GPLv2+ and UCD)

# MUST: The License field in the package spec file must match the actual
license.
=> OK (GPLv2+)

# MUST: 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.
=> OK (COPYING)

# MUST: The spec file must be written in American English.
=> OK

# MUST: The spec file for the package MUST be legible.
=> OK

# MUST: The sources used to build the package must match the upstream source, as
provided in the spec URL. Reviewers should use md5sum for this task. If no
upstream URL can be specified for this package, please see the Source URL
Guidelines for how to deal with this.
=> OK

  b47184c4de61322750071cca7026b1cb  SOURCES/tnef-1.4.6.tar.gz (from SRPM)
  b47184c4de61322750071cca7026b1cb  tnef-1.4.6.tar.gz (from upstream)

# MUST: The package MUST successfully compile and build into binary rpms on at
least one primary architecture.
=> OK (i386)

# MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in
bugzilla, describing the reason that the package does not compile/build/work on
that architecture. The bug number MUST be placed in a comment, next to the
corresponding ExcludeArch line.
=> FAIL(build error on ppc and ppc64)

  koji build result(ppc):
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1716349
  koji build result(ppc64) (a test failed):
  http://koji.fedoraproject.org/koji/taskinfo?taskID=1716351

# MUST: All build dependencies must be listed in BuildRequires, except for any
that are listed in the exceptions section of the Packaging Guidelines ;
inclusion of those as BuildRequires is optional. Apply common sense.
=> FAIL

'kde-filesystem' seems to be required to build.
%{_kde4_datadir} macro is defined in /etc/rpm/macros.kde4 which is included in
kde-filesystem package.

# MUST: The spec file MUST handle locales properly. This is done by using the
%find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.
=> N/A

# MUST: Every binary RPM package (or subpackage) which stores shared library
files (not just symlinks) in any of the dynamic linker's default paths, must
call ldconfig in %post and %postun.
=> N/A

# MUST: Packages must NOT bundle copies of system libraries.
=> OK

# MUST: If the package is designed to be relocatable, the packager must state
this fact in the request for review, along with the rationalization for
relocation of that specific package. Without this, use of Prefix: /usr is
considered a blocker.
=> N/A

# MUST: 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.
=> OK

# MUST: A Fedora package must not list a file more than once in the spec file's
%files listings.
=> OK

# MUST: Permissions on files must be set properly. Executables should be set
with executable permissions, for example. Every %files section must include a
%defattr(...) line.
=> OK

# MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
=> OK

# MUST: Each package must consistently use macros.
=> OK

# MUST: The package must contain code, or permissable content.
=> OK

# MUST: Large documentation files must go in a -doc subpackage. (The definition
of large is left up to the packager's best judgement, but is not restricted to
size. Large can refer to either size or quantity).
=> OK

# MUST: If a package includes something as %doc, it must not affect the runtime
of the application. To summarize: If it is in %doc, the program must run
properly if it is not present.
=> OK

# MUST: Header files must be in a -devel package.
=> N/A

# MUST: Static libraries must be in a -static package.
=> N/A

# MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'
(for directory ownership and usability).
=> N/A

# MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1),
then library files that end in .so (without suffix) must go in a -devel package.
=> N/A

# MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency: Requires:
%{name} = %{version}-%{release}
=> N/A

# MUST: Packages must NOT contain any .la libtool archives, these must be
removed in the spec if they are built.
=> OK

# MUST: Packages containing GUI applications must include a %{name}.desktop
file, and that file must be properly installed with desktop-file-install in the
%install section. If you feel that your packaged GUI application does not need
a .desktop file, you must put a comment in the spec file with your explanation.
=> FAIL

http://fedoraproject.org/wiki/Packaging/Guidelines#desktop
  desktop-file-install usage
  It is not simply enough to just include the .desktop file in the package, one
  MUST run desktop-file-install OR desktop-file-validate in %install (and have
  BuildRequires: desktop-file-utils), to help ensure .desktop file safety and
  spec-compliance.

# MUST: Packages must not own files or directories already owned by other
packages. The rule of thumb here is that the first package to be installed
should own the files or directories that other packages may rely upon. This
means, for example, that no package in Fedora should ever share ownership with
any of the files or directories owned by the filesystem or man package. If you
feel that you have a good reason to own a file or directory that another package
owns, then please present that at package review time.
=> I can't judge.

Because I don't know who owns '%{_datadir}/mimelnk/application/' directory.

# MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot}
(or $RPM_BUILD_ROOT).
=> OK

# MUST: All filenames in rpm packages must be valid UTF-8.
=> OK

# SHOULD: If the source package does not include license text(s) as a separate
file from upstream, the packager SHOULD query upstream to include it.
=> N/A (source package includes license text)

# SHOULD: The description and summary sections in the package spec file should
contain translations for supported Non-English languages, if available.
=> N/A

# SHOULD: The reviewer should test that the package builds in mock.
=> FAIL

RPM build errors:
    File must begin with "/": %{_kde4_datadir}/kde4/services/tnefextract.desktop

# SHOULD: The package should compile and build into binary rpms on all supported
architectures.
=> FAIL(build error on ppc and ppc64)

For more information, please see above.

# SHOULD: The reviewer should test that the package functions as described. A
package should not segfault instead of running, for example.
=> OK(tested with some files in tests/files/datafiles)

# SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague,
and left up to the reviewers judgement to determine sanity.
=> N/A

# SHOULD: Usually, subpackages other than devel should require the base package
using a fully versioned dependency.
=> N/A

# SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and
this is usually for development purposes, so should be placed in a -devel pkg.
A reasonable exception is that the main pkg itself is a devel tool not installed
in a user runtime, e.g. gcc or gdb.
=> N/A
 
# SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin consider requiring the package which provides the file
instead of the file itself.
=> N/A

Naoki

(In reply to comment #6)
> This is my first pre-review so I might be wrong. And please note that this is
> an informal review.
> Since I want to become a packager, I'm pre-reviewing this package. I also
> think this package is useful.
Hi Naoki, thanks for your detailed and helpful pre-review.

> # MUST: rpmlint must be run on every package. The output should be posted in
> the review.
> => FAIL(1 warning)
>   $ rpmlint -i SPECS/tnef-1.4.6-2.fc11.src.rpm
>   tnef.src: W: strange-permission tnef.sh 0444
>   A file that you listed to include in your package has strange permissions.
>   Usually, a file should have 0644 permissions.
I had tried to fix this, and found that it made no difference. I realize now that I was running rpmlint on the -1 version of my package, doh.
So fixed that.

> # MUST: The package must be licensed with a Fedora approved license and meet
> the  Licensing Guidelines .
> => seems OK (GPLv2+ and UCD)

Actually, without raising it, perhaps the mixed X + Y license would better describe it, I'm not really sure.

> # MUST: If the package does not successfully compile, build or work on an
> architecture, then those architectures should be listed in the spec in
> ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in
> bugzilla, describing the reason that the package does not compile/build/work on
> that architecture. The bug number MUST be placed in a comment, next to the
> corresponding ExcludeArch line.
> => FAIL(build error on ppc and ppc64)
I had not tried build on non intel arches, thanks for triggering a mock build.

>   koji build result(ppc):
>   http://koji.fedoraproject.org/koji/taskinfo?taskID=1716349
=====
mv -f .deps/mapi_types.Tpo .deps/mapi_types.Po
mv -f .deps/tnef_names.Tpo .deps/tnef_names.Po
mapi_names.c:1307: fatal error: error writing to -: Broken pipe
compilation terminated.
The bug is not reproducible, so it is likely a hardware or OS problem.
gcc: Internal error: Interrupt (program as)
Please submit a full bug report.
See <http://bugzilla.redhat.com/bugzilla> for instructions.
=====
No idea what is happening here.

>   koji build result(ppc64) (a test failed):
>   http://koji.fedoraproject.org/koji/taskinfo?taskID=1716351
=====
PASS: directory.test
PASS: maxsize.test
\'diff ./body.output ./body.baseline > ./body.diff\' -- Test Failed!
FAIL: body.test
PASS: mime-types.test
cat: write error: Broken pipe
PASS: stdin.test
=====
again broken pipe.

I tried to queue some new scratch builds, but I don't remember the proper commands to set it up. (certificate error).

> # MUST: All build dependencies must be listed in BuildRequires, except for any
> that are listed in the exceptions section of the Packaging Guidelines ;
> inclusion of those as BuildRequires is optional. Apply common sense.
> => FAIL
> 
> 'kde-filesystem' seems to be required to build.
> %{_kde4_datadir} macro is defined in /etc/rpm/macros.kde4 which is included in
> kde-filesystem package.
And, since the installed .desktop file wouldn't be owned without out this package, I am including it now as both Requires and BR.

> # MUST: Packages containing GUI applications must include a %{name}.desktop
Hmmn, that is an unfortunate side effect. The app is not a gui app, and it doesn't make sense for an icon to appear in the normal Application (or other) menu, because the app needs an input tnef file to extract to be useful.

It does make sense to have the right click (open with) menu in nautilus/doplhin though. It seems you get one when you want the other, unless there is another way to achieve this file mime-type association ?

I have asked on fedora-packaging list about this item. Perhaps I'm doing something that isn't allowed for other reasons ?

> # MUST: Packages must not own files or directories already owned by other
> packages. The rule of thumb here is that the first package to be installed
> should own the files or directories that other packages may rely upon. This
> means, for example, that no package in Fedora should ever share ownership with
> any of the files or directories owned by the filesystem or man package. If you
> feel that you have a good reason to own a file or directory that another
> package
> owns, then please present that at package review time.
> => I can't judge.
> 
> Because I don't know who owns '%{_datadir}/mimelnk/application/' directory.
rpm -qf /usr/share/mimelnk/application/
kde-filesystem-4-25.fc11.noarch
, but I think other non kde items (eg gnome desktop uses that folder) live there as well.

> # SHOULD: The reviewer should test that the package builds in mock.
> => FAIL
> 
> RPM build errors:
>     File must begin with "/":
> %{_kde4_datadir}/kde4/services/tnefextract.desktop
As you mention above, that is probably because the macro is not defined, so adding the build requires fixes that.

> # SHOULD: The package should compile and build into binary rpms on all
> supported
> architectures.
> => FAIL(build error on ppc and ppc64)


> # SHOULD: The reviewer should test that the package functions as described. A
> package should not segfault instead of running, for example.
> => OK(tested with some files in tests/files/datafiles)
Did you try the menu integration from either nautilus or dolphin ?
I would appreciate your feedback on testing from both if you are able ;-)

Updated package:
http://members.iinet.net.au/~timmsy/tnef/tnef.spec
http://members.iinet.net.au/~timmsy/tnef/tnef-1.4.6-3.fc11.src.rpm

Hi,

(In reply to comment #7)
> > # MUST: rpmlint must be run on every package. The output should be posted in
> > the review.
> > => FAIL(1 warning)
> <snip>
> I had tried to fix this, and found that it made no difference. I realize now
> that I was running rpmlint on the -1 version of my package, doh.
> So fixed that.
=> Now OK

> > # MUST: If the package does not successfully compile, build or work on an
> > architecture, then those architectures should be listed in the spec in
> > ExcludeArch. <snip>
> > => FAIL(build error on ppc and ppc64)
> I had not tried build on non intel arches, thanks for triggering a mock build.

You are welcome.

> >   koji build result(ppc):
> >   http://koji.fedoraproject.org/koji/taskinfo?taskID=1716349
> =====
> mv -f .deps/mapi_types.Tpo .deps/mapi_types.Po
> mv -f .deps/tnef_names.Tpo .deps/tnef_names.Po
> mapi_names.c:1307: fatal error: error writing to -: Broken pipe
> compilation terminated.
> The bug is not reproducible, so it is likely a hardware or OS problem.
> gcc: Internal error: Interrupt (program as)
> Please submit a full bug report.
> See <http://bugzilla.redhat.com/bugzilla> for instructions.
> =====
> No idea what is happening here.

I also have no idea. This might be a temporary problem.

> >   koji build result(ppc64) (a test failed):
> >   http://koji.fedoraproject.org/koji/taskinfo?taskID=1716351
> =====
> PASS: directory.test
> PASS: maxsize.test
> \'diff ./body.output ./body.baseline > ./body.diff\' -- Test Failed!
> FAIL: body.test
> PASS: mime-types.test
> cat: write error: Broken pipe
> PASS: stdin.test
> =====
> again broken pipe.

Yes, but seems that 'body.test' fails.
I've checked the file in 'tests/cmdline' directory, but I don't
find any cause of the error.

> I tried to queue some new scratch builds, but I don't remember the proper
> commands to set it up. (certificate error).

I guess this page will help you:
http://fedoraproject.org/wiki/PackageMaintainers/UsingKoji#Fedora_Certificates
You may need to update cert file.

> > # MUST: All build dependencies must be listed in BuildRequires, except for any
> > that are listed in the exceptions section of the Packaging Guidelines ;
> > inclusion of those as BuildRequires is optional. Apply common sense.
> > => FAIL
> <snip>
> And, since the installed .desktop file wouldn't be owned without out this
> package, I am including it now as both Requires and BR.
=> OK

> > # MUST: Packages containing GUI applications must include a %{name}.desktop
> Hmmn, that is an unfortunate side effect. The app is not a gui app, and it
> doesn't make sense for an icon to appear in the normal Application (or other)
> menu, because the app needs an input tnef file to extract to be useful.
> 
> It does make sense to have the right click (open with) menu in nautilus/doplhin
> though. It seems you get one when you want the other, unless there is another
> way to achieve this file mime-type association ?
> 
> I have asked on fedora-packaging list about this item. Perhaps I'm doing
> something that isn't allowed for other reasons ?
=> pending

I don't have enough knowledge about nautilus/dolphin integration.
I'm watching fedora-packaging list.
I hope we can find better solution of this issue.

> > # MUST: Packages must not own files or directories already owned by other
> > packages. <snip>
> > => I can't judge.
> > 
> > Because I don't know who owns '%{_datadir}/mimelnk/application/' directory.
> rpm -qf /usr/share/mimelnk/application/
> kde-filesystem-4-25.fc11.noarch
> , but I think other non kde items (eg gnome desktop uses that folder) live
> there as well.
=> OK

Thanks for the information.

> > # SHOULD: The reviewer should test that the package builds in mock.
> > => FAIL
> > 
> > <snip>
> As you mention above, that is probably because the macro is not defined, so
> adding the build requires fixes that.
=> Now OK

> > # SHOULD: The reviewer should test that the package functions as described. A
> > package should not segfault instead of running, for example.
> > => OK(tested with some files in tests/files/datafiles)
> Did you try the menu integration from either nautilus or dolphin ?
> I would appreciate your feedback on testing from both if you are able ;-)

I had not tested the menu integration.
I've installed the new package and tried to right-click on 'winmail.dat'
but I couldn't see any difference in the menu.
Do I need more action after the installation?
I'm using nautilus.

Naoki

Created attachment 363329 [details]
screen shot of popup menu for tnef mime-types

(In reply to comment #8)
> You may need to update cert file.
Yeah, I'm in action again, and triggered a build with the package as in my last post. Unfortunately, it still fails on both ppc arches.

Since the test log requests emailing the upstream, I have done so, providing a link to the koji logs.

> > Did you try the menu integration from either nautilus or dolphin ?
> > I would appreciate your feedback on testing from both if you are able ;-)
> 
> I had not tested the menu integration.
> I've installed the new package and tried to right-click on 'winmail.dat'
> but I couldn't see any difference in the menu.
I attach a screen shot of what I get and intend a user installing the package to have available once installed.

Sounds like it didn't work for you. Perhaps it only works on F11, or maybe possibly need to retest on another machine now that I have changed the package a little to validate the .desktop file that causes the mime-type menu...I'll have some more time by the weekend or so.

Created attachment 363333 [details]
screen shot of popup menu on my Fedora 11 (amatubu)

Comment on attachment 363333 [details]
screen shot of popup menu on my Fedora 11 (amatubu)

(In reply to comment #9)
> Yeah, I'm in action again, and triggered a build with the package as in my last
> post. Unfortunately, it still fails on both ppc arches.
> 
> Since the test log requests emailing the upstream, I have done so, providing a
> link to the koji logs.

I hope the upstream can handle the issue.

> I attach a screen shot of what I get and intend a user installing the package
> to have available once installed.

Thanks.
I've also uploaded a screen shot on my Fedora 11.

> Sounds like it didn't work for you. Perhaps it only works on F11, or maybe
> possibly need to retest on another machine now that I have changed the package
> a little to validate the .desktop file that causes the mime-type menu...I'll
> have some more time by the weekend or so.  

OK.
I'll test the new package when it is available.

Naoki

(In reply to comment #11)
> I'll test the new package when it is available.
OK, I worked out what was wrong (I had missed one of the files needed to provide the menu entry), and retested on a separate kde machine, and all seems well now.
Also, tidied up the .desktop files so that they are legit according to desktop-file-install.

Updated package:
http://members.iinet.net.au/~timmsy/tnef/tnef.spec
http://members.iinet.net.au/~timmsy/tnef/tnef-1.4.6-4.fc11.src.rpm

scratch: https://koji.fedoraproject.org/koji/taskinfo?taskID=1726892

Sorry for late reply.

(In reply to comment #12)
> (In reply to comment #11)
> > I'll test the new package when it is available.
> OK, I worked out what was wrong (I had missed one of the files needed to
> provide the menu entry), and retested on a separate kde machine, and all seems
> well now.

Looks good.

> Also, tidied up the .desktop files so that they are legit according to
> desktop-file-install.

The menu integration is now working for me.

I think almost OK

> # MUST: Packages containing GUI applications must include a %{name}.desktop
=> FAIL

The guideline says 'Packages containing GUI applications must include a %{name}.desktop' but this package does not contain '%{name}.desktop'.
I have no idea whether this is a blocker since this package is not 'GUI application'.

> # MUST: If the package does not successfully compile, build or work on an
> architecture, then those architectures should be listed in the spec in
> ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in
> bugzilla, describing the reason that the package does not compile/build/work on
> that architecture. The bug number MUST be placed in a comment, next to the
> corresponding ExcludeArch line.
> => FAIL(build error on ppc and ppc64)
=> now OK

The new SRPM successfully built on ppc and ppc64.

Naoki

(In reply to comment #4)
> I could review this, but unfortunately I can't test the nautilus integration as
> I do not use gnome.  I'm not sure if it's expected, but it does not seem to
> work for KDE; the  desktop file causes an application/vnd.ms-tnef type to
> appear, but it has no associated application.  

Hi Jason, Naoki has provided a helpful pre-review. I added a dolphin servicemenu which seems to do the trick. Could you spare a few cycles to review if there is anything we have missed, and comment on the "not a gui app, but has a desktop file" ?

(In reply to comment #14)
> Sorry for late reply.
> > # MUST: Packages containing GUI applications must include a %{name}.desktop
> => FAIL
> 
> The guideline says 'Packages containing GUI applications must include a
> %{name}.desktop' but this package does not contain '%{name}.desktop'.
> I have no idea whether this is a blocker since this package is not 'GUI
> application'.
I'll ask on packaging list whether that applies to command line tools that are being given a mime-type/ open with entry rather than a GUI menu entry.

> > # MUST: If the package does not successfully compile, build or work on an
> > architecture, then those architectures should be listed in the spec in
> > ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in
> > bugzilla, describing the reason that the package does not compile/build/work on
> > that architecture. The bug number MUST be placed in a comment, next to the
> > corresponding ExcludeArch line.
> > => FAIL(build error on ppc and ppc64)
> => now OK
> 
> The new SRPM successfully built on ppc and ppc64.
I have reverted the "dont test on ppc ppc64" changes. With the current 1.4.7 version the tests succeed on x86_64, and the koji scratch build succeeds. I note however, that it only built x86 arches. 

Building with f-12 as target succeeds on the ppcX arches:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2064055

Updated package:
http://members.iinet.net.au/~timmsy/tnef/tnef.spec
http://members.iinet.net.au/~timmsy/tnef/tnef-1.4.7-1.fc12.src.rpm
also there is a diff: tnef.spec.1.4.6-5.to.tnef.1.4.7-1
, with the intent of making it easier to pick out changes.

Naoki, do you have a few cycles to finalise this review ?

The following is a link to the packaging thread regarding the desktop file
http://thread.gmane.org/gmane.linux.redhat.fedora.extras.packaging/6744

Ville can't see that as a problem (neither can I).

(In reply to comment #17)
> The following is a link to the packaging thread regarding the desktop file
> http://thread.gmane.org/gmane.linux.redhat.fedora.extras.packaging/6744
Summarizing the response:
- it's OK for desktop integration as long as it does not show up in the desktop menu (where it would be off no use). So I a think we are good to go on this issue.

Anything else I need to look at ?

Hi Naoki, would you have some time to turn this into a formal review ?

I'm sorry that I don't have enough time to check your new package now.
I'm going to look into it this weekend.

I've downloaded the new SRPM and SPEC files and found that the SPEC file and tnef.spec included in the SRPM file are different.
Here's diff:

--- SPECS/tnef.spec    2010-03-20 08:27:32.000000000 +0900
+++ ../SPECS/tnef.spec    2010-03-20 09:51:26.000000000 +0900
@@ -14,18 +14,12 @@
 Source2:   tnef-extract.desktop
 Source3:   tnefextract.desktop
 Source4:   tnef.sh
-#Patch0:    debug.diff
-#Patch1:    tnef.spec
-#Patch2:    tnef-util_utf8.diff
 BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 
 BuildRequires: desktop-file-utils
 BuildRequires: kde-filesystem
 Requires: kde-filesystem 
 
-# tnef tests fail on ppcX during build on koji. Awaiting upstream response.
-#E#xcludeArch: ppc ppc64
-
 
 %description
 TNEF provides a way to unpack those pesky Microsoft MS-TNEF MIME attachments.
@@ -37,9 +31,6 @@
 
 %prep
 %setup -q
-#%#patch0 -p1
-#%#patch1 -p1
-#%#patch2 -p1
 
 
 %build
@@ -85,10 +76,7 @@
 
 
 %check
-# large file test is known to fail on ppcX arch, so test on x86
-#%ifarch %{ix86}
 make check DESTDIR=$RPM_BUILD_ROOT
-#%endif
 
 
 %files

Naoki

(In reply to comment #20)
> I'm sorry that I don't have enough time to check your new package now.
> I'm going to look into it this weekend.
Thanks, Naoki, for putting it on your plans ;-)

> I've downloaded the new SRPM and SPEC files and found that the SPEC file and
> tnef.spec included in the SRPM file are different.
Whoops, I didn't rebuild after removing the commented out bits.

I decided to update to a further release and took the opportunity to adjust the summary and description to capitalize acronyms that rpmlint is picking up as spelling errors:

http://members.iinet.net.au/~timmsy/tnef/tnef.spec
http://members.iinet.net.au/~timmsy/tnef/tnef-1.4.7-2.fc12.src.rpm

Hi David,

I will review this as requested,

First to begin with can you:

- Update to latest upstream
- Change to using uniform macro's don't mix $RPM_BUILD_ROOT with %{buildroot}
- Split the desktop integration into a sub package, some users just need the command line tnef for use with tools like mailscanner they do not need to be saddled with a whole desktop environment, since it does both gnome and kde you could have tnef-gnone and tnef-kde, or just tnef-desktop

Will do the full review when this is done.

*** Bug 722198 has been marked as a duplicate of this bug. ***

(In reply to comment #22)
> I will review this as requested,
OK, thanks, do you want to co-maintain after the package is approved ?
What is your FAS username ?

> - Update to latest upstream
> - Change to using uniform macro's don't mix $RPM_BUILD_ROOT with %{buildroot}
> - Split the desktop integration into a sub package, some users just need the
> command line tnef for use with tools like mailscanner they do not need to be
> saddled with a whole desktop environment
Agreed, done. Not sure whether to require just the filesystem items, or the actual file managers (which I have done in this version).

Updated:
http://members.iinet.net.au/~timmsy/tnef/tnef.spec
http://members.iinet.net.au/~timmsy/tnef/tnef-1.4.8-1.fc15.src.rpm

Its fine, you can maintain it. Please create branches for el5 and el5.

- The spec file link still points to the old spec
- Remove the kde-filesystem requires for the main package
- %post and %postun should be for the sub packages not the main package

OK, 
- remove dolphin requires on kde-filesystem
- move update-desktop-database to gui subpackages
- upload the real tnef.spec to web space (although, I had to shift reload to get the new copy)

http://members.iinet.net.au/~timmsy/tnef/tnef.spec
http://members.iinet.net.au/~timmsy/tnef/tnef-1.4.8-2.fc15.src.rpm

David,

- The main package, still requires kde-filesystem, it shouldn't. 
- The sub packages should Require(post) Require(postun) desktop-file-utils due to the update-desktop-database command being executed by the scriptlets

Please fix these two issues and we should be done.

(In reply to comment #27)
> - The main package, still requires kde-filesystem, it shouldn't. 

OK. (missed that I copied not moved.)

> - The sub packages should Require(post) Require(postun) desktop-file-utils due
> to the update-desktop-database command being executed by the scriptlets

The guidelines seem to suggest that Requires: desktop-file-utils shouldn't be used anymore (since FC6), so I removed that altogether.
See [http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#desktop-database]
which references: icon cache below it.

EL5 I think was based on FC6, so I think that would satisfy that.

Okay, post the updated spec and srpm and i will finalize the review.

Oops, fired the comment before I finished upload of spec, src.rpm:
http://members.iinet.net.au/~timmsy/tnef/tnef.spec
http://members.iinet.net.au/~timmsy/tnef/tnef-1.4.8-3.fc15.src.rpm

[x]  MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.

rpmlint rpmbuild/SRPMS/tnef-1.4.8-3.fc15.src.rpm rpmbuild/RPMS/x86_64/tnef-1.4.8-3.fc15.x86_64.rpm 
tnef.src: I: enchant-dictionary-not-found en_US
tnef.x86_64: E: incorrect-fsf-address /usr/share/doc/tnef-1.4.8/COPYING
tnef.x86_64: W: no-manual-page-for-binary tnef.sh
2 packages and 0 specfiles checked; 1 errors, 1 warnings.

[x]  MUST: The package must be named according to the Package Naming Guidelines .
[x]  MUST: The spec file name must match the base package %{name}
[x]  MUST: The package must meet the Packaging Guidelines .
[x]  MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines.
[x]  MUST: The License field in the package spec file must match the actual license.
[x]  MUST: License included in %doc
[x]  MUST: The spec file must be written in American English.
[x]  MUST: The spec file for the package MUST be legible.
[x]  MUST: The sources used to build the package must match the upstream source.

sha1sum tnef-1.4.8.tar.gz rpmbuild/SOURCES/tnef-1.4.8.tar.gz 
19431176ee523fe3fd5e745882a9083426cc5671  tnef-1.4.8.tar.gz
19431176ee523fe3fd5e745882a9083426cc5671  rpmbuild/SOURCES/tnef-1.4.8.tar.gz

[x]  MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture.
[x]  MUST: If the package does not successfully compile, build or work on an architecture, it should be listed in ExcludeArch.
[x]  MUST: All build dependencies must be listed in BuildRequires
[N/A]  MUST: The spec file MUST handle locales properly.
[N/A]  MUST: Every binary RPM package (or subpackage) which stores shared library files must call ldconfig
[x]  MUST: Packages must NOT bundle copies of system libraries.
[N/A]  MUST: If the package is designed to be relocatable, must state this fact.
[x]  MUST: A package must own all directories that it creates.
[x]  MUST: A Fedora package must not list a file more than once.
[x]  MUST: Permissions on files must be set properly.
[x]  MUST: Each package must consistently use macros.
[x]  MUST: The package must contain code, or permissable content.
[x]  MUST: Large documentation files must go in a -doc subpackage.
[x]  MUST: If a package includes something as %doc, it must not affect the runtime of the application
[N/A]  MUST: Header files must be in a -devel package.
[N/A]  MUST: Static libraries must be in a -static package.
[N/A]  MUST: Library files that end in .so must go in a -devel package.
[N/A]  MUST: Devel packages must require the base package using a fully versioned dependency.
[x]  MUST: Packages must NOT contain any .la libtool archives.
[x]  MUST: GUI applications must include a %{name}.desktop file, installed with desktop-file-install
[x]  MUST: Packages must not own files or directories already owned by other packages. 
[x]  MUST: All filenames in rpm packages must be valid UTF-8.

-------------------------------------------------------------------
   This package (tnef) is APPROVED by topdog
-------------------------------------------------------------------

OK, thanks Andrew. Shall I put you in initial cc or secondary owner ?

New Package SCM Request
=======================
Package Name: tnef
Short Description: Extract files from email attachments like winmail.dat
Owners: dtimms
Branches: f14 f15 el5 el6
InitialCC: baz

Git done (by process-git-requests).

Removed baz from InitialCC.

tnef-1.4.8-4.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/tnef-1.4.8-4.fc15

tnef-1.4.8-4.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/tnef-1.4.8-4.el6

tnef-1.4.8-4.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/tnef-1.4.8-4.fc14

Package tnef-1.4.8-4.el6:
* should fix your issue,
* was pushed to the Fedora EPEL 6 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=epel-testing tnef-1.4.8-4.el6'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/tnef-1.4.8-4.el6
then log in and leave karma (feedback).

tnef-1.4.8-4.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

tnef-1.4.8-4.fc15 has been pushed to the Fedora 15 stable repository.  If problems still persist, please make note of it in this bug report.

tnef-1.4.8-4.2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/tnef-1.4.8-4.2.el6

tnef-1.4.8-4.2.el6 has been pushed to the Fedora EPEL 6 stable repository.

Package Change Request
======================
Package Name: tnef
New Branches:  epel7
Owners: dtimms

Git done (by process-git-requests).