Bug 448215 - Review Request: mozvoikko - Finnish Voikko spell-checker extension for Mozilla programs
Review Request: mozvoikko - Finnish Voikko spell-checker extension for Mozill...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ville Skyttä
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-24 10:47 EDT by Ville-Pekka Vainio
Modified: 2008-07-25 16:02 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-07-25 16:02:30 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
ville.skytta: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ville-Pekka Vainio 2008-05-24 10:47:45 EDT
Spec URL: http://vpv.fedorapeople.org/packages/mozvoikko/firefox-voikko.spec
SRPM URL: http://vpv.fedorapeople.org/packages/mozvoikko/firefox-voikko-0.9.4.1-0.1.fc9.src.rpm
Description: 
This is mozvoikko, a Firefox extension for using the Finnish spell-checker
Voikko.

Rpmlint gives the following errors:
firefox-voikko.i386: E: zero-length /usr/lib/mozilla/extensions/{ec8030f7-c20a-464f-9b0e-13a3a9e97384}/{b676e3ff-cda7-4e0c-b2b8-74e4bb40a67a}/chrome.manifest
firefox-voikko.i386: E: explicit-lib-dependency libvoikko

There are reasons for these, though. The empty chrome.manifest is needed or else Firefox will say the chrome registration failed. The explicit dependency on libvoikko is needed, because the extension calls libvoikko in such a way that rpmbuild can't detect it. If I drop the dependency, the extension wouldn't do anything for users who don't have libvoikko installed already.
Comment 1 Ville-Pekka Vainio 2008-05-24 11:41:26 EDT
Upstream just released a new version after I posted this bug. This makes
packaging a bit simpler for Linux distributions.

Updated spec: http://vpv.fedorapeople.org/packages/mozvoikko/firefox-voikko.spec
Updated SRPM:
http://vpv.fedorapeople.org/packages/mozvoikko/firefox-voikko-0.9.4.1.1-0.1.fc9.src.rpm
Comment 2 Ville Skyttä 2008-05-24 12:37:49 EDT
I don't have F-9 yet so not taking the review at this point, but here's some
notes just from looking at the specfile and srpm:

- Source0 is not a full URL to the tarball.

- The %{_libdir}/mozilla/extensions/%{firefox_app_id} dir is not owned by this
package, is it owned by something else in its dependency chain?

- The Makefile patch hardcodes NSPR_INCLUDES=-I/usr/include/nspr4, I think it
could be changed to something like NSPR_INCLUDES=$(shell pkg-config
--cflags-only-I nspr)

- The Makefile patch hardcodes XULRUNNER_SDK=/usr/lib/xulrunner-sdk-1.9pre - is
that correct on lib64 archs too, and isn't there a pkg-config or something that
could be called like in the NSPR_INCLUDES comment above?  Ditto for
VOIKKO_INCLUDES by the way.
Comment 3 Ville-Pekka Vainio 2008-05-24 16:31:43 EDT
Thanks for the comments.
Updated spec: http://vpv.fedorapeople.org/packages/mozvoikko/firefox-voikko.spec
New SRPM:
http://vpv.fedorapeople.org/packages/mozvoikko/firefox-voikko-0.9.4.1.1-0.2.fc9.src.rpm

(In reply to comment #2)
> - Source0 is not a full URL to the tarball.

Fixed.
 
> - The %{_libdir}/mozilla/extensions/%{firefox_app_id} dir is not owned by this
> package, is it owned by something else in its dependency chain?

Yes, firefox.
 
> - The Makefile patch hardcodes NSPR_INCLUDES=-I/usr/include/nspr4, I think it
> could be changed to something like NSPR_INCLUDES=$(shell pkg-config
> --cflags-only-I nspr)

Changed as recommended.

> - The Makefile patch hardcodes XULRUNNER_SDK=/usr/lib/xulrunner-sdk-1.9pre - is
> that correct on lib64 archs too, and isn't there a pkg-config or something that
> could be called like in the NSPR_INCLUDES comment above?  Ditto for
> VOIKKO_INCLUDES by the way.

That's not correct, because of the current outage I haven't been able to do a
scratch build, so this went unnoticed. The following is used now:
XULRUNNER_SDK=$(shell pkg-config --variable=sdkdir libxul)

Libvoikko is my package, but that doesn't have a pc file, upstream doesn't
provide one either. I can try to add those, but I've never done them before, so
it may take some learning and time (I assume they need to be made during the
build?). However, the location of the header file(s) probably won't be changing
anytime soon, for any arch, so I think just leaving the static value there
should be ok.
Comment 4 Ville-Pekka Vainio 2008-05-24 18:01:27 EDT
As an additional comment, I did a scratch build of the current SRPM in Koji and
it succeeded. The logs are available for a while at
http://koji.fedoraproject.org/koji/taskinfo?taskID=627290
Comment 5 Ville-Pekka Vainio 2008-05-26 04:38:47 EDT
After discussions with upstream I've decided to rename the package to mozvoikko.
This is because the same package can also provide spell checking for Thunderbird
and Seamonkey, when the versions based on xulrunner come to Fedora.

I've also added firefox to Requires for now, as it is currently the only program
which can use mozvoikko and it wasn't really in the dependency chain before,
even though that's what I claimed in comment #3. The path patch hasn't changed,
as there's not yet pkg-config support in libvoikko, I'm working on it.

New spec: http://vpv.fedorapeople.org/packages/mozvoikko/mozvoikko.spec
New SRPM:
http://vpv.fedorapeople.org/packages/mozvoikko/mozvoikko-0.9.4.1.1-0.3.fc9.src.rpm
Comment 6 Ville-Pekka Vainio 2008-05-26 16:46:59 EDT
Upstream released a new version. In this version XULRUNNER_SDK and NSPR_INCLUDES
are queried using pkg-config, VOIKKO_INCLUDES was dropped completely. Upstream
also added some content to chrome.manifest, so that rpmlint error is fixed. I
updated the Fedora path patch so that the xulrunner include dirs are now handled
correctly. I'm not completely sure why the package even built before, but it
did, I guess the same headers are in multiple locations or something...

Anyway,
Updated spec: http://vpv.fedorapeople.org/packages/mozvoikko/mozvoikko.spec
New SRPM:
http://vpv.fedorapeople.org/packages/mozvoikko/mozvoikko-0.9.4.1.2-0.1.fc9.src.rpm
Comment 7 Ville-Pekka Vainio 2008-05-31 15:25:03 EDT
Upstream released a new version and basically used my path patch, so no patching
is needed anymore.

Updated spec: http://vpv.fedorapeople.org/packages/mozvoikko/mozvoikko.spec
New SRPM:
http://vpv.fedorapeople.org/packages/mozvoikko/mozvoikko-0.9.4.1.3-0.1.fc9.src.rpm
Comment 8 Ville-Pekka Vainio 2008-06-24 16:54:36 EDT
Upstream released a new version, this is the first "official" release via
Sourceforge. There were only a couple of small changes, here's the updated package.

Updated SPEC: http://vpv.fedorapeople.org/packages/mozvoikko/mozvoikko.spec
New SRPM:
http://vpv.fedorapeople.org/packages/mozvoikko/mozvoikko-0.9.5-0.1.fc9.src.rpm
Comment 9 Ville Skyttä 2008-07-07 14:53:18 EDT
I finally got around to updating to F-9 and thus reviewing this.

If I understand correctly, using xulrunner-unstable makes this prone to breakage
on updates - is there some versioned dependency towards some package that could
be used so that it would be easier to notice such cases?

The dependency on libvoikko works, but as the sources hardcode libvoikko.so.1
and libmalaga.so.7, it could be good to have the dependency to those sonames. 
I've done that in the pcsc-perl package, check it out for an example if you
agree implementing that would be feasible.

Would Applications/Internet be a better Group: value?  Aren't all Mozilla apps
this package works with Internet related ones?

When other Mozilla apps that can use this are shipped in Fedora, the dependency
on firefox should probably be dropped, right?

None of the above are blockers but just comments and food for thought, approved
as is.
Comment 10 Ville-Pekka Vainio 2008-07-14 17:15:41 EDT
Thanks for the review. I think you have made some important points here and I
probably won't build the package until I have at least some sort of answers for
those questions. Unfortunately I've been a bit busy lately, I will try to make
time to work on this package this week.
Comment 11 Ville-Pekka Vainio 2008-07-18 12:55:03 EDT
Just an update, I'm currently working on the xulrunner-unstable/dependency
issue. There was an update to Firefox today in F-9, which caused a dependency
issue with nspluginwrapper (it had gecko-libs = 1.9 as a dependency). I had the
mozvoikko package installed, which has an unversioned dependency on xulrunner
and it worked normally after the update. My current suggestion is to leave it as
is and if there is actual breakage, I'll just have to rebuild the package.
Apparently you can't count that much on version numbers with xulrunner.

A new testing version with some Makefile changes was released today, I'll have
to test that on Fedora, but it seems like the package could BuildRequire just
xulrunner-devel instead of xulrunner-devel-unstable. But still some of the
headers mozvoikko needs are apparently classified as unstable upstream, I'm not
sure why they are in the "regular" devel package on Fedora. I'll probably post
about this on the fedora-devel mailing list.
Comment 12 Ville-Pekka Vainio 2008-07-18 16:00:03 EDT
Updated spec: http://vpv.fedorapeople.org/packages/mozvoikko/mozvoikko.spec
New SRPM:
http://vpv.fedorapeople.org/packages/mozvoikko/mozvoikko-0.9.5-0.2.fc9.src.rpm

(In reply to comment #9)
> If I understand correctly, using xulrunner-unstable makes this prone to breakage
> on updates - is there some versioned dependency towards some package that could
> be used so that it would be easier to notice such cases?

I've now asked about this on fedora-devel, we'll see if there are any comments.
Voikko upstream developers told me there's quite rarely breakage with the
unstable headers mozvoikko uses. I've added a patch which modifies
Makefile.xulrunner so that xulrunner-devel-unstable is not needed anymore, but I
think that won't actually change the situation, because the same headers are
just included from a different directory now.

(The new testing version which was released today just modified
Makefile.xulrunner in a way which made it simpler for Debian, but it didn't work
on Fedora, so upstream will continue with 0.9.5.)

> The dependency on libvoikko works, but as the sources hardcode libvoikko.so.1
> and libmalaga.so.7, it could be good to have the dependency to those sonames. 
> I've done that in the pcsc-perl package, check it out for an example if you
> agree implementing that would be feasible.

Done, I haven't yet built the package on 64 bit, though, but it should work
there as well.

> Would Applications/Internet be a better Group: value?  Aren't all Mozilla apps
> this package works with Internet related ones?

Changed. To me it doesn't really matter that much, but this is probably better.

> When other Mozilla apps that can use this are shipped in Fedora, the dependency
> on firefox should probably be dropped, right?

Yes. My current plan of doing this is to put the files into
%{_libdir}/mozilla/extensions/%{firefox_app_id}/%{firefox_ext_id} as is done now
and then symlink them to %{_libdir}/mozilla/extensions/<{thunderbird, seamonkey}
app id here>/%{firefox_ext_id}. This would mean that the mozvoikko package would
have to co-own those directories together with the firefox, thunderbird and
seamonkey packages, but then any actual dependencies to the applications (except
xulrunner of course) could be dropped.
Comment 13 Ville-Pekka Vainio 2008-07-21 14:12:10 EDT
Since I have not gotten any feedback on my fedora-devel post about this package
and xulrunner, I'll request CVS and build the package. For F-9 I'll probably
keep it in updates-testing until I get some feedback from users that it works
with their Mozilla profiles and so on.

Here's the request:

New Package CVS Request
=======================
Package Name: mozvoikko
Short Description: Finnish Voikko spell-checker extension for Mozilla programs
Owners: vpv
Branches: F-9
Cvsextras Commits: yes
Comment 14 Kevin Fenzi 2008-07-22 11:46:38 EDT
I wonder if it would be worth checking with the fedora-packaging list to see if
anyone had thoughts on xulrunner/extension packaging before importing this?

There was some discussion last year on fedora-devel: 
https://www.redhat.com/archives/fedora-devel-list/2007-April/msg00855.html

Comment 15 Ville Skyttä 2008-07-23 13:19:13 EDT
If you ask me, no need to hold the import for that; I think this is nothing new
and there are already packages in the distro in a similar situation with
xulrunner.  Not sure about extension packaging but anyway, the plan in comment
13 sounds perfectly reasonable to me.
Comment 16 Kevin Fenzi 2008-07-24 13:52:06 EDT
I suppose not, just would be good to do moving forward... 

cvs done.
Comment 17 Ville-Pekka Vainio 2008-07-25 16:02:30 EDT
The package has been built and should (soon) be in Rawhide and F9
updates-testing. Thanks!

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