Ray-V / tde-slackbuilds

A fork of Thorn Inurcide's SlackBuilds for the Trinity Desktop Environment
17 stars 5 forks source link

ispell vs. aspell #22

Closed Ndolam closed 3 years ago

Ndolam commented 3 years ago

Hi Ray, I built 14.0.9 yesterday on S64-14.2. I have aspell installed, and chose it when asked. Part way through the build I get

-- Checking for one of the modules 'dbus-1' -- Using /etc/dbus-1/system.d for DBUS system configuration files -- Using /etc/dbus-1/session.d for DBUS session configuration files -- Using /usr/share/dbus-1/services for DBUS session service files -- Using /usr/share/dbus-1/system-services for DBUS system service files -- Checking for one of the modules 'dbus-1-tqt' -- Looking for inotify.h -- Looking for inotify.h - not found -- Looking for sys/inotify.h -- Looking for sys/inotify.h - found -- Found ASPELL: /usr/lib64/libaspell.so
CMake Error at cmake/modules/TDEMacros.cmake:65 (message): #################################################

Spell checker selected as default (ISPELL) is not enabled to build.

################################################# Call Stack (most recent call first): CMakeLists.txt:1187 (tde_message_fatal)

-- Configuring incomplete, errors occurred! See also "/tmp/build/tmp-tdelibs/tdelibs-trinity-14.0.9/build-tdelibs/CMakeFiles/CMakeOutput.log". See also "/tmp/build/tmp-tdelibs/tdelibs-trinity-14.0.9/build-tdelibs/CMakeFiles/CMakeError.log". tdelibs.SlackBuild FAILED at line 135 Script done, file is /tmp/tdelibs-14.0.9-x86_64-1-build-log Cannot install /tmp/tdelibs-14.0.9--1.txz: file not found

Not quite sure of what its problem is, I edited the file specifying that (changed it to Ispell) and restarted the build. Everything worked fine after that. I don't know what its problem is, but I thought I'd mention it.

Thanks for your efforts with these slackbuilds.

SlavekB commented 3 years ago

Starting with R14.0.9, there is a new build option DEFAULT_SPELL_CHECKER, where the default value is ISPELL. If you want aspell as your default, add this build option to your build script and set it to ASPELL.

Ndolam commented 3 years ago

Um, yah, but... Ray's BUILD-TDE.sh script asks which one I want, and I gave an answer. (Above I said I changed it to ispell, but I meant to say I changed it to aspell). I really don't seriously care about its default spelling checker, but given that I have both aspell and ispell installed, and I had to pick something, I (unfortunately) picked ispell. What I did not know is that it (apparently??) wants a libispell.so, which I do not have.

So maybe what could be done is to either (1) only list spelling choices that are going to work, given what is installed on the system, or (2) if a spelling checker is chosen that doesn't have all the required components available, tell the user they need to install stuff or make a different choice.

SlavekB commented 3 years ago

Ah, I see it. In this case, it is advisable to make a pull-request, which according to the answer not only sets the appropriate option to ON, but also sets the default spell checker – see this part of the script:

https://github.com/Ray-V/tde-slackbuilds/blob/master/Core/tdelibs/tdelibs.SlackBuild#L129

Ndolam commented 3 years ago

Ummm... you sure it is that part of that script? Because by the time we get there, the user is no longer entering his/her desires. I would have thought something more along the following edit to BUILD-TDE.sh:

only run this if tdelibs has been selected

rm -f $TMPVARS/SPELL [[ $(grep -o tdelibs $TMPVARS/TDEbuilds) ]] && { ASPELL= HSPELL= ISPELL= SUFFIX= [ "$ARCH" = x86_64 ] && SUFFIX=64 [ -r /usr/lib$SUFFIX/libaspell.so ] ASPELL='" Aspell" ""' # note surrounding single quotes ... similar condition for ISPELL and HSPELL ... dialog --cr-wrap --nocancel --no-shadow --colors --title " Spell checker " --men u \ " Choose a Spell checker. If you chose a spell checker it must be installed, or the build will exit.

This won't affect any Spell checker being installed later, it's just a work-arou nd for a mandatory selection being forced in the source.

" \ 19 75 4 \ $ASPELL \ $HSPELL \ $ISPELL \ " None" "Don't have one installed" \ 2> $TMPVARS/SPELL }

Untested, probably need to fuss with the quoting and/or the empty strings.

That, of course, is the "don't show the user if the needed component is not already installed" choice. Which may or may not be the better of the two I suggested.

Thoughts?

SlavekB commented 3 years ago

In the BUILD-TDE.sh script, the user makes his selection, but in the Core/tdelibs/tdelibs.SlackBuild script, variables are set based on this selection.

The code in the tdelibs.SlackBuild script is currently written so that according to the user's choice, exactly one spell checker is enabled. Therefore, there is also a need for this spell checker to be set as the default. If you want to modify it so that tdelibs is built with the support of multiple spell checkers, it would have to be modified in both scripts.

You need to find a consensus on how to resolve this. I am not a Slackware user, so this decision is up to you.

Ndolam commented 3 years ago

Hi Slavek,

Thanks for the input, but I'm not following you.

At no time did I want two spelling checkers. My bug report here (well, what I was trying to say, maybe I have been unclear) is that BUILD-TDE.sh allowed me to pick something which caused the build to fail. I don't think I have any issue with Core/tdelibs/tdelibs.SlackBuild using the value that BUILD-TDE.sh set up; it was (as far as I know) doing what it was told to do. The issue is that when BUILD-TDE.sh offered me some options, where one of them (the one I picked) was apparently doomed to failure. I think it makes sense to exclude giving the user options that will cause the build to fail.

SlavekB commented 3 years ago

I dare say that the problem is definitely in Core/tdelibs/tdelibs.SlackBuild.

The BUILD-TDE.sh script is prepared so that the user can select one spell checker. Starting with R14.0.9, here is a new option that specifies the default spell checker and its default value is ISPELL. Therefore, it is essential that the value selected in BUILD-TDE.sh be used in Core/tdelibs/tdelibs.SlackBuild not only to set WITH_<spell-checker>, but also to set DEFAULT_SPELL_CHECKER to match it. Otherwise, it will cause a mismatch between the build options and thus the FTBFS.

We can discuss that the CMake rules in tdelibs could set the default spell checket automatically if only one is allowed, but even if such a change was made, it will apply to R14.0.10 and later. Therefore, for R14.0.9 it is necessary to change SlackBuilt as I mentioned.

Ndolam commented 3 years ago

I think I finally understand what you are saying about tdelibs.SlackBuild, thanks. At this point I'm no longer 100% sure what option I selected when running BUILD-TDE.sh (and the file holding that information was deleted along the way), but since I eventually got tdelibs to build, I guess it is all good (for me).

Hopefully Ray will chime in at some point and Do The Right Thing.

Cheers.

Ray-V commented 3 years ago

Thanks for your efforts with these slackbuilds.

Thanks for the feedback, it's good to know that all the effort has been useful.


Hopefully ... Doing The Right Thing ...

Sorry for the hassle.
I obviously overthought the work-around when all that was needed was to change the message type from tde_message_fatal to message.

However, thanks to Slavek for his comments which have led to a better solution.

Slackware doesn't include Hspell, so the test for setting spell checkers ON only needs to be for Aspell and Ispell.

These patches work for me for no spell checker installed; either installed; or both installed.

The BUILD-TDE.sh dialog isn't required ..

--- BUILD-TDE.sh
+++ BUILD-TDE.sh
@@ -485,25 +485,4 @@
 }

-
-## only run this if tdelibs has been selected
-rm -f $TMPVARS/SPELL
-[[ $(grep -o tdelibs $TMPVARS/TDEbuilds) ]] && {
-dialog --cr-wrap --nocancel --no-shadow --colors --title " Spell checker " --menu \
-"
-Choose a Spell checker.
-If you chose a spell checker it must be installed, or the build will exit.
-
-This won't affect any Spell checker being installed later, it's just a work-around for a mandatory selection being forced in the source.
- 
-" \
-19 75 4 \
-" Aspell" "" \
-" Hspell" "" \
-" Ispell" "" \
-" None" "Don't have one installed" \
-2> $TMPVARS/SPELL
-}
-
-
 ## only run this if tdebase has been selected
 rm -f $TMPVARS/RUNLEVEL

As the slack-desc for Aspell has this entry, I've prioritised Aspell over Ispell
# GNU Aspell is a spell checker designed to eventually replace Ispell.

So if Aspell, or no spell checker, is installed the default spell checker is Aspell.
If only Ispell is installed the default spell checker is Ispell.

--- Core/tdelibs/tdelibs.SlackBuild
+++ Core/tdelibs/tdelibs.SlackBuild
@@ -127,8 +127,11 @@
 }

-[[ $(grep Aspell $TMPVARS/SPELL) ]] && ASPELL=ON || {
-[[ $(grep Hspell $TMPVARS/SPELL) ]] && HSPELL=ON ; } || {
-[[ $(grep Ispell $TMPVARS/SPELL) ]] && ISPELL=ON ; } || \
-sed -i 's|^.*Spell checker selected as default.*$|message( STATUS " ## no spell checker selected ##" )|' ../CMakeLists.txt
+## Slackware doesn't include Hspell
+## Prioritise Aspell according to its slack-desc:
+# GNU Aspell is a spell checker designed to eventually replace Ispell.
+[[ -d /usr/lib$LIBDIRSUFFIX/ispell ]] && ISPELL=ON && DEF_SP_CHKR=ISPELL
+[[ -d /usr/lib$LIBDIRSUFFIX/aspell ]] && ASPELL=ON && DEF_SP_CHKR=ASPELL # override ISPELL if both installed
+## just show message without failing
+sed -i 's|tde_message_fatal( "Spell checker|message( "Spell checker|' ../CMakeLists.txt

 cmake \
@@ -139,4 +142,5 @@
     -DCMAKE_BUILD_TYPE=Release \
     -DCMAKE_INSTALL_PREFIX=$INSTALL_TDE \
+    -DDEFAULT_SPELL_CHECKER=${DEF_SP_CHKR:-"ASPELL"} \
     -DSYSCONF_INSTALL_DIR=$SYS_CNF_DIR \
     -DLIB_SUFFIX=$LIBDIRSUFFIX \
@@ -157,5 +161,5 @@
     -DWITH_GCC_VISIBILITY="OFF" \
     -DWITH_HAL="OFF" \
-    -DWITH_HSPELL=${HSPELL:-"OFF"} \
+    -DWITH_HSPELL="OFF" \
     -DWITH_INOTIFY="ON" \
     -DWITH_ISPELL=${ISPELL:-"OFF"} \
Ndolam commented 3 years ago

Ray (and Slavek), thanks very much for the updates.

And Ray, again I will say thanks very much for the slackbuilds. The BUILD-TDE.sh script makes using the slackbuilds a walk in the park.

Ndolam commented 3 years ago

I will close this now, since I think it is all good.