Bug 212704 - Review Request: manedit - UNIX Manual Page Editor
Review Request: manedit - UNIX Manual Page Editor
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-10-28 03:11 EDT by Mamoru TASAKA
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-11-13 04:27:57 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
simplify a bit spec file and add patches (1023 bytes, patch)
2006-10-29 12:55 EST, Patrice Dumas
no flags Details | Diff
add MANBASE for the man pages base directory and simplify (2.13 KB, patch)
2006-10-29 12:57 EST, Patrice Dumas
no flags Details | Diff
use MANBASE, and use X11BASE not only on FreeBSD (656 bytes, patch)
2006-10-29 12:59 EST, Patrice Dumas
no flags Details | Diff
also use LOCALBASE outside of freebsd (746 bytes, patch)
2006-10-31 04:27 EST, Patrice Dumas
no flags Details | Diff

  None (edit)
Description Mamoru TASAKA 2006-10-28 03:11:08 EDT
Spec URL: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/manedit.spec
SRPM URL: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/manedit-0.7.1-1.src.rpm
Description: 
ManEdit is a UNIX manual page editor and viewer, 
it is designed specifically for the editing of the 
UNIX manual page format using an integrated XML interface.

-------------------------------------------------------------------------
Note: manedit got orphaned on 2005-04-14 according to
http://fedoraproject.org/wiki/Extras/OrphanedPackages.
Comment 1 Patrice Dumas 2006-10-29 12:55:38 EST
Created attachment 139670 [details]
simplify a bit spec file and add patches
Comment 2 Patrice Dumas 2006-10-29 12:57:56 EST
Created attachment 139671 [details]
add MANBASE for the man pages base directory and simplify
Comment 3 Patrice Dumas 2006-10-29 12:59:17 EST
Created attachment 139672 [details]
use MANBASE, and use X11BASE not only on FreeBSD
Comment 4 Patrice Dumas 2006-10-29 13:05:20 EST
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.
Comment 5 Mamoru TASAKA 2006-10-30 03:06:23 EST
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
Comment 6 Parag AN(पराग) 2006-10-31 02:31:42 EST
Nice application. If patrice is not interested to sponsor this package then i
can do full review and approve this package.
Comment 7 Patrice Dumas 2006-10-31 04:21:09 EST
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

Comment 8 Patrice Dumas 2006-10-31 04:27:52 EST
Created attachment 139821 [details]
also use LOCALBASE outside of freebsd
Comment 9 Patrice Dumas 2006-10-31 04:29:19 EST
I updated the manbase patch since I discovered while testing that
LOCALBASE was still not used, so not FHS compliant path was used.
Comment 10 Parag AN(पराग) 2006-10-31 04:38:01 EST
OK.

Mtasaka,
   Can you upload new package by making changes as suggested by Patrice Dumas so
that i can review this package?
Comment 11 Mamoru TASAKA 2006-10-31 04:45:53 EST
Okay I will check a new patch after I take a supper.
Comment 12 Parag AN(पराग) 2006-10-31 04:48:21 EST
I think there is bug in displaying character ’ in manview for any man page
Comment 13 Parag AN(पराग) 2006-10-31 05:21:21 EST
is that gtk library problem>
Comment 14 Patrice Dumas 2006-10-31 05:31:30 EST
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.
Comment 15 Patrice Dumas 2006-10-31 05:39:20 EST
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.
Comment 16 Mamoru TASAKA 2006-10-31 05:47:17 EST
I would appreciate it if someone would gtk-2-ize this package,
which will be a lot of help.
Comment 17 Parag AN(पराग) 2006-10-31 05:55:05 EST
I already tried to look for gtk2ize, not deeply as makefiles are not friendly
from top DIR but failed to understand anymore.
Comment 18 Patrice Dumas 2006-10-31 06:08:37 EST
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.
Comment 19 Mamoru TASAKA 2006-10-31 06:25:10 EST
(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.

Comment 20 Mamoru TASAKA 2006-10-31 08:46:21 EST
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.
Comment 21 Mamoru TASAKA 2006-11-02 06:18:25 EST
For apostrophe issue (I don't know if this is a bug of manedit or gtk+)
and gtk2ization, I mailed to perhaps-upstream.
Comment 22 Patrice Dumas 2006-11-02 06:21:54 EST
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). 
Comment 23 Mamoru TASAKA 2006-11-02 06:33:26 EST
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.
Comment 24 Patrice Dumas 2006-11-02 06:56:52 EST
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.
Comment 25 Mamoru TASAKA 2006-11-02 09:26:42 EST
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.
Comment 26 Mamoru TASAKA 2006-11-10 05:28:10 EST
Well, what should I do for this package?
Comment 27 Patrice Dumas 2006-11-10 05:45:16 EST
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.
Comment 28 Mamoru TASAKA 2006-11-10 10:17:28 EST
Okay, I will check the warnings later.
Comment 30 Patrice Dumas 2006-11-11 16:21:08 EST
(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!
Comment 31 Parag AN(पराग) 2006-11-11 23:43:35 EST
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.
Comment 32 Patrice Dumas 2006-11-12 13:52:55 EST
No problem with accepting, the utf8 support issue is clearly
an upstream issue.
Comment 33 Parag AN(पराग) 2006-11-13 02:57:39 EST
ok. APPROVED.
Don't Forget to CLOSE this bug once you finish building package.
Comment 34 Mamoru TASAKA 2006-11-13 04:27:57 EST
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.

Note You need to log in before you can comment on or make changes to this bug.