samples/bluetooth/gatt: make it a CMake target and library

[marc-hb]

When adding sources outside the current directory using some "..", CMake
generates very long, absolute binary_dir output directories. Example
produced by sanitycheck -T samples/bluetooth/

build
├── CMakeFiles
│   ├── app.dir
│   │   ├── build.make
│   │   ├──
│   │   ├──
│   │   ├── flags.make
│   │   ├── home
│   │   │   └── john
│   │   │       └── zephyrproject
│   │   │           └── zephyr
│   │   │               └── samples
│   │   │                   └── bluetooth
│   │   │                       └── gatt
│   │   │                           └── bas.c.obj
│   │   ├── link.txt
│   │   ├── progress.make
│   │   └── src
│   │       └── main.c.obj
```
This "leaks" absolute and private source paths in output directories and
makes it extremely difficult to compare binaries compiled from different
source locations.

CMake supports a "binary_dir" argument that solves this, it's already
being used for modules in `/CMakeLists.txt`:
```
  add_subdirectory(${module_path}
                   ${CMAKE_BINARY_DIR}/modules/${module_name})
```
However this binary_dir argument requires a CMake target:
  https://cmake.org/pipermail/cmake/2019-June/069608.html
So also add a new "samples_bt_gatt" target and library.

The most common (only?) reason to add code in a _parent_ directory is
when that code is shared by more than one CMake project, as is the case
here. For all the usual software modularity reasons, every CMake guide I
ever saw tells to use targets for shared code.

.obj files are identical before/after this change. Linking in a
different order unfortunately causes address changes and ripple effects
which cause a lot of noise when trying to compare binaries and other
products past the linking stage(s). I looked anyway at the before/after
diff of the .map files and diffoscope of the .elf files and couldn't
find any difference besides the address changes.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>

[zephyrbot]

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

[marc-hb]

I have a few more tested commits similar to this one in my workspace, each one addressing the exact same problem but in totally different and unrelated places. This includes four different west modules, trying not to get distanced by @nashif who is creating new Zephyr modules at the speed of light. Modules are especially prone to this issue because they're located in "$ZEPHYR_BASE/../some/thing". As this one is the first one I would like an acknowledgement that the approach is OK not just here but in general. If not or not quite then I'll create a new github issue to discuss the approach in general.

[nashif]

PRIVATE kernel? how is this related to the kernel?

[marc-hb]

PRIVATE kernel? how is this related to the kernel?

Apparently through libc errno.h, see compilation failures below when you remove this line. Any better way to fix this?

PS: I think it's one of the advantages of using CMake targets: it exposes dependencies. Especially true for west modules (other, similar PRs not submitted yet)

Scanning dependencies of target samples_bt_gatt
[  1%] Building C object gatt_bindir/CMakeFiles/samples_bt_gatt.dir/bas.c.obj
In file included from ~/zephyrproject/include/misc/errno_private.h:34,
                 from ~/zephyrproject/lib/libc/minimal/include/errno.h:20,
                 from ~/zephyrproject/samples/bluetooth/gatt/bas.c:14:
tmp/sanity-out/qemu_cortex_m3/samples/bluetooth/peripheral_esp/sample.bluetooth.peripheral_esp/zephyr/include/generated/syscalls/errno_private.h:6:10: fatal error: syscall_list.h: No such file or directory
 #include <syscall_list.h>
          ^~~~~~~~~~~~~~~~
compilation terminated.

[  2%] Building C object gatt_bindir/CMakeFiles/samples_bt_gatt.dir/cts.c.obj
In file included from ~/zephyrproject/include/misc/errno_private.h:34,
                 from ~/zephyrproject/lib/libc/minimal/include/errno.h:20,
                 from ~/zephyrproject/samples/bluetooth/gatt/cts.c:14:
tmp/sanity-out/qemu_cortex_m3/samples/bluetooth/peripheral_esp/sample.bluetooth.peripheral_esp/zephyr/include/generated/syscalls/errno_private.h:6:10: fatal error: syscall_list.h: No such file or directory
 #include <syscall_list.h>
          ^~~~~~~~~~~~~~~~
compilation terminated.

etc.

[mbolivar]

Please use zephyr_library_link_libraries(...) or target_link_libraries(${ZEPHYR_CURRENT_LIBRARY}" ...). The abstraction in extensions.cmake doesn't guarantee that the target name is the same as the library name. (It happens to work for now, but I'm working on upstreaming a feature that changes that.)

[marc-hb]

Please use zephyr_library_link_libraries(...)

I couldn't find any zephyr_ wrapper with PRIVATE, that's why I used target_link_libraries()

or target_link_libraries(${ZEPHYR_CURRENT_LIBRARY}" ...)

It's really not intuitive to have to use ${ZEPHYR_CURRENT_LIBRARY} instead of just my_lib_name on the line immediately after zephyr_library_named(my_lib_name). Is this going to be the way forward or are there going to be more zephyr_ wrappers instead? Already discussed in issue #8439 and more generally #8827 (probably better places than here to resume those generic discussions if needed)

The abstraction in extensions.cmake doesn't guarantee that the target name is the same as the library name. (It happens to work for now, but I'm working on upstreaming a feature that changes that.)

Scary. Can you point at more details, some github issue maybe? The documentation of every target_ command in the CMake book starts with:

target_something_something(mytarget ...)

The named mytarget must have been created by a command such as add_executable(mytarget... ) or add_library(mytarget ...) and must not be an ALIAS target.

So unless I'm missing some CMake subtlety, the assumption that the CMake name[*] of a library is identical to the name of its corresponding target seems pervasive across all CMake's documentation and even the zephyr_ additional layer of indirection didn't seem to complexify that... yet?

[*] not the filename of course

[mbolivar]

I couldn't find any zephyr_ wrapper with PRIVATE, that's why I used target_link_libraries()

Got it. I missed that, thanks.

It's really not intuitive to have to use ${ZEPHYR_CURRENT_LIBRARY} instead of just my_lib_name on the line immediately after zephyr_library_named(my_lib_name).

Still, the API of these extensions has never guaranteed that the underlying CMake library's logical target name is the same as the first argument to zephyr_library_named(), so this is not new.

The fact that it happens to work is kind of unfortunate, though; it would have been nice for zephyr_library_named to add some sort of prefix to the library's name as a target. Especially since target names have to be globally unique, some sort of zephyr- namespace would have been nice to have from the beginning.

Is this going to be the way forward or are there going to be more zephyr_ wrappers instead? Already discussed in issue #8439 and more generally #8827 (probably better places than here to resume those generic discussions if needed)

Not sure what you mean by "the way forward": use of ${ZEPHYR_CURRENT_LIBRARY} allows the native CMake functions to be used, has for some time, and is not going away.

So unless I'm missing some CMake subtlety, the assumption that the CMake name[*] of a library is identical to the name of its corresponding target seems pervasive across all CMake's documentation and even the zephyr_ additional layer of indirection didn't seem to complexify that... yet?

I agree with your reading of the CMake documentation for its own functions, but that is not what extensions.cmake guarantees or requires:

# A Zephyr library can be constructed by the function zephyr_library
# or zephyr_library_named. The constructors create a CMake library
# with a name accessible through the variable ZEPHYR_CURRENT_LIBRARY.

(It goes on to say "The variable ZEPHYR_CURRENT_LIBRARY should seldom be needed since
the zephyr libraries have methods that modify the libraries", but I disagree with this on the grounds you've said -- we should be using the CMake functions whenever possible. It's just the target name itself which I want to be careful about here, but there is a variable to dereference to get that name already.)

And finally:

# The methods modify the CMake target_* API to reduce boilerplate;
#  PRIVATE is assumed
#  The target is assumed to be ZEPHYR_CURRENT_LIBRARY

This is relevant to upstreaming the multi-image feature used downstream by Nordic (#13672). Rework I am doing of that PR relies strongly on the abstraction barrier provided by ${ZEPHYR_CURRENT_LIBRARY} without introducing any new upstream CMake API requirements.

[marc-hb]

target_link_libraries(samples_bt_gatt PRIVATE kernel)

@nashif:

PRIVATE kernel? how is this related to the kernel?

Maybe this was because of
zephyr_library: missing 'kernel-mode' vs 'app-mode' documentation #19582

[ozhuraki]

LGTM

[mbolivar]

-1 for a minute while we're messing about with target names

[mbolivar]

Thanks for your patience. These changes LGTM

[marc-hb]

Thanks for your patience.

As stated above I will submit similar changes in other places so thank you for thoroughly reviewing the first one and making sure it's "right".

Any other comment anyone? CMake-czar @SebastianBoe maybe?

[marc-hb]

Rebased to add new bluetooth/peripheral_ht/ thermometer sample just added by... @overheat

No other change.

[SebastianBoe]

We should finish the work started here #9874 to change the gatt services into Zephyr libraries/sources that can be enabled through Kconfig.

This would improve consistency between gatt services (all gatt services can be enabled through Kconfig, instead of dis being enabled through Kconfig and the rest through CMake).

And cut down on CMake boilerplate (less code in each sample for enabling gatt services).

It would also improve consistency wrt. how gatt services and other Zephyr components are enabled (Kconfig).

I see it is noted that this is just one example where the patch to extensions.cmake is needed. Assuming it is agreed that gatt services should be moved to subsys/bluetooth/services I would prefer to review the extensions.cmake patch in the context of one of the non-gatt examples.

The leaking of absolute paths is a valid concern and must be fixed somehow.

[joerchan]

@marc-hb @SebastianBoe I created #17355 which includes the long forgotten work of #9874 and moves the rest of the GATT services as well.
Sorry for undoing all of the Cmake work that was done. The cmake-czar sebastian has spoken about what is "right"

[marc-hb]

I would prefer to review the extensions.cmake patch in the context of one of the non-gatt examples.
The leaking of absolute paths is a valid concern and must be fixed ...

OK, bad luck: I chose a bad example as a template. Will submit another one in another PR. Hopefully there's still a good one not in a module, that would spice things up.

Could you briefly comment on the cmake+target+library approach in general, pretending it were not gatt with a small imagination effort? This is a pretty low number of lines. Actually zero imagination effort for @mbolivar's commit specifically.

... somehow.

But not like this?

Sorry for undoing all of the Cmake work that was done.

One step back, two steps forward? Anything better than standing still! Constantly rebasing+fixing the other cases is getting a bit tedious (and now I'm glad I didn't submit the whole lot at once)

[SebastianBoe]

We should not deprecate zephyr_library_ in favor of target_.

#8439 has some good points, but it is not merged, and it is not canon.

Wrapping the target_ API has several benefits, for instance we want
zephyr_library_compile_options to have precedence over zephyr_compile_options.

3e7db9f#diff-e64e7d3f59d40d3325fa7a458c531dd3L409

And many others. It can not be deprecated offhandedly like this.

[marc-hb]

Wrapping the target_ API has several benefits [...] It can not be deprecated offhandedly like this.

So no "encouragement" to stop using the zephyr_ layer of indirection but is OK not to use it in a particular case like this one?
In other words is @mbolivar's new zephyr_library_cmake_target() "unwrapper" still OK in situations like this one or is just wrong in any case right now?

If the new "unwrapper" is wrong then should it be target_link_libraries(${ZEPHYR_CURRENT_LIBRARY}" PRIVATE kernel)? Some new zephyr_library_link_libraries_PRIVATE(...) wrapper to be added? Other? To be determined?

Sorry to insist in an obsolete PR but this seems to me like a basic, one-line zephyr_library_ question for a pretty simple use case. If it's actually complicated then please just say so and I'll create a new github issue for this question specifically.

[joerchan]

@marc-hb Sebastian is on vacation, so I'll try to answer on his behalf.

My understand was that a particular case like this should be avoided. There is a general pattern for how libraries are added and we should use that as much as we can.

If you need a zephyr_link_private then that should probably be added, there is already a zephyr_link_interface (although it is not in use anymore, so maybe that should be deleted).
I'm curious though, why do we need a PRIVATE version, I'm afraid I don't really understand the difference right now.
If you think PRIVATE is better maybe zephyr_library_link_libraries should be changed to use PRIVATE if possible.

If you wish to follow up on this, please create new github issue and assign Sebastian there.

[marc-hb]

New, simpler attempt to "set precedence" in zephyrproject-rtos/hal_nordic#6. Simpler because it doesn't require any zephyr_link_private or similar.