Don't call PythonInterp.cmake in KF5I18NMacros.
ClosedPublic

Authored by krop on Mar 9 2018, 11:28 AM.

Details

Summary

Due to CMake caching the variables, anything depending on ki18n would be
unable to call find_package(PythonInterp) with a specific version.

Instead we just hardcode the python executable path.

Test Plan
  • Downloaded, installed a tarball with translations.
  • The python2 backend in cantor doesn't claim python 2.7 was found if it's not true.

Diff Detail

Repository
R249 KI18n
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
krop created this revision.Mar 9 2018, 11:28 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 9 2018, 11:28 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
krop requested review of this revision.Mar 9 2018, 11:28 AM
krop edited the test plan for this revision. (Show Details)
krop added a subscriber: Build System.
krop added a comment.Mar 9 2018, 11:33 AM

Disclaimer: I'm not really happy with this solution.

The alternative is to call find_program(KI18N_PYTHON_EXECUTABLE) with a list of known names (python3.7 python3.6 python2.7 ...) and remove the find_package(PythonInterp) line.

krop added a comment.Mar 24 2018, 2:10 PM

ping. Unless someone objects, I will push this change in a couple days.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 27 2018, 1:54 PM
This revision was automatically updated to reflect the committed changes.

@cgiboudeaux @dfaure

I am almost entirely certain this broke building of ki18n itself when *it* contains translations. Neon packs translations into git builds, so here's the failure:
https://build.neon.kde.org/job/xenial_unstable_kde_ki18n_bin_amd64/135/consoleText

At a glance I'd say the change of include(cmake/KF5I18NMacros.cmake) to include KI18n_BINARY_DIR is to blame. Latter being the installation target directory it'd not be available at build time.

This is gonna cause trouble major trouble for the next release.

krop added a comment.Mar 28 2018, 10:25 AM

@cgiboudeaux @dfaure

[...]

This is gonna cause trouble major trouble for the next release.

No, it's a very minor issue that's already fixed.

dfaure added a comment.Apr 7 2018, 7:36 AM

This broke the unittest :

c
Internal cmake changing into directory: /d/kde/build/5/frameworks/ki18n/autotests/ki18n_install
Error: cmake execution failed
The C compiler identification is GNU 4.8.5
The CXX compiler identification is GNU 5.3.1
Check for working C compiler: /home/dfaure/bin/cc
Configuring
Check for working C compiler: /home/dfaure/bin/cc -- works
Detecting C compiler ABI info
Configuring
Detecting C compiler ABI info - done
Detecting C compile features
Configuring
Configuring
Configuring
Detecting C compile features - done
Check for working CXX compiler: /home/dfaure/bin/c++
Configuring
Check for working CXX compiler: /home/dfaure/bin/c++ -- works
Detecting CXX compiler ABI info
Configuring
Detecting CXX compiler ABI info - done
Detecting CXX compile features
Configuring
Configuring
Configuring
Configuring
Detecting CXX compile features - done
CMake Error at CMakeLists.txt:4 (include):

include could not find load file:

  KF5I18NMacros

CMake Error at CMakeLists.txt:6 (ki18n_install):

Unknown CMake command "ki18n_install".

Configuring

Thanks for having a look.

This change has broken the build of all projects that make use of pmap resources on Windows, as the Python path at build time is not necessarily the path at install time (and it definitely not on the Binary Factory nodes)
In particular it breaks KGeography - see https://binary-factory.kde.org/job/KGeography_Nightly_win32/112/console

Restricted Application edited subscribers, added: kde-frameworks-devel; removed: Frameworks. · View Herald TranscriptJul 14 2018, 4:58 AM
krop added a comment.Jul 14 2018, 9:51 AM

This change has broken the build of all projects that make use of pmap resources on Windows, as the Python path at build time is not necessarily the path at install time (and it definitely not on the Binary Factory nodes)
In particular it breaks KGeography - see https://binary-factory.kde.org/job/KGeography_Nightly_win32/112/console

FTR, this should be fixed by https://cgit.kde.org/ki18n.git/commit/?id=c5b721dc1753f8ca3c5af06169425751379f635f

Tests ran successfully:

  • Building ki18n, running ctest,
  • Applying the change to ki18n 5.48.0, building, running ctest