Don't export internal helper executables
ClosedPublic

Authored by vkrause on Jul 30 2017, 7:44 PM.

Details

Summary

Exporting the executables will make the cmake config file check for their
presence when used, which isn't really necessary, and is actually a problem
for cross-compilation, where the helper executables don't exist for the
target but for the host.

Diff Detail

Repository
R246 Sonnet
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
vkrause created this revision.Jul 30 2017, 7:44 PM
Restricted Application added a project: Frameworks. Β· View Herald TranscriptJul 30 2017, 7:44 PM
dfaure added a subscriber: dfaure.Aug 8 2017, 7:40 AM

Why are those executables even installed? For crosscompiling maybe? I'm confused about how that would work. One would build sonnet for host, install it, and then build sonnet for target, importing targets from the host build?
Isn't that "import" what this commit just broke?

Maybe the INSTALL lines should be inside the else, there's no need to install the compiled-for-target executables....

(AFAICS they are internal, not used outside sonnet at all)

As per the if below the proposed change it is used for cross compiling.

Subscribing original author and committer.

(kind of off topic: This code should really be changed to use the KF5_HOST_TOOLING we use elsewhere, where the tools get put into its own Targets.cmake file separate from the libs and then loaded from KF5_HOST_TOOLING when xcompiling https://phabricator.kde.org/source/kconfig/browse/master/KF5ConfigConfig.cmake.in;023e3ecfe985e09f786134fc28793d24383998f8$11 when not xcompiling the targets are fairly useless and don't need to get imported at all)

(kind of off topic: This code should really be changed to use the KF5_HOST_TOOLING we use elsewhere

You can show me a working example of this KF5_HOST_TOOLING support ?

https://phabricator.kde.org/source/kconfig/browse/master/KF5ConfigConfig.cmake.in;023e3ecfe985e09f786134fc28793d24383998f8$11 when not xcompiling the targets are fairly useless and don't need to get imported at all)

What content does the file included by

include("${KCONFIGCOMPILER_PATH}")

should have to be usable ?

I assume installation is done for cross-compilation, yes. For Yocto this wouldn't even be necessary though, we manually inject the host executables into the -dev packages there, for this it only matters that they are built, not that they are installed (see D7102 and D7104 for a complete example).

sitter added a comment.Aug 8 2017, 2:13 PM

I assume installation is done for cross-compilation, yes. For Yocto this wouldn't even be necessary though, we manually inject the host executables into the -dev packages there, for this it only matters that they are built, not that they are installed (see D7102 and D7104 for a complete example).

That's also not using the unified KF5_HOST_TOOLING approach 😒

sitter added a subscriber: apol.Aug 8 2017, 2:27 PM

https://phabricator.kde.org/source/kconfig/browse/master/KF5ConfigConfig.cmake.in;023e3ecfe985e09f786134fc28793d24383998f8$11 when not xcompiling the targets are fairly useless and don't need to get imported at all)

What content does the file included by

include("${KCONFIGCOMPILER_PATH}")

should have to be usable ?

The exported executable. Basically: CMake has two Targets.cmake files. One for the library artifacts and one completely separate one for the helpers. This allows including either/or depending on what is needed and by extension including the host's helpers when cross compiling

Further references:

This ultimately results in find_package(KF5Config) always importing KF5::kconfig_compiler, but depending on the xcompiling stuff it may be the host's.

This would work pretty much exactly the same for sonnet with the additional change that the xcompile if inside the source itself (https://phabricator.kde.org/source/sonnet/browse/master/data/CMakeLists.txt;c4c007cc3c0b9ee0ee46e91d026752662dd867a4$18) would change to find_file; include boilerplate of https://phabricator.kde.org/source/kconfig/browse/master/KF5ConfigConfig.cmake.in;023e3ecfe985e09f786134fc28793d24383998f8$12

I think @apol wrote the initial implementation of this magic and can probably answer questions better than me though :)

I think @apol wrote the initial implementation of this magic and can probably answer questions better than me though :)

I guess no. He also did not provide a working example see https://git.reviewboard.kde.org/r/128971/

apol accepted this revision.Jan 4 2018, 12:12 PM

This patch is correct, EXPORT is only useful if the target is used within cmake, AFAIU.

Furthermore, we should modify sonnet to install these applications into LIBEXEC rather than bin, otherwise we can't have both architectures installed along with the library that is going to use it (at least on some linux distros, e.g. debian based), but this we can do in a separate patch.

This revision is now accepted and ready to land.Jan 4 2018, 12:12 PM
apol added a comment.Jan 4 2018, 12:17 PM

Or as @dfaure says, don't install at all, because I don't see who uses this. Or remove altogether?πŸ”₯

In D7008#185897, @apol wrote:

Or as @dfaure says, don't install at all, because I don't see who uses this. Or remove altogether?πŸ”₯

That works for me on Yocto, but in general this seems wrong, as target-sonnet will need to use host-sonnet's executable(s).

In D7008#185897, @apol wrote:

Or as @dfaure says, don't install at all, because I don't see who uses this. Or remove altogether?πŸ”₯

The helpers are used by sonnet at build time, so to cross compile sonnet you need the helpers of the host (of e.g. x64) while you xbuild (for aarch64) as you cannot build the helpers for aarch64 and then run them on x64. Much the same as kconfig compliation, except the only user is the framework itself.

Mind you, one could solve this "properly" without having to install the helpers. I am not sure the time investment is worth the effort though given we know the kconfig approach is sane and working in production.
One could convert the helpers into a sepearate cmake "subproject" which gets built by the sonnet build (i.e. sonnet source contains sonnet-helpers source and builds them during build but disregarding the cross compile configs and never installing the helpers). Which could probably be done via ExternalProject. It does however raise the question of how do you pass cmake configuration to that subproject (different prefix so it can find libraries etc). So that may open another can of worms.

This revision was automatically updated to reflect the committed changes.