[nrf fromlist] cmake: zephyr_library_amend feature #219

tejlmand · 2019-10-17T12:57:59Z

This PR allows amending to a Zephyr library created inside Zephyr repo in a generic way.

Currently, if no files are added to a library, as example the drivers/entropy zephyr library because no drivers (c-files) are selected using Kconfig, the following error when running cmake / menuconfig:

CMake Error at /projects/github/ncs/zephyr/cmake/extensions.cmake:411 (add_library):                       
  No SOURCES given to target: drivers__entropy
Call Stack (most recent call first):
  /projects/github/ncs/zephyr/cmake/extensions.cmake:388 (zephyr_library_named)
  /projects/github/ncs/zephyr/drivers/entropy/CMakeLists.txt:3 (zephyr_library)

This PR allows to amend drivers files out-of-tree to an existing zephyr library.

As example:
drivers/entropy/CMakeLists.txt creates a zephyr library as:

 zephyr_library()

but due to kconfig settings, no files are added to the lib in Zephyr.

The amend function allows to amend to such a lib, by creating a
CMakeLists.txt file following identical folder structure in a Zephyr Module:

    <zephyr_module_oot>/drivers/entropy/CMakeLists.txt
    zephyr_library_amend()
    zephyr_library_sources() # Sources are amended to the original library

which means that the library created in Zephyr is no longer empty.

Signed-off-by: Torsten Rasmussen [email protected]

tejlmand · 2019-10-17T13:33:12Z

@SebastianBoe Please review this.
The PR is placed here to allow this to be used for the upcoming release.

Please comment if you believe code should be changed to get it approved upstream later.

mbolivar

changelog needs to begin with [nrf toup], please see https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrf/ug_dev_model.html#oss-repositories-downstream-project-history

tejlmand · 2019-10-18T07:46:20Z

changelog needs to begin with [nrf toup], please see https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrf/ug_dev_model.html#oss-repositories-downstream-project-history

Thanks for notifying.
[nrf toup] added.

SebastianBoe

PR must be created upstream before this is merged, and @thomasstenersen should review.

thomasstenersen · 2019-10-18T10:26:04Z

Some background. As I originally did this, the BT_LL_NRFXLIB selects CONFIG_ENTROPY_NRF_FORCE_ALT and adds its source to the Zephyr entropy library.
As discussed in the original PRs (zephyrproject-rtos/zephyr#14509, zephyrproject-rtos/zephyr#13631) we chose to go with a Nordic-only workaround.

@tejlmand :

I'm a bit out of the loop here, what is the scenario where the library will be empty?
Is there something else selecting this config without providing a source?

tejlmand · 2019-10-18T10:47:52Z

PR must be created upstream before this is merged, and @thomasstenersen should review.

yes, I know, but wanted to make this PR first to allow others and especially @thomasstenersen to provide feedback before going upstream.

tejlmand · 2019-10-18T10:54:31Z

Some background.

Thanks for commenting, and the link.

In our case with the cc310, an entropy driver will be provided internally in the fw-nrfconnect-nrf repo, similar to entropy_ll_nrfxlib.c.
I did consider to directly use the "magic" name drivers__entropy to place the file, but was under impression from @SebastianBoe at earlier talks that users are not supposed to directly use those names outside the zephyr CMake wrapped calls.
(I had not discovered you where already doing it that way)

If this approach is generally accepted (or at least accepted in nrf repo), I will add the cc310 entropy driver in same way.

SebastianBoe · 2019-10-18T10:56:23Z

You are correct that one should not use the library name when it can be avoided, but for this use-case we were unable to avoid it.

ioannisg

Why this patch cannot go upstream as is? Is there any controversial content?

tejlmand · 2019-10-18T11:44:04Z

You are correct that one should not use the library name when it can be avoided, but for this use-case we were unable to avoid it.

I'm happy to use the library name directly, but it is possible to avoid using the library name with this PR.
If we take this PR upstream, then the zephyr_library will not be created when CONFIG_ENTROPY_FORCE_ALT=y due to the safeguard in CMakeLists.txt, and instead it is possible fw-nrfconnect-nrf to do:

zephyr_library()
zephyr_library_sources(entropy_alt.c)

The principle in this PR will also allows a more generic approach for Zephyr community to provide alt implementation of entropy if they so wish.

@SebastianBoe and @thomasstenersen Please let me know if you think it has value to go ahead with this PR upstream for a more generic approach and to avoid directly using the lib name drivers__entropy, or if I should just follow same use-case as @thomasstenersen

tejlmand · 2019-10-18T11:45:38Z

Why this patch cannot go upstream as is? Is there any controversial content?

Due to the fact that we already did this in slightly different way internally, I just wanted to give people opportunity to give feedback first.

SebastianBoe · 2019-10-18T11:55:04Z

I'm sceptical, what if we want entropy_handlers.c and also a different implementation of the nrf driver?

I think we should follow @thomasstenersen 's work.

thomasstenersen · 2019-10-18T12:14:17Z

@tejlmand thanks for the context. It's not pretty and I myself would prefer your solution, really. But I don't think this will be accepted upstream until there are others than Nordic that needs this functionality (that was the argument last time).

mbolivar · 2019-10-18T19:04:25Z

But I don't think this will be accepted upstream until there are others than Nordic that needs this functionality (that was the argument last time).

If that is the case, please use [nrf noup] (noup != toup) so we can track it as such

tejlmand · 2019-10-21T11:05:46Z

But I don't think this will be accepted upstream until there are others than Nordic that needs this functionality (that was the argument last time).

If that is the case, please use [nrf noup] (noup != toup) so we can track it as such

Completely agree, if this is not going upstream.
But I still expect to push this upstream.

tejlmand · 2019-10-21T11:07:52Z

Thanks for all the comments and based on this, and especially this from @SebastianBoe

I'm sceptical, what if we want entropy_handlers.c and also a different implementation of the nrf driver?

So I have now made a more generic way of doing this.

I have reworked the PR, so read the updated description at the top.
@thomasstenersen @SebastianBoe please take a fresh look.

With the changes, it is now possible to create a <NCS>/nrf/drivers/entropy/CMakeLists.txt file with content:

zephyr_library_amend()
zephyr_library_sources_if_kconfig(entropy_cc310.c)
zephyr_library_sources_ifdef(CONFIG_ENTROPY_NRF_LL_NRFXLIB entropy_ll_nrfxlib.c)

@thomasstenersen for this to also work for entropy_ll_nrfxlib this file must be relocated, and secondly, if the Kconfig flag or the file is renamed, then you can also use the if_kconfig syntax instead.

SebastianBoe · 2019-10-21T11:32:13Z

cmake/extensions.cmake

Is this error missing an if-statement ?

good spotted, thansk.
This was added at last minut, cause I wanted to safe guard accidential calls from Zephyr repo.
Guess it was done a little to fast.

SebastianBoe · 2019-10-21T11:42:15Z

The approach LGTM, we just need a PR posted upstream and then we can merge this.

tejlmand · 2019-10-21T11:56:52Z

The approach LGTM, we just need a PR posted upstream and then we can merge this.

zephyrproject-rtos/zephyr#19980

SebastianBoe · 2019-10-21T12:19:16Z

The tag should be fromlist according to Marti.

mbolivar · 2019-10-21T16:06:32Z

Thanks for bearing with me on the patch bookkeeping! These tags make it a lot easier to track patches by category, and help bring attention to patches which may have been merged upstream during mergeups.

SebastianBoe · 2019-10-22T08:50:32Z

DNM until #220 is merged. It is policy to block PR's to fw-nrfconnect-zephyr when an upmerge PR is ongoing.

SebastianBoe

NACKing to block the merge

mbolivar · 2019-10-23T18:32:01Z

NACKing to block the merge

the mergeup is in, please revisit @SebastianBoe

This commit introduces the cmake extension zephyr_library_amend. This function allows for adding files in an out-of-tree Zephyr module to a zephyr library created in zephyr repo CMake files. As example: drivers/entropy/CMakeLists.txt creates an zephyr library as: zephyr_library() only available to zephyr itself. The amend function allows to amend to such a lib, by creating a CMakeLists.txt file following identical folder structure in a Zephyr Module: <zephyr_module_oot>/drivers/entropy/CMakeLists.txt zephyr_library_amend() zephyr_library_sources() # Sources are amended to the original library Signed-off-by: Torsten Rasmussen <[email protected]>

This commits adds ${IMAGE} in order to support multiimage in the amend feature. Signed-off-by: Torsten Rasmussen <[email protected]>

thst-nordic · 2019-10-24T12:51:36Z

The CI is failing because of an intermittent bug with QEMU that is on master also. You can merge this without the CI pass.

tejlmand force-pushed the zephyr_entropy_driver_alt branch from c9bd240 to d96ea43 Compare October 17, 2019 13:16

tejlmand changed the title ~~drivers: entropy: Allow using alternative driver implementation~~ [TOUP] drivers: entropy: Allow using alternative driver implementation Oct 17, 2019

tejlmand mentioned this pull request Oct 17, 2019

[DNM] drivers: entropy: Adjust Kconfig ENTROPY_FORCE_ALT flag nrfconnect/sdk-nrf#1298

Closed

mbolivar suggested changes Oct 17, 2019

View reviewed changes

tejlmand force-pushed the zephyr_entropy_driver_alt branch from d96ea43 to 7a04826 Compare October 18, 2019 07:41

SebastianBoe requested changes Oct 18, 2019

View reviewed changes

SebastianBoe changed the title ~~[TOUP] drivers: entropy: Allow using alternative driver implementation~~ [nrf toup] drivers: entropy: Allow using alternative driver implementation Oct 18, 2019

ioannisg reviewed Oct 18, 2019

View reviewed changes

tejlmand force-pushed the zephyr_entropy_driver_alt branch from 7a04826 to 95414db Compare October 21, 2019 10:46

tejlmand changed the title ~~[nrf toup] drivers: entropy: Allow using alternative driver implementation~~ [nrf toup] cmake: zephyr_library_amend feature Oct 21, 2019

SebastianBoe reviewed Oct 21, 2019

View reviewed changes

tejlmand force-pushed the zephyr_entropy_driver_alt branch from 95414db to 4d822bb Compare October 21, 2019 11:51

tejlmand force-pushed the zephyr_entropy_driver_alt branch from 4d822bb to e9d0d8c Compare October 21, 2019 11:59

tejlmand force-pushed the zephyr_entropy_driver_alt branch from e9d0d8c to 805e60b Compare October 21, 2019 12:30

tejlmand changed the title ~~[nrf toup] cmake: zephyr_library_amend feature~~ [nrf fromlist] cmake: zephyr_library_amend feature Oct 21, 2019

tejlmand force-pushed the zephyr_entropy_driver_alt branch from 805e60b to f83f330 Compare October 21, 2019 13:19

SebastianBoe approved these changes Oct 21, 2019

View reviewed changes

mbolivar approved these changes Oct 21, 2019

View reviewed changes

SebastianBoe added the DNM label Oct 22, 2019

SebastianBoe requested changes Oct 22, 2019

View reviewed changes

tejlmand force-pushed the zephyr_entropy_driver_alt branch from f83f330 to b2325fc Compare October 22, 2019 09:09

frkv mentioned this pull request Oct 24, 2019

Adding CC310 entropy driver utilizing nrf_cc310_platform library nrfconnect/sdk-nrf#1268

Merged

tejlmand added 2 commits October 24, 2019 08:33

[nrf noup] cmake: multiimage: zephyr_library_amend feature

0f79114

This commits adds ${IMAGE} in order to support multiimage in the amend feature. Signed-off-by: Torsten Rasmussen <[email protected]>

tejlmand force-pushed the zephyr_entropy_driver_alt branch from b2325fc to 0f79114 Compare October 24, 2019 06:33

SebastianBoe approved these changes Oct 24, 2019

View reviewed changes

SebastianBoe removed the DNM label Oct 24, 2019

SebastianBoe merged this pull request into nrfconnect:master Oct 24, 2019

[nrf fromlist] cmake: zephyr_library_amend feature #219

[nrf fromlist] cmake: zephyr_library_amend feature #219

Uh oh!

Conversation

tejlmand commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tejlmand commented Oct 17, 2019

Uh oh!

mbolivar left a comment

Choose a reason for hiding this comment

Uh oh!

tejlmand commented Oct 18, 2019

Uh oh!

SebastianBoe left a comment

Choose a reason for hiding this comment

Uh oh!

thomasstenersen commented Oct 18, 2019

Uh oh!

tejlmand commented Oct 18, 2019

Uh oh!

tejlmand commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SebastianBoe commented Oct 18, 2019

Uh oh!

ioannisg left a comment

Choose a reason for hiding this comment

Uh oh!

tejlmand commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tejlmand commented Oct 18, 2019

Uh oh!

SebastianBoe commented Oct 18, 2019

Uh oh!

thomasstenersen commented Oct 18, 2019

Uh oh!

mbolivar commented Oct 18, 2019

Uh oh!

tejlmand commented Oct 21, 2019

Uh oh!

tejlmand commented Oct 21, 2019

Uh oh!

SebastianBoe Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

tejlmand Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

tejlmand Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

SebastianBoe commented Oct 21, 2019

Uh oh!

tejlmand commented Oct 21, 2019

Uh oh!

SebastianBoe commented Oct 21, 2019

Uh oh!

mbolivar commented Oct 21, 2019

Uh oh!

SebastianBoe commented Oct 22, 2019

Uh oh!

SebastianBoe left a comment

Choose a reason for hiding this comment

Uh oh!

mbolivar commented Oct 23, 2019

Uh oh!

thst-nordic commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tejlmand commented Oct 17, 2019 •

edited

Loading

tejlmand commented Oct 18, 2019 •

edited

Loading

tejlmand commented Oct 18, 2019 •

edited

Loading

thst-nordic commented Oct 24, 2019 •

edited

Loading