Libelektra: Replace `try_compile`

[ingwinlu]

https://github.com/ElektraInitiative/libelektra/blob/master/src/plugins/passwd/CMakeLists.txt#L4

This is not reliable as it will not fail when the function is not available (no -Werror that would make try_compile fail when it is not linkable. so it just warns about an implicit declaration).

[markus2330]

Thank you for raising the question!

There is not really an easy answer for this question. Systems that do not have the symbol should fail during linking in try_compile. (The name try_compile is confusing, it actually also links.) So for these systems everything should work as expected.

I assume you talk about systems that have the symbol but some header file magic might (not) include the prototype. In such cases we have undefined behavior (if the prototype does nothing it works; if the prototype has some magic to select the correct symbol it will segfault at run-time.)

-Werror might not always be the correct choice because compilers might generate unrelated warnings (e.g. return value not used). (The -Werror build job will hopefully catch such problems for our compilers but not for every compiler out there.)

So I think we should only fix the build system if it is broken, meaning we actually encounter (or know that we would encounter) problems on a specific system.

[ingwinlu]

i ran into a problem with the file mentioned in the OP when building a static musl build.
The build was configured by cmake as if hasfgetpwent was available, but during linking it issued a warning regaring implicit declaration (and would have probably segfaulted when running).

Isn't https://cmake.org/cmake/help/v3.0/module/CheckFunctionExists.html the better alternative for all such checks?

[markus2330]

Yes, of course it is. This is a very good suggestion! And we already use it at some places.

[ingwinlu]

Updated OP

[ingwinlu]

The current implementation is definitely not working:
https://build.libelektra.org/jenkins/blue/organizations/jenkins/libelektra/detail/PR-2063/52/pipeline/444/
Cmake believes the function is available. segfaults during tests.

[markus2330]

Yes, checkFunctionExists is the way to go. Then such problems are (hopefully) fixed by the CMake community.

[ingwinlu]

checkFunctionExists will not magically take care of setting definitions. or if you want to have another look at it: checkFuncitonExists also returns true for functions that are not exported due to missing feature macros.

2078 has a commit in it that uses checkFunctionExists and sets the definitions accordingly.

@markus2330 how do you like this approach? can we use that in other cases as well?

[markus2330]

Thank you for taking care of this!

In general I like that we get rid of maintaining these little source files (and use checkFunctionExists instead). But I do not like that we might fail to detect if we are able to compile some plugin. (In #2078 the #error might be triggered. It would be better if we can detect this during the CMake stage and simply remove the plugin.)

[ingwinlu]

I am not convinced checkFunctionExists is the way to go anymore as it detects functions that might not be available (depending on various things).

I will test some more in #2056 and #2078.

[markus2330]

Seems like we start using CMake in non-standard ways.

[ingwinlu]

@markus2330 i don't think there is a 'standard' way to use CMake except for trivial projects :p,

My original approach was a 'bad' one as the behaviour depends on the libc implementation. Which is never something you want in the long run.

The new one is very simple: to check if a symbol is exported (without modifying any settings). use it if true, or disable if no alternative implementation not requiring it is available.

This has the same behaviour as it is currently on gnu + macos. It just won't do on musl because they do not assume a set _GNU_SOURCE. So to build it on musl you would need to cmake -DCOMMON_FLAGS="-D_GNU_SOURCE" ..

Biggest plus with this approach is that plugins get disabled correctly if you set the c std to c99. Now suddenly _GNU_SOURCE is no longer set and currently that means a lot of plugins are included but would segfault when run.

[ingwinlu]

It is also easier to add a section to add_plugin called REQUIRED_SYMBOLS where we would pass a list of symbols.

[markus2330]

It is also easier to add a section to add_plugin called REQUIRED_SYMBOLS where we would pass a list of symbols.

This proposal is along the lines with #2088, we should discuss it there.

[ingwinlu]

from #2095 we can take some examples on how I would like to implement it currently.

• libs/elektra: https://github.com/ElektraInitiative/libelektra/pull/2095/commits/ab6f672b6c53ec93a1b45a1672f504154686b8dd use strings.h if available, else use hardcoded replacements
• plugins/boolean: https://github.com/ElektraInitiative/libelektra/pull/2095/commits/46198c0bee61ff96d63b1c86e2d02d6118f3c746 use str functions if they are there, else disable plugin

[ingwinlu]

We now have a good way to solve this with save_check_symbol_exists.

[markus2330]

Yes, thank you for this solution! Are there still places where it needs to be used or can we close the issue?