Bug 212704

Created attachment 139670 [details]
simplify a bit spec file and add patches

Created attachment 139671 [details]
add MANBASE for the man pages base directory and simplify

Created attachment 139672 [details]
use MANBASE, and use X11BASE not only on FreeBSD

Some simplification in patch, renamed without freeBSD.

The LOCALBASE, in my opinion should remain along /usr/local,
it is for the locally installed man pages.

Thank you for patches.

Well, some files are installed in wrong directory in -1 srpm and
I fixed it.

http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/manedit.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/manedit-0.7.1-2.src.rpm

Nice application. If patrice is not interested to sponsor this package then i
can do full review and approve this package.

You can do full review, I'll comment now and then.

* a desktop file for manview is missing

* in manedit/images, there are is a serie of pixmaps for the
viewer (mp_viewer*) and for manedit (manedit*) for the hicolor
theme, to be used instead of the images in pixmaps, with the 
approriate snippets used too.

* manwrap don't seem to be needed, it is only used on freebsd,
you should just remove it. If you keep it, it would require in
my opinion a patch to handle also bzip2 files, and also gzcat
should be replaced with zcat.

* there is a dependency on man, used to transform to text. 
groff don't seem to be needed, however.

* I have tested and there seems to be some issues with utf8
in the viewer (not in the editor). Look at  
/usr/share/man/man1p/grep.1p.gz
the ' is garbled in programmer's manual

Created attachment 139821 [details]
also use LOCALBASE outside of freebsd

I updated the manbase patch since I discovered while testing that
LOCALBASE was still not used, so not FHS compliant path was used.

OK.

Mtasaka,
   Can you upload new package by making changes as suggested by Patrice Dumas so
that i can review this package?

Okay I will check a new patch after I take a supper.

I think there is bug in displaying character ’ in manview for any man page

is that gtk library problem>

Indeed. Also I tried to use accented letters in the editor and it
converts the utf8 characters literally into latin1 characters.
Same if I try to view/edit man pages in the fr locale.

This is something that should be fixed, since almost all the localized
man pages use utf8 nowadays in fedora.

It is certainly linked with the fact that gtk1 don't use UTF8 in
the default case, there is a need to set the locale or the like.
I think this should be discussed with upstream.

Forgot to say that the locale issue is not a blocker in my
opinion (but may be for other reviewers), but contacting 
upstream should be a must in that case, and it wouldn't be 
bad to add a sentence in the description, otherwise some user 
may garble their localized man pages.

I would appreciate it if someone would gtk-2-ize this package,
which will be a lot of help.

I already tried to look for gtk2ize, not deeply as makefiles are not friendly
from top DIR but failed to understand anymore.

Porting to gtk2 is a task for upstream, if I'm not wrong there
are API incompatibilities. Otherwise the changes in Makefile
should be fairly minor, maybe changing gtk-config to 
pkg-config gtk+-2.0 should be enough. I have just tested that it 
breaks the build, with issues with GtkText.

(In reply to comment #17)
> I already tried to look for gtk2ize, not deeply as makefiles are not friendly
> from top DIR but failed to understand anymore.

(In reply to comment #18)
> Porting to gtk2 is a task for upstream, if I'm not wrong there
> are API incompatibilities. <snip>
> I have just tested that it 
> breaks the build, with issues with GtkText.

In fact I tried to gtk2ize before submitting this bug as you did, Patrice,
however it failed. I noticed that it needs not a few changes to deal with
fonts, which includes some (or many?) API change, and I gave up........

Anyway I will re-check the patch proposed and resubmit.

SPEC and SRPM are updated.

http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/manedit.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/manedit-0.7.1-3.src.rpm

Currently I don't know how to deal with apostrophe issue.

For apostrophe issue (I don't know if this is a bug of manedit or gtk+)
and gtk2ization, I mailed to perhaps-upstream.

In my opinion, the apostrophe issue is also an utf8 issue, the 
apostrophe used isn't the ascii apostrophe but an utf8 encoded
apostrophe (which is believed to be better looking). 

Well, perhaps the current way is
* wait for upstream's response (I think upstream is learfox[at]twu.net 
  according to http://wolfpack.twu.net/contacts.html .....)
* or add some comments like "this is gtk+ package and some (especially UTF-8)
  characters will appear as garbages")
, perhaps.

I think upstream must be contacted about that issue, but I 
don't think we should wait for this issue to be solved before
accepting manedit in fedora extras. the comment in %description
seems enough to me.

Okay, I added about some comments in %%description stage.

http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/manedit.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/manedit-0.7.1-4.src.rpm

As I commented about, I mailed upstream about gtk2ization.

Well, what should I do for this package?

The spec file seems right to me, however I have spotted some compilers
warnings that are worrisome, and for some of them certainly easy to
fix:

There are many warnings about signedness:
pointer targets in passing argument 2 of '__builtin___strncpy_chk' differ in
signedness
I think this could be investigated. I don't know exactly what is the
potential impact of such warnings, but I guess in some cases there
is room for trouble.

Some include files are certainly not used as they should, I think this 
should be fixed, as it is easy to fix and it may uncover other issues:
editor.c:199: warning: implicit declaration of function 'memcpy'
editor.c:199: warning: incompatible implicit declaration of built-in function
'memcpy'

There are also these warnings:
editorop.c:1146: warning: ignoring return value of 'mkstemp', declared with
attribute warn_unused_result
maneditop.c:566: warning: ignoring return value of 'mkstemp', declared with
attribute warn_unused_result
viewerfio.c:177: warning: ignoring return value of 'mkstemp', declared with
attribute warn_unused_result
viewerfio.c:181: warning: ignoring return value of 'mkstemp', declared with
attribute warn_unused_result

And lastly (but maybe this should be better sent upstream)
prefop.c: In function 'PrefDoApply':
prefop.c:107: warning: 'gdk_color.pixel' is used uninitialized in this function



There are other warnings, but they don't seem to be problematic 
to me.

Okay, I will check the warnings later.

Well, would you check the following?

http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/manedit.spec
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/manedit-0.7.1-5.src.rpm

(In reply to comment #29)
> Well, would you check the following?
> 
> http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/manedit.spec
>
http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/manedit-0.7.1-5.src.rpm

Everything is allright now, nice!

Official Review:
+ package builds in mock (development i386) for FC6.
+ rpmlint is silent for RPM and SRPM.
+ source files match upstream.
229586101245eb61018ff031c2efb247  manedit-0.7.1.tar.bz2
+ package meets naming and packaging guidelines.
+ specfile is properly named, is cleanly written but NOT properly indented.
+ Spec file is written in American English.
+ Spec file is legible.
+ dist tag is present.
+ build root is correct.
+ license is open source-compatible.  License text included in package.
+ %doc is small; no -doc subpackage required.
+ %doc does not affect runtime.
+ COPYING included in %doc.
+ BuildRequires are proper.
+ %clean is present.
+ package installed properly.
+ Macro use appears rather consistent.
+ Package contains code, not content.
+ no headers or static libraries.
+ no .pc files.
+ no -devel subpackage exists
+ no .la files.
+ no translations are available.
+ owns the directories it creates.
+ doesn't own any directories it shouldn't.
+ no duplicates in %files.
+ file permissions are appropriate.
+ Desktop file installed successfully
+ Desktop file is handled correctly in SPEC file.
+ GUI application

Patrice,
    Need your suggestion whether this package can be approved with rendering
problem in manview? I think we can accept this package and let the rendering
problem be solved in next release.

No problem with accepting, the utf8 support issue is clearly
an upstream issue.

ok. APPROVED.
Don't Forget to CLOSE this bug once you finish building package.

Okay.

* Rebuild for FE-devel succeeded.
* SyncNeeded is requested for FE-6 (FE-5 branch already exists)
* Add manedit to owners.list, comps files
* Declare that manedit is unorphaned

Thank you for reviewing and approving this package. Now I
close this bug as CLOSED NEXTRELEASE.

NOTE: I have not yet received any response from upstream.