Improve spell checker build configuration #107
Merged
SlavekB
merged 5 commits from feat/add-config-spellcheckers into master 4 years ago
No reviewers
No Label
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: TDE/tdelibs#107
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'feat/add-config-spellcheckers'
Deleting a branch is permanent. It CANNOT be undone. Continue?
Here are some questions and suggestions.
##### check for aspell ########################### we need ASPELL_DATADIR tooset ( ASPELL_DATADIR "/usr/lib/aspell" CACHE PATH "aspell data directory" )I understand correctly that this is the default path, which may be overwritten below by the result from
pspell-config --pkgdatadir?Yes sure, variable will be overritten, but cache is not.
find_package( ASPELL )if( NOT ASPELL_FOUND )message(FATAL_ERROR "\nASPELL support are requested, but not found on your system" )else( NOT ASPELL_FOUND )I would prefer the condition to be reversed – not negative. So
if( ASPELL_FOUND )… dopspell-configdetection and movetde_message_fatalaselse( )block.or
endif ()here?FATALERRORwill stop processing CMake.I think Slavek is just saying to reverse the if condition and the if/else blocks order only.
Yes, exactly as @MicheleC says – I meant to reverse the condition and change the order of the blocks. Alternatively, your new code may not be inside
else()– it may simply follow this condition.Great, it looks good after adjustment.
fix to change
ASPELL_DATADIRcache fromexecute_processand prevent to override set from the command lineIt looks good… just one small note.
####" default spell checker #####################set( DEFAULT_SPELL_CHECKER "ISPELL" CACHE STRING "default spell checker" )if( WITH_ASPELL )set( DEFAULT_SPELL_CHECKER "ASPELL" )I assume this is the same case where the variable is set, but the cached value remains unchanged?
Questionable point:
Why FreeBSD and OpenBSD want to be ASPELL by default?
If it is just beneficial for packaging, I want to drop
WITH_ASPELLhunk, packager should use the new knob instead.If it came from Operating system related issues, I want to change the condition. But it probably means that
ispellwill not work as expected on such platforms.ispellplugin also should be optional?Removed selecting ASPELL part, packagers should select wanted one by themselves.
Ok, that sounds like a good idea.
Maybe there we could add a check to see if the corresponding
WITH_${DEFAULT_SPELL_CHECKER}is turned on and report a problem if one that is not enabled is required as default.Note: As you mentioned earlier, it seems like a good idea to create the
WITH_ISPELLoption so that it can also be optional.I added the
WITH_ISPELLoption and also a check if the spell checker selected as default is enabled for the build. That solves my previous comment.The question is whether to set
WITH_ISPELLas the defaultON?Well, I set
WITH_ISPELLtoONby default so that it doesn't cause FTBFS for existing builds. And since there are no further comments or suggestions, I will move forward and merge it. Thank you all for your efforts and cooperation.056d9c5d0dinto master 4 years ago056d9c5d0d.