| Summary: | app-text/enchant-1.4.2 (Version bump) | ||
|---|---|---|---|
| Product: | Gentoo Linux | Reporter: | Serkan Kaba (RETIRED) <serkan> |
| Component: | [OLD] GNOME | Assignee: | GNOME Office (OBSOLETE) <gnome-office+disabled> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | flammie, pva, serkan |
| Priority: | High | Keywords: | EBUILD |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
| Bug Depends on: | 156100 | ||
| Bug Blocks: | 145188, 207025, 226213 | ||
| Attachments: |
Patch against the current 1.3.0 ebuild
Patch against the current 1.3.0 ebuild Patch against the current 1.3.0 ebuild enchant-1.4.2.ebuild.patch Patch against the current 1.3.0 ebuild Patch against the current 1.3.0 ebuild |
||
|
Description
Serkan Kaba (RETIRED)
2008-07-06 18:52:33 UTC
Created attachment 159717 [details, diff]
Patch against the current 1.3.0 ebuild
Is there a recommended default checker? The reason I ask is that it is annoying for users when installs/upgrades break because a USE flag needs to be selected. So *if* there is a sane default checker, IMO its USE flag should default to enabled. Serkan, I'll answer you here :) I'd better use hunspell as default engine. AFAIK it is more feature reach but as the same time a bit is slower. But, I found that fedore now uses hunspell as the spellchecker library. The reasons are here: http://fedoraproject.org/wiki/Releases/FeatureDictionary For us this means that it could be good idea to build by default everything with hunspell too. I need to check what should be done here, but this means that in future we'll have hunspell enabled by default in profile, thus it's better to avoid it enabled in ebuild. So what to do now? ispell backend does not use ispell library but uses ispell-like library inside of enchant (take a look at src/ispell/README) so it does not require any dependencies so it's good idea use this as default. Not USE-flag default, just fallback in case user have not selected anything, like this: DEPEND="hunspell? (app-text/hunspell) aspell? (virtual/aspell-dict)" if use hunspell -o use aspell -o use enchant-ispell; then myconf="$(use_enable aspell) \ $(use_enable hunspell) \ $(use_enable enchant-ispell ispell)" else myconf="--enable-ispell" fi I've changed ispell USE flag as even it is enabled enchant does not use ispell library, thus ispell flag is bad thing in this case. Also such scheme with default fallback does not require confutils_require_any and this is a good thing (TM). No need to die() :) Also I think it's good idea to have possibility enable other engines as well: --disable-ispell enable the ispell backend default=auto --disable-myspell enable the myspell backend default=auto --disable-aspell enable the aspell backend default=auto --disable-voikko enable the voikko backend default=auto --disable-uspell enable the uspell backend default=auto --disable-hspell enable the hspell backend default=auto --disable-zemberek enable the zemberek backend default=auto or are there any reasons you avoid them? automagic detection isn't good, if you want them fine, if you want to delay it, please at least force them off (cf the devmanual).
@pva, with regard to your snippet of shell, I'd rather use the confutils function ${something}_any which would be a bit more easily expanded for future use flag enabled backends.
(In reply to comment #3) > Serkan, I'll answer you here :) > > I'd better use hunspell as default engine. AFAIK it is more feature reach but > as the same time a bit is slower. But, I found that fedore now uses hunspell as > the spellchecker library. The reasons are here: > > http://fedoraproject.org/wiki/Releases/FeatureDictionary > > For us this means that it could be good idea to build by default everything > with hunspell too. I need to check what should be done here, but this means > that in future we'll have hunspell enabled by default in profile, thus it's > better to avoid it enabled in ebuild. > > So what to do now? ispell backend does not use ispell library but uses > ispell-like library inside of enchant (take a look at src/ispell/README) so it > does not require any dependencies so it's good idea use this as default. Not > USE-flag default, just fallback in case user have not selected anything, like > this: > > DEPEND="hunspell? (app-text/hunspell) > aspell? (virtual/aspell-dict)" > > if use hunspell -o use aspell -o use enchant-ispell; then > myconf="$(use_enable aspell) \ > $(use_enable hunspell) \ > $(use_enable enchant-ispell ispell)" > else > myconf="--enable-ispell" > fi > > I've changed ispell USE flag as even it is enabled enchant does not use ispell > library, thus ispell flag is bad thing in this case. > > Also such scheme with default fallback does not require confutils_require_any > and this is a good thing (TM). No need to die() :) > > Also I think it's good idea to have possibility enable other engines as well: > > --disable-ispell enable the ispell backend default=auto > --disable-myspell enable the myspell backend default=auto > --disable-aspell enable the aspell backend default=auto > --disable-voikko enable the voikko backend default=auto > --disable-uspell enable the uspell backend default=auto > --disable-hspell enable the hspell backend default=auto > --disable-zemberek enable the zemberek backend default=auto > > or are there any reasons you avoid them? > I added support for Zemberek but the dependency currently resides in java-overlay. Voikko may be added as I said in my initial comment. hspell website is down now (I don't know if this is temp. situation) and only useful thing I found about it after googling is Debian package site located at http://packages.debian.org/hspell . Reaadme of uspell says the lib resides in abiword cvs but I couldn't see any (Link to PLD Linux snapshot which dates back to 20031030 is available at http://rpmseek.com/rpm-dl/uspell-1.1.1-0.20031030.1.src.html?hl=com&cs=uspell:PN:0:0:0:0:1096982) For the default engine ispell seems reasonable in terms of dependencies (It has none). But aspell or hunspell seems to provide dictionaries for more languages. (In reply to comment #4) > @pva, with regard to your snippet of shell, I'd rather use the confutils > function ${something}_any which would be a bit more easily expanded for future > use flag enabled backends. Hm, why die() is better? What do you mean by "more easily expanded for future use flag enabled backends"? It's worse if package just die when all backends are disabled. Notably when the package itself is bundled with ispell-like library (thus no additional deps). If you want you could notify user with elog, that he/she didn't enable any backends and thus we are falling back to echant-ispell but in any case this solution is better then die as in confutils_require_any solution. (In reply to comment #5) > I added support for Zemberek but the dependency currently resides in > java-overlay. Voikko may be added as I said in my initial comment. hspell > website is down now (I don't know if this is temp. situation) and only useful > thing I found about it after googling is Debian package site located at > http://packages.debian.org/hspell . Readme of uspell says the lib resides in > abiword cvs but I couldn't see any (Link to PLD Linux snapshot which dates back > to 20031030 is available at > http://rpmseek.com/rpm-dl/uspell-1.1.1-0.20031030.1.src.html?hl=com&cs=uspell:PN:0:0:0:0:1096982) Thank you. I supposed things like that... May be short info about state could be useful in ebuild. (In reply to comment #6) > Hm, why die() is better? What do you mean by "more easily expanded for future > use flag enabled backends"? > > It's worse if package just die when all backends are disabled. Notably when the > package itself is bundled with ispell-like library (thus no additional deps). > If you want you could notify user with elog, that he/she didn't enable any > backends and thus we are falling back to echant-ispell but in any case this > solution is better then die as in confutils_require_any solution. > oops, my bad, I thought there was a non-dying variant. Maybe a good thing to add to the eclass ? (In reply to comment #8) > (In reply to comment #6) > > Hm, why die() is better? What do you mean by "more easily expanded for future > > use flag enabled backends"? > > > > It's worse if package just die when all backends are disabled. Notably when the > > package itself is bundled with ispell-like library (thus no additional deps). > > If you want you could notify user with elog, that he/she didn't enable any > > backends and thus we are falling back to echant-ispell but in any case this > > solution is better then die as in confutils_require_any solution. > > > > oops, my bad, I thought there was a non-dying variant. Maybe a good thing to > add to the eclass ? > If it doesn't die it will leave the package with library only and no providers. The best solution seems to enable ispell by default (EAPI 1), IMO. (In reply to comment #8) > Maybe a good thing to add to the eclass ? Actually there is nothing to add in eclass as my solution is based on analyzing what is best for this package and could vary from package to package. (In reply to comment #9) > If it doesn't die it will leave the package with library only and no providers. > The best solution seems to enable ispell by default (EAPI 1), IMO. Again, notice, that there is no ispell provider. There is ispell like library, so it's better avoid ispell USE flag in this case. Also notice, that in future it's good idea to enable hunspell by default in profiles. If you enable ispell in ebuild now, you'll end having two defaults for now reason. This is the case where per ebuild defaults are not best idea and where is a clean way to avoid them :) (In reply to comment #10) > (In reply to comment #8) > > Maybe a good thing to add to the eclass ? > > Actually there is nothing to add in eclass as my solution is based on analyzing > what is best for this package and could vary from package to package. > > (In reply to comment #9) > > If it doesn't die it will leave the package with library only and no providers. > > The best solution seems to enable ispell by default (EAPI 1), IMO. > > Again, notice, that there is no ispell provider. There is ispell like library, > so it's better avoid ispell USE flag in this case. > > Also notice, that in future it's good idea to enable hunspell by default in > profiles. If you enable ispell in ebuild now, you'll end having two defaults > for now reason. This is the case where per ebuild defaults are not best idea > and where is a clean way to avoid them :) > --enable-ispell adds support for ispell (but enchant has its own implementation) You only need to add a dictionary to get it working. (In reply to comment #11) > --enable-ispell adds support for ispell (but enchant has its own > implementation) You only need to add a dictionary to get it working. Ah, we don't have ispell USE flag. Sorry for disinformation. Then ispell is Ok. Created attachment 160172 [details, diff]
Patch against the current 1.3.0 ebuild
Changes in this patch.
* Made ispell flag default.
* Added a postinst message for ispell USE flag (for myspell there's already has a message in the package, for aspell we depend on virtual/aspell-dict)
* Add back what I have unintentionally removed.
Created attachment 160603 [details, diff]
Patch against the current 1.3.0 ebuild
Changes since the last patch:
* Revert USE defaults.
* Remove confutils_require_any check.
* Fallback to ispell when no USE flags are given.
Should postinst message be given if we fallback?
Created attachment 160608 [details, diff] enchant-1.4.2.ebuild.patch Serkan, here is my version of this ebuild. Besides some changes back (see below) it adds fixes for bug 145188. Without explicit paths to dictionaries I doubt enchant works. At least I tested hunspell and it failed to see dictionaries. Have you tested ebuild? I tested ispell support and found that it is completely broken in enchant (take a look at my upstream bug report http://bugzilla.abisource.com/show_bug.cgi?id=11703 where upstream confirms that). This means that we should drop it as ispell support completely and thus there is now way not to die... The question I think we should resolve before bumping this package. Have looked at other bugs? e.g. bug 207025 and bug 226213? Does this version resolves them? Does this version still has parallel make in install broken (yup we need to find old bug and try to reproduce it)? As soon as we'll have either positive or negative answers to this questions I'll be ok to bump enchant. (In reply to comment #15) > Created an attachment (id=160608) [edit] > enchant-1.4.2.ebuild.patch > > Serkan, here is my version of this ebuild. Besides some changes back (see > below) it adds fixes for bug 145188. Without explicit paths to dictionaries I > doubt enchant works. At least I tested hunspell and it failed to see > dictionaries. Have you tested ebuild? > Myspell works with that way. Aspell works withouth adding that aspell parameter (a new patch will follow) > I tested ispell support and found that it is completely broken in enchant (take > a look at my upstream bug report > http://bugzilla.abisource.com/show_bug.cgi?id=11703 where upstream confirms > that). This means that we should drop it as ispell support completely and thus > there is now way not to die... > > The question I think we should resolve before bumping this package. Have looked > at other bugs? e.g. bug 207025 and bug 226213? Does this version resolves them? > Does this version still has parallel make in install broken (yup we need to > find old bug and try to reproduce it)? As soon as we'll have either positive or > negative answers to this questions I'll be ok to bump enchant. > Now enchant can use external hunspell so #207025 is fixed. The test issue flameeyes reported is still there. Created attachment 160614 [details, diff]
Patch against the current 1.3.0 ebuild
Removed the extra aspell option.
(In reply to comment #16) > (In reply to comment #15) > > Created an attachment (id=160608) [edit] > > enchant-1.4.2.ebuild.patch > > > > Serkan, here is my version of this ebuild. Besides some changes back (see > > below) it adds fixes for bug 145188. Without explicit paths to dictionaries I > > doubt enchant works. At least I tested hunspell and it failed to see > > dictionaries. Have you tested ebuild? > > > > Myspell works with that way. Aspell works withouth adding that aspell parameter > (a new patch will follow) > > > I tested ispell support and found that it is completely broken in enchant (take > > a look at my upstream bug report > > http://bugzilla.abisource.com/show_bug.cgi?id=11703 where upstream confirms > > that). This means that we should drop it as ispell support completely and thus > > there is now way not to die... > > > > The question I think we should resolve before bumping this package. Have looked > > at other bugs? e.g. bug 207025 and bug 226213? Does this version resolves them? > > Does this version still has parallel make in install broken (yup we need to > > find old bug and try to reproduce it)? As soon as we'll have either positive or > > negative answers to this questions I'll be ok to bump enchant. > > > > Now enchant can use external hunspell so #207025 is fixed. The test issue > flameeyes reported is still there. > Regarding to parallel install see my initial patch was withouth -j1 and worked fine (Didn't try to reproduce the old bug though) So we have 2 issues left. * Not building tests by default. * Test paralle make. (In reply to comment #15) > Created an attachment (id=160608) [edit] > enchant-1.4.2.ebuild.patch > > I tested ispell support and found that it is completely broken in enchant (take > a look at my upstream bug report > http://bugzilla.abisource.com/show_bug.cgi?id=11703 where upstream confirms > that). This means that we should drop it as ispell support completely and thus > there is now way not to die... Huh. Found bug #132433 about the issue. Created attachment 160647 [details, diff]
Patch against the current 1.3.0 ebuild
* Parallel make seems to work fine.
* Fixed compiling tests when run with FEATURES="-test". Thanks goes to Diego Pettenò.
Bumped in portage. Added other bugs fixed as dependencies. |