Bug 229826

Could you please translate the content of the spec file in English? For now I get:
%description
基于 Gtk2+ 的 CHM 文件阅读工具

使用æ示
* 与有些 chm 阅读工具ä¸åŒï¼ŒChmSee 采用的是
先将 chm 文件解压，å†è¯»å– html 文件的方å¼ã€‚
解压åŽçš„文件ä¿å­˜åœ¨ $HOME/.chmsee/bookshelf 


Thank you

You can keep the description in another language, but you should
tag it with the appropriate language code.

gnochm (that I maintain) already owns 
/usr/share/icons/gnome/48x48/mimetypes/application-x-chm.png
/usr/share/icons/gnome/48x48/mimetypes/gnome-mime-application-x-chm.png

What do you propose? I guess that the best would be to have this added 
to gnome themes. In the mean time I don't see any good solution. 
It seems that we will need a separate package that holds only those files.
This package could be either a subpackage of gnochm or of chmsee.

Why don't make chmsee conflicts with gnochm for now, so yum could prompt before
download? Adding chm mime-type to gnome themes is too slow to happen. Besides
that, from my point of view chmsee is better than gnochm so it worth a push. Thanks.

Spec URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee.spec
SRPM URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee-1.0.0-0.5.beta.src.rpm

> gnochm (that I maintain) already owns 
> /usr/share/icons/gnome/48x48/mimetypes/application-x-chm.png
> /usr/share/icons/gnome/48x48/mimetypes/gnome-mime-application-x-chm.png
> 
> What do you propose? I guess that the best would be to have this added 
> to gnome themes. In the mean time I don't see any good solution. 
> It seems that we will need a separate package that holds only those files.
> This package could be either a subpackage of gnochm or of chmsee.

Well, I don't know any of the gnome-icon-theme maintainers, could you please
contact them? 

If we share/depend on a common package, you see, I am not a extras contributor
yet, I'll just follow your decision,make chmsee depends on your subpackage. 

BTW, Sorry for my rude in the last comment, but please give chmsee a try. I like
their work! :)

Regards.

(In reply to comment #3)
> Why don't make chmsee conflicts with gnochm for now, so yum could prompt before
> download?

That's wrong. There should be no conflict in fedora except if there
is a good reason. Having an icon in common is not a good reason.

> Adding chm mime-type to gnome themes is too slow to happen.

Indeed, we must find a short term solution.

> Besides
> that, from my point of view chmsee is better than gnochm so it worth a push.
Thanks.

Whether chmsee is better than gnochm or not is not relevant.
Both should be in fedora and installable in parallel.
The fact that they both handle the same mimetype is also not
an issue at all. (xchm and kchmviewer also handle the chm
mimetype).

chmsee don't seem to show the index?

I'd like to see chmsee in fedora too.

After some thinking, I think I found a solution: use the same
file for
/usr/share/icons/gnome/48x48/mimetypes/gnome-mime-application-x-chm.png
for both packages. That way they may coexist nicely. Does that
looks good to you?

(In reply to comment #4)

> Well, I don't know any of the gnome-icon-theme maintainers, could you please
> contact them? 

I don't know them either... (I don't use gnome, but fluxbox or icewm)...

Issues:

* the openssl-devel buildrequires is a bit weird. Where does it
  comes from?

* In general it is not usefull to have the package name in 
  the summary, all the tools should already show it

* desktop-file-utils shared-mime-info should not be in Requires(pre)
  Requires(post) as explained on the past scripts snippets page

* However desktop-file-utils should be a BuildRequires.

* --vendor should be fedora and not gnome.

* --add-category Utility seems to be unusefull to me since it is already
  in the desktop file. 


Suggestions:

* I suggest removing Application category in the patch and not on the
  command line

* I suggest calling the patch along 
chmsee-1.0.0-desktop-mimetype.patch

* I suggest putting the chmsee-icon.png in 
/usr/share/icons/hicolor/48x48/apps
call it chmsee.png and adapt the desktop file accordingly.

* snippet for icon theme update should be used (for gnome 
theme instead of hicolor, or both if you follow my suggestion
above), see:

http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda

* %description is a bit terse, especially compared with the zh_CN
one.

(In reply to comment #7)
> Issues:
> 
> * the openssl-devel buildrequires is a bit weird. Where does it
>   comes from?
> 

chmsee works by extracting all files.  Openssl is used to calculate md5 from
filename. The md5sum will be used as a directory name, to hold files extracted
from different  CHM files.

> * In general it is not usefull to have the package name in 
>   the summary, all the tools should already show it
> 

removed for default "Summary".

rpmlint complains about "Summary not capitalized" for Summary(zh_CN) if there is
no Capitalized "ChmSee".  

> * desktop-file-utils shared-mime-info should not be in Requires(pre)
>   Requires(post) as explained on the past scripts snippets page
> 

removed. I thought they should be there but I haven't read those guidelines for
a long time.

> * However desktop-file-utils should be a BuildRequires.
> 

thanks.

> * --vendor should be fedora and not gnome.
> 

changed.

> * --add-category Utility seems to be unusefull to me since it is already
>   in the desktop file. 
> 

removed.

> 
> Suggestions:
> 
> * I suggest removing Application category in the patch and not on the
>   command line
> 
> * I suggest calling the patch along 
> chmsee-1.0.0-desktop-mimetype.patch
> 

renamed.

> * I suggest putting the chmsee-icon.png in 
> /usr/share/icons/hicolor/48x48/apps
> call it chmsee.png and adapt the desktop file accordingly.
> 

done.

> * snippet for icon theme update should be used (for gnome 
> theme instead of hicolor, or both if you follow my suggestion
> above), see:
> 

done. The app icon was in pixmaps directory so this was not needed. Moving it to
hicolor seems to be a good idea.

> 
> * %description is a bit terse, especially compared with the zh_CN
> one.
> 

check it :)



Spec URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee.spec
SRPM URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee-1.0.0-0.6.beta.src.rpm

And I wonder why this issue blocks nothing. I remembered when I submitted this,
it blocks FE_NEW 163776.   Who should change its block/depend status? 

The status is always "NEW" and the assignment is always "nobody", why? Who
could/will change them?

Could you review my package and sponsor me, or should I find another one who
will review it and sponsor me? or should I look for a third people to sponsor
me? (/me confused somehow)

(In reply to comment #8)

> chmsee works by extracting all files.  Openssl is used to calculate md5 from
> filename. The md5sum will be used as a directory name, to hold files extracted
> from different  CHM files.

Ok.
 

> rpmlint complains about "Summary not capitalized" for Summary(zh_CN) if there is
> no Capitalized "ChmSee".  

rpmlint is wrong in that case - and it is not strange, because
the summary is in a non english (and even non latin) locale.
 
> > * desktop-file-utils shared-mime-info should not be in Requires(pre)
> >   Requires(post) as explained on the past scripts snippets page
> > 
> 
> removed. I thought they should be there but I haven't read those guidelines for
> a long time.

It has changed in FC-5 if I recall well.

(In reply to comment #9)
> And I wonder why this issue blocks nothing. I remembered when I submitted this,
> it blocks FE_NEW 163776.   Who should change its block/depend status? 

I don't exactly know, since we are changing how things work. 
The blockers are not used anymore, instead flags are used. But 
things are not completely stabilized.

> The status is always "NEW" and the assignment is always "nobody", why? Who
> could/will change them?

I could change it, but I don't' like to be assigned formally as
a reviewer, except when there are blocking issues that I want to
be solved. My practice is to comment on reviews, and when it is 
acceptable I do the formal review. That way it also leave the possibility
for another reviewer that don't have the same ideas than me (on things
that aren't hard policies) to accept the package. For example for this
submission, Manuel may want to do some reviewing too, and I want to leave
this open to him (even though he isn't a sponsor...)

> Could you review my package and sponsor me, or should I find another one who
> will review it and sponsor me? or should I look for a third people to sponsor
> me? (/me confused somehow)

I am a sponsor and I can sponsor you. This package needs to be 
approved, and I should also look at your activity around fedora.
I have seen that you begun reviewing a package, I'll follow that, I'll 
look for you in mailing lists, on the net and so on. Other sponsors
may also do the same. You can send me a private mail if you have tracks
of your activity that would help me decide to sponsor you.

There is also a page in the wiki
http://fedoraproject.org/wiki/Extras/HowToGetSponsored

* %description is right (even a bit too long now ;-). It should be 
cut at about 80 columns. 
s/conviniently/conveniently/
s;Try to $HOME/.chmsee ;Try to remove $HOME/.chmsee;

* I think that the chmsee-icon.png file should be kept in pixmaps, even
though it is also in the theme directory.

* in $RPM_BUILD_ROOT/%{_datadir}, / is unuseful.

* you should keep timestamps when installing data files, adding
-p to install call.

* What about my proposal, using the same file for 
/usr/share/icons/gnome/48x48/mimetypes/gnome-mime-application-x-chm.png

It seems to me that the file used should be a file specific of the 
file type, not a file corresponding with an application. Therefore
the file chmfile.png from gnochm seems a better candidate to me than
chmsee-icon.png. 

As a side note, I don't like that much that icon file, I prefer the 
ones that are with xchm, I find them better looking, and there are 
different sizes (and they are installed in the hicolor mimetype 
theme). However for the gnome theme it seems to me that an icon from 
a gnome/gtk package is better.

* Suggestion: 
use your real name in changelog

(In reply to comment #11)
> 
> I could change it, but I don't' like to be assigned formally as
> a reviewer, except when there are blocking issues that I want to
> be solved. My practice is to comment on reviews, and when it is 

I can understand about this. Being a mentor will makes one get upset very
easily. I have another package in review and I didn't response to my sponsor
quickly so I guess I may have offended him/her and he or she has given up giving
more advice & directions. :(

But having to wait for a long time before some one could formally review one's
package will make the contributor upset, very easily, too. I was and am confused
by those guidelines: some one just told me since I am not a sponsor, I should
not assign the bug to myself when doing the review . 

(And the review guideline tells me if I am in the " fedorabugs" group then I
could start to review packages, and the first step is to assign the bug to
myself. Being a sponsored contributor? That means I should be in the "cvsextras"
group, and  it does not matter I think.)

(In reply to comment #12)
> * %description is right (even a bit too long now ;-). It should be 
> cut at about 80 columns. 
> s/conviniently/conveniently/
> s;Try to $HOME/.chmsee ;Try to remove $HOME/.chmsee;
> 

corrected. thx.

> * I think that the chmsee-icon.png file should be kept in pixmaps, even
> though it is also in the theme directory.
> 

ok.

> * in $RPM_BUILD_ROOT/%{_datadir}, / is unuseful.
> 

removed.

> * you should keep timestamps when installing data files, adding
> -p to install call.
> 

done.

> * What about my proposal, using the same file for 
> /usr/share/icons/gnome/48x48/mimetypes/gnome-mime-application-x-chm.png
> 
> It seems to me that the file used should be a file specific of the 
> file type, not a file corresponding with an application. Therefore
> the file chmfile.png from gnochm seems a better candidate to me than
> chmsee-icon.png. 
> 
> As a side note, I don't like that much that icon file, I prefer the 
> ones that are with xchm, I find them better looking, and there are 
> different sizes (and they are installed in the hicolor mimetype 
> theme). However for the gnome theme it seems to me that an icon from 
> a gnome/gtk package is better.
> 

I don't quite understand this. I said I will just follow your decision, and
please just tell me, what I should do? Thanks!


Spec URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee.spec
SRPM URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee-1.0.0-0.7.beta.src.rpm

About #13: I think the bug should always be assigned to some one, so the
contributor will not be confused so much. Nothing more. :p

(In reply to comment #13)

> I can understand about this. Being a mentor will makes one get upset very
> easily. 

Why? Anyway I am not a mentor, but a fellow fedora contributor who
happen to be a sponsor.

> I have another package in review and I didn't response to my sponsor
> quickly so I guess I may have offended him/her and he or she has given up giving
> more advice & directions. :(

There may be many reasons beside personal feelings explaining
why a reviewer stops reviewing, we are all benevolent.

> I was and am confused
> by those guidelines: some one just told me since I am not a sponsor, I should
> not assign the bug to myself when doing the review . 

If you are not sponsored you cannot approve any package. If you are
sponsored but you are not a sponsor you cannot approve packages of
people not sponsored.

> (And the review guideline tells me if I am in the " fedorabugs" group then I
> could start to review packages, and the first step is to assign the bug to
> myself. Being a sponsored contributor? That means I should be in the "cvsextras"
> group, and  it does not matter I think.)

You can always comment on any bug, even if you are not already 
sponsored. But you cannot approve. And it is possible to comment
on reviews without doing the formal review. That is what I do 
all the time. Currently I am not doing a formal review of chmsee,
but commenting. Some comments are must fix, but as I said in another 
comment I don't want to get in the way of another sponsor willing 
to sponsor you, or another contributor wanting to review.

(In reply to comment #15)
> (In reply to comment #13)
> 
> > I was and am confused
> > by those guidelines: some one just told me since I am not a sponsor, I should
> > not assign the bug to myself when doing the review . 
> 
> If you are not sponsored you cannot approve any package. If you are
> sponsored but you are not a sponsor you cannot approve packages of
> people not sponsored.
> 

:D 
much clear now. If I cannot approve any package, I should not assign the bug to
myself.

Regarding the mimetype icon file, I propose that you add
the chmfile.png file from gnochm (it is in 
/usr/share/pixmaps/chmfile.png ). Don't forget to use
something like cp -p to get it to keep the timestamp. 
Then you could add chmfile.png as Source1 and in %install:

mkdir -p $RPM_BUILD_ROOT%{_datadir}/icons/gnome/48x48/mimetypes/
cp -p %{SOURCE1}
$RPM_BUILD_ROOT%{_datadir}/icons/gnome/48x48/mimetypes/application-x-chm.png
ln -s application-x-chm.png
$RPM_BUILD_ROOT%{_datadir}/icons/gnome/48x48/mimetypes/gnome-mime-application-x-chm.png

Since both file will be exactly the same rpm should install them 
without problem.

This solution is not perfect, because we have to duplicate the 
chmfile.png file, but it seems to be the best to me.

Why isn't %{?_smp_mflags} used?

(In reply to comment #17)
> Regarding the mimetype icon file, I propose that you add
> the chmfile.png file from gnochm (it is in 
> /usr/share/pixmaps/chmfile.png ). Don't forget to use
> something like cp -p to get it to keep the timestamp. 
> Then you could add chmfile.png as Source1 and in %install:
> 

done. This icon is better than nothing because it is just a question mark and a
CHM doc.. The question mark looks like "I know nothing" instead of "help docs".
And chm is not only for help docs, this is why chmsee-icon.png looks like a book
(though more like a dictionary)

(In reply to comment #18)
> Why isn't %{?_smp_mflags} used?

linking problems, don't know how to fix :(


Spec URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee.spec
SRPM URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee-1.0.0-0.8.beta.src.rpm

(In reply to comment #19)

> 
> done. This icon is better than nothing because it is just a question mark and a
> CHM doc.. The question mark looks like "I know nothing" instead of "help docs".
> And chm is not only for help docs, this is why chmsee-icon.png looks like a book
> (though more like a dictionary)

A question mark is not necessarily "I know nothing", it may also mean
"I have a question". 

> (In reply to comment #18)
> > Why isn't %{?_smp_mflags} used?
> 
> linking problems, don't know how to fix :(

Please put a comment saying something along
# %%{?_smp_mflags} breaks build. Linking problems?

> 
> Spec URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee.spec
> SRPM URL:
ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee-1.0.0-0.8.beta.src.rpm

Another issue is that now that there is something installed in the
gnome theme, you should add the gtk-update-icon-cache scriptlet for
that theme too (in addition to the one for the hicolor theme).

The source doesn't seems to match upstream:
b18df276ff8050668ff3da163efe147c  chmsee-1.0.0-beta.tar.gz
8ebce73126d94cc646565f38aa94dcc9  ../SOURCES/chmsee-1.0.0-beta.tar.gz

Seems like upstream did a new release without changing the tarball 
name...

The build log if enabled %{?_smp_flags}

/bin/sh ../libtool --tag=CC   --mode=link gcc
-DPACKAGE_DATA_DIR=\""/usr/share"\" -DCHMSEE_DATA_DIR=\""/usr/share/chmsee"\"
-DPACKAGE_LOCALE_DIR=\""/usr/share/locale"\" -DGLADE_FILE=\"chmsee.glade\"
-DBOOKMARK_FILE=\"chmsee_bookmarks\" -DBOOKINFO_FILE=\"chmsee_bookinfo\"
-DCHMSEE_NO_LINK=\"chmsee_no_link\" -pthread -I/usr/include/glib-2.0
-I/usr/lib/glib-2.0/include -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include
-I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0
-I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/libglade-2.0
-I/usr/include/libxml2 -I/usr/include -I/usr/include/firefox-2.0.0.1/.
-I/usr/include/firefox-2.0.0.1/commandhandler
-I/usr/include/firefox-2.0.0.1/content -I/usr/include/firefox-2.0.0.1/dom
-I/usr/include/firefox-2.0.0.1/find -I/usr/include/firefox-2.0.0.1/gfx
-I/usr/include/firefox-2.0.0.1/gtkembedmoz -I/usr/include/firefox-2.0.0.1/locale
-I/usr/include/firefox-2.0.0.1/pref -I/usr/include/firefox-2.0.0.1/webbrwsr
-I/usr/include/firefox-2.0.0.1/string -I/usr/include/firefox-2.0.0.1/xpcom
-I/usr/include/firefox-2.0.0.1/gtkembedmoz -I/usr/include/firefox-2.0.0.1
-I/usr/include/firefox-2.0.0.1/xpcom -I/usr/include/firefox-2.0.0.1/string
-I/usr/include/nspr4    -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
-fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic
-fasynchronous-unwind-tables libcppwrapper.la -pthread -lgthread-2.0 -lrt
-lglade-2.0 -lgtk-x11-2.0 -lxml2 -lgdk-x11-2.0 -latk-1.0 -lgdk_pixbuf-2.0
-lpng12 -lm -lpangocairo-1.0 -lpango-1.0 -lcairo -lgobject-2.0 -lgmodule-2.0
-ldl -lglib-2.0 -L/usr/lib -lchm -lssl -lcrypto -ldl -L/usr/lib/firefox-2.0.0.1
-lgtkembedmoz -lxpcom -lplds4 -lplc4 -lnspr4 -lpthread -ldl   
-R/usr/lib/firefox-2.0.0.1   -o chmsee chmsee-main.o chmsee-marshal_main.o
chmsee-bookmarks.o chmsee-booktree.o chmsee-chmfile.o chmsee-chmsee.o
chmsee-html.o chmsee-link.o chmsee-parser.o chmsee-setup.o chmsee-startup.o
chmsee-utils.o  
libtool: link: cannot find the library `libcppwrapper.la' or unhandled argument
`libcppwrapper.la'
make[3]: *** [chmsee] Error 1
make[3]: *** Waiting for unfinished jobs....
ar cru .libs/libcppwrapper.a .libs/libcppwrapper_la-gecko_utils.o
ranlib .libs/libcppwrapper.a
creating libcppwrapper.la
(cd .libs && rm -f libcppwrapper.la && ln -s ../libcppwrapper.la libcppwrapper.la)
make[3]: Leaving directory `/home/yuan/rpmbuild/BUILD/chmsee-1.0.0-beta/src'
make[2]: *** [all] Error 2
make[2]: Leaving directory `/home/yuan/rpmbuild/BUILD/chmsee-1.0.0-beta/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/yuan/rpmbuild/BUILD/chmsee-1.0.0-beta'
make: *** [all] Error 2
error: Bad exit status from /var/tmp/rpm-tmp.25314 (%build)




icon scriptlet updated.
source tarball updated.

Spec URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee.spec
SRPM URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee-1.0.0-0.9.beta.src.rpm

---------------------------------------
BuildRequires:  gecko-devel
---------------------------------------
Well, this style of handling gecko dependency is a problem
because rpm cannot treat rpath dependency correnctly.

* Details:
  (I use FC-devel so here I write especially on rawhide)
  - First, /usr/bin/chmsee requires some libraries from firefox,
    and /usr/bin/chmsee has rpath because the libraries in firefox
    is not installed under default library path.
----------------------------------------------------
[root@localhost ~]# ldd -r /usr/bin/chmsee | grep firefox
        libgtkembedmoz.so => /usr/lib/firefox-2.0.0.1/libgtkembedmoz.so (0x007a6000)
        libxpcom.so => /usr/lib/firefox-2.0.0.1/libxpcom.so (0x00b6d000)
        libxpcom_core.so => /usr/lib/firefox-2.0.0.1/libxpcom_core.so (0x04ec8000)
[root@localhost ~]# objdump --headers --private-headers /usr/bin/chmsee | grep RPATH
  RPATH       /usr/lib/firefox-2.0.0.1
----------------------------------------------------
    - And when you check the libraries' dependency by
      "rpm -q --requires chmsee", chmsee surely requires libxpcom.so.
      Then "rpm -q --whatprovides libxpcom.so" returns firefox.
      Note that rpm only checks the library name and does not
      check _rpath_ .

     - Well, firefox 2.0.0.2 is already released and sooner or later
       2.0.0.2 will appear on rawhide.
     - Then I update firefox to 2.0.0.2 and the problem happens.
       firefox 2.0.0.2 also provides "libxpcom.so" so no conflict
       occurs between firefox and chmsee according to rpm judgment.
       However chmsee actually requires 
       "/usr/lib/firefox-2.0.0.1/libxpcom.so" so chmsee won't be
       launched.....

So:
* Usually we have to write explicitly (the following is on FC-devel)
-------------------------------------------------
Requires: firefox-devel = 2.0.0.1
Requires: firefox = 2.0.0.1
-------------------------------------------------
  On FC-5, this is 1.5.0.9 and soon will be 1.5.0.10.

(In reply to comment #22)
> So:
> * Usually we have to write explicitly (the following is on FC-devel)
> -------------------------------------------------
> Requires: firefox-devel = 2.0.0.1
> Requires: firefox = 2.0.0.1
> -------------------------------------------------

This should be:
------------------------------------------------
BuildRequires: firefox-devel = 2.0.0.1
Requires: firefox-devel = 2.0.0.1
------------------------------------------------

Again mistaken... very very sorry...
------------------------------------------------
BuildRequires: firefox-devel = 2.0.0.1
Requires: firefox = 2.0.0.1
------------------------------------------------

(In reply to comment #21)
> The build log if enabled %{?_smp_flags}

This is not for the fedora package but for upstream. There is no 
problem for us to disable smp_mflags. However I had a look, and a
possibility is that chmsee_LDFLAGS is used incorrectly to specify
library link, it should only be for linker options, chmsee_LDADD
is for link. So in my opinion, it should be along:

chmsee_LDFLAGS = -R$(GECKO_HOME) \
        $(AM_LDFLAGS)

chmsee_LDADD = \
        libcppwrapper.la \
        @CHMSEE_LIBS@ \
        @CHMLIB_LIBS@ \
        @MD5_LIBS@ \
        $(GECKO_LIBS) \
        $(GECKO_EXTRA_LIBS)

It may also be another issue.

I have spotted 2 other problems, still for upstream:

*  $(addprefix is not portable

* in the GLIB_GENMARSHAL rules, the files shouldn't be created in 
  $(srcdir), $(srcdir) could be read-only.


> icon scriptlet updated.

Still missing in %postun

Also I think the use of the gnochm icon for mimetype deserves a 
comment. It is in changelog, but it seems to me that a comment
near the Source1 or near the install calls in %install should be
there too.

(In reply to comment #24)
> ------------------------------------------------
> BuildRequires: firefox-devel = 2.0.0.1
> Requires: firefox = 2.0.0.1
> ------------------------------------------------

added.

(In reply to comment #25)
> 
> This is not for the fedora package but for upstream. There is no 
> problem for us to disable smp_mflags. However I had a look, and a
> possibility is that chmsee_LDFLAGS is used incorrectly to specify
> library link, it should only be for linker options, chmsee_LDADD
> is for link. So in my opinion, it should be along:
> 
> chmsee_LDFLAGS = -R$(GECKO_HOME) \
>         $(AM_LDFLAGS)
> 
> chmsee_LDADD = \
>         libcppwrapper.la \
>         @CHMSEE_LIBS@ \
>         @CHMLIB_LIBS@ \
>         @MD5_LIBS@ \
>         $(GECKO_LIBS) \
>         $(GECKO_EXTRA_LIBS)
> 

Thanks very much!

> 
> 
> > icon scriptlet updated.
> 
> Still missing in %postun
> 

:D

> Also I think the use of the gnochm icon for mimetype deserves a 
> comment. It is in changelog, but it seems to me that a comment
> near the Source1 or near the install calls in %install should be
> there too.

added.


Spec URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee.spec
SRPM URL: ftp://ftp.fedora.cn/pub/fedora-cn/in-review/chmsee-1.0.0-0.10.beta.src.rpm

* rpmlint (ignorable, but you could want to fix it):
W: chmsee mixed-use-of-spaces-and-tabs (spaces: line 8, tab: line 1)
* free software license, included
* follow guidelines
* scriptlets are right
* sane provides
* match upstream:
b18df276ff8050668ff3da163efe147c  chmsee-1.0.0-beta.tar.gz
* %files section right
* .desktop right

APPROVED

You seem to be quite active in the fedora community, and I'll
sponsor you. I think that you learned a lot in this review and
others, but I am not completely sure that you know the guidelines
and procedures enough already to formally approve packages; in 
little time you will be experienced enough, but maybe for your 
first reviews you could do the whole review but ask for another 
look before formally approving. Feel also free to disregard my 
advice...


- first note:
there is a potential directory owning issue since
/usr/share/icons/gnome/
/usr/share/icons/hicolor/
and some directories below may be unowned. However this is an issue
that should be fixed with a freedesktop filesystem package or the
like, so I let it be. I would suggest owning those directories, though.

- 2nd note:
I'll try to remember to watch out changes in the gnochm icon.

- 3rd note, there is linking against libraries that are not useful:
$ ldd -u -r /usr/bin/chmsee 
Unused direct dependencies:

        /lib/librt.so.1
        /usr/lib/libatk-1.0.so.0
        /usr/lib/libpng12.so.0
        /lib/libm.so.6
        /usr/lib/libpangocairo-1.0.so.0
        /usr/lib/libcairo.so.2
        /lib/libgmodule-2.0.so.0
        /lib/libssl.so.6
        /usr/lib/libplds4.so
        /usr/lib/libplc4.so
        /usr/lib/libnspr4.so
        /lib/libdl.so.2

This is very common, and not a big deal, but this could force 
unneeded rebuilds when the sonames change and cause rpm/yum/... 
to be less efficient. In general the fix is to use Private 
rightly in the .pc files of those libs, and sometimes there
is also things to do in the package to avoid over linking.
This is not a must fix at all you can completely ignore that
issue.

New Package CVS Request
=======================
Package Name: chmsee
Short Description: A Gtk+2 CHM document viewer
Owners: bbbush.yuan
Branches: FC-6
InitialCC: pertusus, jungleji

Package co-maintainer Change Request
=======================
Package Name: chmsee
Short Description: A Gtk+2 CHM document viewer
Owners: bbbush.yuan, pertusus
Branches: FC-6

(In reply to comment #29)
> Package co-maintainer Change Request
> =======================

> Branches: FC-6

Only for FC-6? Not for all the branches? 

(In reply to comment #30)
> 
> > Branches: FC-6
> 
> Only for FC-6? Not for all the branches? 
> 

It is said that "devel" branch is always implied. Isn't it?

(In reply to comment #31)

> It is said that "devel" branch is always implied. Isn't it?

If I'm not wrong not for co-maintainers. A co-maintainer may
only co-maintain a given branch.

In fact unless I am wrong, there a 2 way of becoming co-maintainer,
be added to owners.list, or have an entry in pkg.acl. Since the
fedora-cvs request is to be added to owners.list, you are right
that branches are irrelevant. 

Yuan, seems like the package is built, so you can close the bug.

Now test2 is out.

It now uses pkg-config to configure firefox-gtkmozembed include and libs path.
Since on fedora, firefox-gtkmozembed.pc doesn't contain
-R/usr/lib/firefox-2.0.0.3/   so RPATH is not used when linking chmsee. 

Since /usr/lib/firefox-2.0.0.3/ is not in ldconfig path, the compiled chmsee
binary cannot run without setting LD_LIBRARY_PATH.

run "ldd /usr/bin/chmsee" will output
[yuan@mstar ~]$ ldd /usr/bin/chmsee |grep "not found"
       libgtkembedmoz.so => not found
       libxpcom.so => not found
       libxpcom_core.so => not found

It is suggested to make a wrapper "chmsee" to call the binary (rename it
chmsee.bin or so), then in the wrapper we can search for firefox install
directory to avoid rebuild everytime firefox has an update. I think it may
introduce security issue if anyone make another wrapper to load different
libgtkmozembed.so.

Any opinion? I can see the same thing on FE package lifera, but they use fixed
LD_LIBRARY_PATH (that is, they need to rebuild when firefox has an update).

Thanks.

Seems like the bug is in firefox-gtkmozembed.pc? then it should be 
fixed there. I'd say use LD_LIBRARY_PATH in the interim or wait 
for the issue to be fixed.

The software which uses gecko engine usually
sets rpath on the binary to be installed by itself.
So IMO introducing rpath to chmsee is a reasonable
solution.

And every time firefox is updated, the software
which uses gecho engine always has to be rebuilded
anyway because gecho engine usually becomes incompatible

Liferea not just sets LD_LIBRARY_PATH, it also hardcodes the Firefox
home dir into the binary.