Filter tests based on CMake variables. #12393

nashif · 2019-01-09T14:07:21Z

This PR introduces another way to filter tests based on variable available in CMakeCache.txt.

codecov-io · 2019-01-09T14:54:49Z

Codecov Report

Merging #12393 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #12393   +/-   ##
=======================================
  Coverage   48.62%   48.62%           
=======================================
  Files         313      313           
  Lines       46441    46441           
  Branches    10712    10712           
=======================================
  Hits        22584    22584           
  Misses      19398    19398           
  Partials     4459     4459

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 128d2e2...040c887. Read the comment docs.

galak · 2019-01-09T15:05:08Z

samples/net/sockets/big_http_download/sample.yaml

@@ -3,7 +3,7 @@ sample:
  name: big_http_download
 tests:
  test:
-    platform_exclude: esp32
+    filter: TOOLCHAIN_HAS_NEWLIBB


Typo, 'BB' at end of line.

SebastianBoe · 2019-01-09T15:07:37Z

samples/net/sockets/big_http_download/sample.yaml

@@ -3,7 +3,7 @@ sample:
  name: big_http_download
 tests:
  test:
-    platform_exclude: esp32
+    filter: TOOLCHAIN_HAS_NEWLIBB


Suggested change

filter: TOOLCHAIN_HAS_NEWLIBB

filter: TOOLCHAIN_HAS_NEWLIB

galak · 2019-01-09T15:11:54Z

This PR introduces another way to filter tests based on variable available in CMakeCache.txt.

Why do we need that?

SebastianBoe · 2019-01-09T15:14:28Z

Why do we need that?

Evidently, he needs to filter based on what kind of toolchain is used.

I believe we can already filter on Kconfig. So another approach would be to introduce a Kconfig.toolchain file. This would also solve some other problems we had where we needed to know in Kconfig, properties of the toolchain.

nashif · 2019-01-09T15:25:56Z

Why do we need that?

see discussion in #9522

nashif · 2019-01-09T15:35:48Z

Evidently, he needs to filter based on what kind of toolchain is used.

not quite, needed to filter based on capability of a toolchain. :-)

galak · 2019-01-09T15:52:17Z

see discussion in #9522

Seems like we should have the filter be on CONFIG_NEWLIB_LIBC, and that we need to have some means to convey if CONFIG_NEWLIB_LIBC is supported based on the toolchain.

nashif · 2019-01-09T15:59:02Z

Seems like we should have the filter be on CONFIG_NEWLIB_LIBC, and that we need to have some means to convey if CONFIG_NEWLIB_LIBC is supported based on the toolchain.

how exactly do you want to do that? if you filter on CONFIG_NEWLIB_LIBC, then it is always true because it will be in the application configuration. I am not a big fan of creating a Kconfig layer for toolchains, toolchain configuration should only be cmake (toolchain file in cmake speak).

regardless of the newlib issue, I think adding another filter based on cmake can be useful.

galak · 2019-01-09T16:11:42Z

how exactly do you want to do that? if you filter on CONFIG_NEWLIB_LIBC, then it is always true because it will be in the application configuration. I am not a big fan of creating a Kconfig layer for toolchains, toolchain configuration should only be cmake (toolchain file in cmake speak).

regardless of the newlib issue, I think adding another filter based on cmake can be useful.

We'll not sure how we intend to define toolchain features, but we can use kconfigfunctions.py as a means to access that info.

Mostly I feels for this specific case of NEWLIB, we need some means to pull toolchain features into Kconfig, since you can now enable NEWLIB for a toolchain that doesn't support it. So we have 2 means to say something similar and they may not get used consistently.

galak

We need to make the NEWLIB feature exposed to Kconfig as well so that we are consistent.

andyross

This does everything I want it to. And I tend to agree with @nashif that this is probably best done outside of kconfig as it's not an app or platform configuration thing. Though I could live with kconfig too obviously as that was the shortcut I picked originally.

I guess it depends on how closely tied we think sanitycheck/CI/SDK are. If they're all a unified group then it doesn't matter and kconfig is easier. If we want sanitycheck to run reliably on someone's custom toolchain (e.g. where they didn't bother with newlib even though everything else is compatible), then this makes more sense.

nashif · 2019-01-10T20:44:42Z

We need to make the NEWLIB feature exposed to Kconfig as well so that we are consistent.

I am not following why you want to do that. This change does not change the code or the Kconfig setup or dependencies, it is isolated to making sure our filtering for sanitycheck is sane and scalable and done on the toolchains that we often tests with the assumption that anyone should come with their own toolchain file and build zephyr without having to deal with Kconfig dependencies.

galak · 2019-01-10T23:27:56Z

I am not following why you want to do that. This change does not change the code or the Kconfig setup or dependencies, it is isolated to making sure our filtering for sanitycheck is sane and scalable and done on the toolchains that we often tests with the assumption that anyone should come with their own toolchain file and build zephyr without having to deal with Kconfig dependencies.

I feel like this change now we have 2 different means that convey NEWLIB support from a test perspective. One is does the toolchain support newlib, the other is newlib configured. It seems like we should have lib/libc/Kconfig have something like:

config NEWLIB_LIBC
   depends on TOOLCHAIN_SUPPORTS_NEWLIB
   ...

and maybe TOOLCHAIN_SUPPORTS_NEWLIB is depends on $(cmake_var_test,TOOLCHAIN_SUPPORTS_NEWLIB) implemented in kconfigfunctions.py

nashif · 2019-01-11T15:42:12Z

I feel like this change now we have 2 different means that convey NEWLIB support from a test perspective.

Those are 2 different things, I do not see any duplication here.

Does the toolchain support feature A
Application wants to use feature A

I do not see them on the same level.

Adding the dependency on kconfig level is a bad idea IMO, where does this end? Will we be checking for the host OS in kconfig next and showing users different configurations based on their environment? IMO the configuration has to be the same regardless of the environment being used or when there are interchangeable components such as the toolchain or the build environment.

and maybe TOOLCHAIN_SUPPORTS_NEWLIB is depends on $(cmake_var_test,TOOLCHAIN_SUPPORTS_NEWLIB) implemented in kconfigfunctions.py

the macros are a cool feature indeed, but I would apply some caution in using them for this and many other things, lots of magic and dependencies will be created which will make the configuration system very difficult to maintain and might result in random configuration changes caused by the dynamic nature.

Talking about dependencies, you proposed macro will not work, the cache file is created after kconfig file was generated but there might be other issues involved.

This change is not intrusive to Zephyr and its configuration system:

adding filtering based on cmake variable is a good thing in general regardless of the toolchain, I am starting to think about other usecases for that
The toolchain file changes would be required even with your proposed solution
The testcase yaml files will be simplified and not tied to boards, if we have a toolchain for esp32 that would support newlib, no changes needed to many testcases.

carlescufi · 2019-01-13T23:49:17Z

@galak I am not sure how your proposal would work. Doesn't the filtering (i.e. will this particular test be compiled and run for this arch/toolchain) need to happen before the Kconfig tree is parsed? Otherwise, are we actually running Kconfig separately from the actual build in order to see if a particular Kconfig option resolves correctly?

SebastianBoe · 2019-01-14T11:27:41Z

@carlescufi : This (filtering based on Kconfig) is already supported. Not sure how it was implemented. I assume they pre-run CMake and then check the .config file.

SebastianBoe · 2019-01-14T11:31:00Z

About whether Kconfig should be aware of the capabilities of the
toolchain.

IMHO it should. This would simplify the configuration system for
the user as it would allow us to hide options from the user.

E.g. don't present the user with the ability to enable NEWLIB,
LTO, stack-canaries, retpoline, etc..

It would also allow us to detect whether the enabled toolchain
can build the enabled ARCH. A misconfiguration that today creates
an obscure error message.

About where the scope of Kconfig ends. It ends when an option
does not affect the final binary. E.g. the IDE, editor, version
control system, host OS etc. does not affect the final binary,
but the enabled components, board, and capabilities of a
toolchain does.

The toolchain is not interchange-able (although I wish it was).

About how to have Kconfig be aware of the capabilities of a
toolchain. I am not familiar with 'kconfigfunctions.py', so I
can't speak to it's limitations or applicability here.

But AFAICT declaring toolchain capabilities could be supported
with the same mechanism that we use to declare board
capabilities. By having a 'Kconfig' file placed within the
toolchain's configuration directory.

That being said, I agree with @nashif that filtering based on
CMakeCache.txt is a useful feature and needs to be included
anyway, but I don't agree that this particular use-case requires
it.

nashif · 2019-02-06T02:48:48Z

About whether Kconfig should be aware of the capabilities of the
toolchain.

IMHO it should. This would simplify the configuration system for
the user as it would allow us to hide options from the user.

and that is why it should not. If I have an application that depends on a feature that is provided by the toolchain, it should not automagically disappear if I used some other or incompatible toolchain. Toolchains should not feed into the the configuration system, an application building with options that are incompatible with the toolchain should just not build.

SebastianBoe · 2019-02-07T11:20:15Z

If I have an application that depends on a feature that is provided by the toolchain, it should not automagically disappear if I used some other or incompatible toolchain.

It won't automatically disappear. It would be present in prj.conf, and would trigger the warning:

warning: SOME_TOOLCHAIN_FEATURE_A (defined at toolchain/Kconfig:9) was assigned the value 'y'
but got the value 'n'.

When Kconfig has matured[0], it will even produce an error.

Both schemes can cause the build to fail, but only one can prevent users from creating invalid configurations in the first place. And the Kconfig scheme fails earlier, which is generally desirable.

nashif · 2019-02-07T14:11:18Z

It won't automatically disappear. It would be present in prj.conf, and would trigger the warning:
warning: SOME_TOOLCHAIN_FEATURE_A (defined at toolchain/Kconfig:9) was assigned the value 'y'
but got the value 'n'.
When Kconfig has matured[0], it will even produce an error.

Both schemes can cause the build to fail, but only one can prevent users from creating invalid configurations in the first place. And the Kconfig scheme fails earlier, which is generally desirable.

We have a fundamental disagreement here. I do not think the toolchain should be in Kconfig at all for various reason explained above. Any toolchain also a custom one should be easy to integrate with Zephyr and with only minimal setup, for example by providing a toolchain file or by making it available in the path, adding Kconfig to the mix will just make this complex and impossible for anyone to build with Zephyr if the toolchain is not well integrated.

Applications should just fail if they need options that are not provided by the toolchain. period.

Having said that, this PR is orthogonal to all of this and does not change anything in Zephyr, it is just a mean for us to have better coverage in testing without complex whitelisting and filtering.

If you have a proposal for introducing Kconfig on the toolchain level, you are welcome to send out an RFC and have a thorough discussion with all those involved. If that goes through, it will only build on top of this PR, there is no conflict at all.

Parse CMakeCache.txt and filter based on variables defined in CMake. The CMakeCache.txt parsing is borrowed from west. Signed-off-by: Anas Nashif <[email protected]>

newlib is not supported with this toolchain, so mark it as such and filter tests based on the variable defined in the toolchain file. Signed-off-by: Anas Nashif <[email protected]>

Do not run with toolchains that do not support newlib. Signed-off-by: Anas Nashif <[email protected]>

SebastianBoe

Will send out an RFC when this re-appears later.

I understand that you want it to be easy for external parties to integrate their toolchains. I hope that if we were to add toolchain information to Kconfig, it would be done in such a way that it would not increase the amount of configuration required for the external party.

My primary concern is not the advanced user that is integrating a new toolchain, but the common user that is using a pre-integrated toolchain.

BTW, in case you missed it, the toolchain variant used is now known to Kconfig and Kconfig is acting differently based on the toolchain (please don't shoot the messenger though, I didn't review this).

https://github.com/zephyrproject-rtos/zephyr/pull/12980/files#diff-ce44be35085646157ff1120a9a32c4a8R32

please re-review

galak · 2019-02-08T04:46:05Z

Reference issue / enhancement #13162

nashif requested review from andrewboie, andyross, jukkar, nniranjhana, pfalcon, SebastianBoe, spoorthik and tbursztyka as code owners January 9, 2019 14:07

nashif mentioned this pull request Jan 9, 2019

x86_64 architecture #9522

Merged

nashif force-pushed the toolchain_cap branch from c56c828 to 7f1b0ca Compare January 9, 2019 14:13

galak reviewed Jan 9, 2019

View reviewed changes

SebastianBoe reviewed Jan 9, 2019

View reviewed changes

zephyrproject-rtos deleted a comment from sigvartmh Jan 9, 2019

nashif force-pushed the toolchain_cap branch from 7f1b0ca to 04d81c3 Compare January 9, 2019 15:27

nashif mentioned this pull request Jan 9, 2019

Filter tests based on toolchain capabilities #12396

Closed

galak previously requested changes Jan 9, 2019

View reviewed changes

andyross approved these changes Jan 9, 2019

View reviewed changes

andrewboie approved these changes Jan 9, 2019

View reviewed changes

nashif force-pushed the toolchain_cap branch from 04d81c3 to 51240c0 Compare January 10, 2019 20:42

nashif added the dev-review To be discussed in dev-review meeting label Feb 6, 2019

nashif force-pushed the toolchain_cap branch from 51240c0 to 040c887 Compare February 6, 2019 14:31

nashif added 3 commits February 7, 2019 09:13

sanitycheck: support filtering based on cmake cache

e198581

Parse CMakeCache.txt and filter based on variables defined in CMake. The CMakeCache.txt parsing is borrowed from west. Signed-off-by: Anas Nashif <[email protected]>

toolchain: espressif: mark newlib support false

510dd9c

newlib is not supported with this toolchain, so mark it as such and filter tests based on the variable defined in the toolchain file. Signed-off-by: Anas Nashif <[email protected]>

samples/tests: filter based on toolchain features

69b265e

Do not run with toolchains that do not support newlib. Signed-off-by: Anas Nashif <[email protected]>

nashif force-pushed the toolchain_cap branch from 040c887 to 69b265e Compare February 7, 2019 14:13

SebastianBoe approved these changes Feb 7, 2019

View reviewed changes

galak mentioned this pull request Feb 8, 2019

Add exposing TOOLCHAIN_HAS_NEWLIB to Kconfig #13162

Closed

galak merged commit 74c5736 into zephyrproject-rtos:master Feb 8, 2019

pabigot removed the dev-review To be discussed in dev-review meeting label Oct 24, 2019

tejlmand mentioned this pull request Aug 24, 2021

Armclang / armlink support in Zephyr - Zephyr CMake linker functions #36140

Merged

7 tasks

This was referenced Jan 17, 2023

kconfig: review proposal on PR#53338 keith-packard/zephyr#1

Closed

Improved toolchain / libc handling #53856

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter tests based on CMake variables. #12393

Filter tests based on CMake variables. #12393

nashif commented Jan 9, 2019 •

edited

Loading

codecov-io commented Jan 9, 2019 •

edited

Loading

galak Jan 9, 2019

SebastianBoe Jan 9, 2019 •

edited

Loading

galak commented Jan 9, 2019

SebastianBoe commented Jan 9, 2019

nashif commented Jan 9, 2019

nashif commented Jan 9, 2019

galak commented Jan 9, 2019

nashif commented Jan 9, 2019

galak commented Jan 9, 2019

galak left a comment

andyross left a comment

nashif commented Jan 10, 2019

galak commented Jan 10, 2019

nashif commented Jan 11, 2019

carlescufi commented Jan 13, 2019

SebastianBoe commented Jan 14, 2019

SebastianBoe commented Jan 14, 2019

nashif commented Feb 6, 2019

SebastianBoe commented Feb 7, 2019

nashif commented Feb 7, 2019

SebastianBoe left a comment

galak commented Feb 8, 2019

Filter tests based on CMake variables. #12393

Filter tests based on CMake variables. #12393

Conversation

nashif commented Jan 9, 2019 • edited Loading

codecov-io commented Jan 9, 2019 • edited Loading

Codecov Report

galak Jan 9, 2019

Choose a reason for hiding this comment

SebastianBoe Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

galak commented Jan 9, 2019

SebastianBoe commented Jan 9, 2019

nashif commented Jan 9, 2019

nashif commented Jan 9, 2019

galak commented Jan 9, 2019

nashif commented Jan 9, 2019

galak commented Jan 9, 2019

galak left a comment

Choose a reason for hiding this comment

andyross left a comment

Choose a reason for hiding this comment

nashif commented Jan 10, 2019

galak commented Jan 10, 2019

nashif commented Jan 11, 2019

carlescufi commented Jan 13, 2019

SebastianBoe commented Jan 14, 2019

SebastianBoe commented Jan 14, 2019

nashif commented Feb 6, 2019

SebastianBoe commented Feb 7, 2019

nashif commented Feb 7, 2019

SebastianBoe left a comment

Choose a reason for hiding this comment

galak commented Feb 8, 2019

nashif commented Jan 9, 2019 •

edited

Loading

codecov-io commented Jan 9, 2019 •

edited

Loading

SebastianBoe Jan 9, 2019 •

edited

Loading