KDb: Better support for ICU
ClosedPublic

Authored by staniek on Feb 23 2018, 2:08 AM.

Details

Summary
  • KDb: ICU is required
  • KDb: prepend craft's lib dir to LIB path so find_package(ICU) finds our files. Otherwise it is possible that find_package(ICU) finds some random private copies such as those coming with MSVS.
NOTE: maybe make this fix general purpose by always prepending 'lib' dir to LIB?
Test Plan

craft kdb

Diff Detail

Repository
R877 Craft Blueprints for KDE
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
staniek requested review of this revision.Feb 23 2018, 2:08 AM
staniek created this revision.
staniek added a subscriber: Craft.

And adding NO_SYSTEM_ENVIRONMENT_PATH to find_library is no option for you?

# Look for the library.
find_library(
  ICU_LIBRARY
  NAMES ${LIB_NAMES}
  NO_SYSTEM_ENVIRONMENT_PATH
  DOC "Libraries to link against for the common parts of ICU")
mark_as_advanced(ICU_LIBRARY)

And adding NO_SYSTEM_ENVIRONMENT_PATH to find_library is no option for you?

# Look for the library.
find_library(
  ICU_LIBRARY
  NAMES ${LIB_NAMES}
  NO_SYSTEM_ENVIRONMENT_PATH
  DOC "Libraries to link against for the common parts of ICU")
mark_as_advanced(ICU_LIBRARY)

Thanks for the review. I should have mentioned that I tested this possibility quite carefully. In find_library it can work for Craft itself (and environment acting this way) but it's more like workaround because it can easily break non-Windows (and Windows non-craft) systems that depend on properly set up system locations.

Also the change would be in FindICU which I want to eventually remove and use the version from cmake installation itself (available just since cmake 3.7 so cannot make the switch just now), and I am actually moving to that since 3.2: we will start to use the new one in since KDb 3.2, change is pushed already https://phabricator.kde.org/R15:80eb44d2c2f0ad4f54ad19353a32af1f03aa9e77.

If we add NO_SYSTEM_ENVIRONMENT_PATH to find_package (what is not what you proposed but I checked that too) to make the workaround in KDb itself, it only disables usage of INCLUDE and PATH, also cmake docs confirm this. Not the LIB variable which contains paths such as C:/Program Files (x86)/Windows Kits/10/Lib/10.0.16299.0/um/x64 (set by the installer of the Kit, not me). So NO_SYSTEM_ENVIRONMENT_PATH changes nothing. The same happens no matter if my legacy FindICU.cmake is used or the stock one from newest CMake.

In this patch description I proposed to always prepend os.path.join(CraftStandardDirs.craftRoot(), 'lib') to LIB in Craft libs. I do not see disadvantages so far, i.e. no reason why this dire should not have priority. So you see any? It's similar thing we do for PATH (for DLLs and EXEs). If this is accepted, changes to compile() here are not needed and no craft-specific ifdefs will be needed.

The prepending potentially fixes any similar case for other packages.

vonreth accepted this revision.Feb 23 2018, 11:40 AM

I'd rather not change that environment variable for everything.

I'm ok with the change when you add a comment why this was needed.

Btw have you tried to copy the findicu from cmake 3.7 to your project?

This revision is now accepted and ready to land.Feb 23 2018, 11:40 AM

I'd rather not change that environment variable for everything.

I'm ok with the change when you add a comment why this was needed.

OK, one idea: can we add the behavior to to the utils API so packages that decide to want this behavior can just use the API without copy/pasting? It's not just about ICU even if I personally use it elsewhere so far (well I believe there can be conflicts with mysql libs installed to program files and to craft in the same OS).

Example:

class Package(CMakePackageBase):
   def __init__(self):
      CMakePackageBase.__init__(self)
      CMakePackageBase.prependCraftLib(True) <----- just one line needed

Btw have you tried to copy the findicu from cmake 3.7 to your project?

Yes I tried, it's much more extensive code than mine so I want to use it in the future versions. have not switched in 3.1 because we're in RC stage of the release. CMake's version behaves the same way as mine regarding finding library since the same find_library is used. I basically rarely see NO_SYSTEM_ENVIRONMENT_PATH being forces in public/OS-portable cmake files. Mostly only in app's own custom cmakelists.txt files. CMake's version also uses modern cmake target properties (i.e. ICU::i18n name, etc.) which is always good.

Prepending LIB was only used once, for a broken build system.

This revision was automatically updated to reflect the committed changes.