Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: arch: arm: no_multithreading: Limit platforms allowed #34749

Merged

Conversation

nordic-krch
Copy link
Contributor

@nordic-krch nordic-krch commented May 1, 2021

When multithreading is off, kernel source files like sem.c (samphore
implementation) are not present in the build. Some platforms by default
fetch modules or drivers that are using multithreading primitives and
because of that fails to compile when multithreading is off.

Limit the test to only qemu platforms since test is arch specific.

Fixes #34739.

Signed-off-by: Krzysztof Chruscinski krzysztof.chruscinski@nordicsemi.no

When multithreading is off, kernel source files like sem.c (samphore
implementation) are not present in the build. Some platforms by default
fetch modules or drivers that are using multithreading primitives and
because of that fails to compile when multithreading is off.

Limit the test to only qemu platforms since test is arch specific.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
@github-actions github-actions bot added the area: Tests Issues related to a particular existing or missing test label May 1, 2021
Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to run on hardware, we cannot do it like this.

@galak
Copy link
Collaborator

galak commented May 3, 2021

This needs to run on hardware, we cannot do it like this.

Do you expect a fix soon? If not, I'd like to merge this and than we deal with expanding this to run on other platforms as a follow up PR.

@ioannisg
Copy link
Member

ioannisg commented May 3, 2021

This needs to run on hardware, we cannot do it like this.

Do you expect a fix soon? If not, I'd like to merge this and than we deal with expanding this to run on other platforms as a follow up PR.

So let me ask this - we merged changes in the support of CONFIG_MULTITHREADING=n feature, (probably here: #34279) that broke this test, which has been working for quite a long time, i.e. we had support for CONFIG_MULTITHREADING=n on real ARM hardware, and we consider that the fix is to run the test only for QEMU targets?

Looking closely on #34279; it appears to me that this PR has introduced additional tests for the CONFIG_MULTITHREADING=n use-case, but all of them build and run only on QEMU targets, why have we agreed to go this way? Nothing in the description of the PR suggests that from now on, the CONFIG_MULTITHREADING=n feature is only supported on qemu targets, not hardware.

@stephanosio stephanosio requested a review from nashif May 4, 2021 12:06
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some platforms by default
fetch modules or drivers that are using multithreading primitives and
because of that fails to compile when multithreading is off.

In #34739, we are seeing messages like:

libzephyr.a(rtt_console.c.obj): in function `k_mutex_lock':
libzephyr.a(rtt_console.c.obj): in function `k_mutex_unlock':
libzephyr.a(rtt_console.c.obj): in function `k_sleep':

Were these functions undefined/unsupported prior to #34279 when CONFIG_MULTITHREADING=n?

@nordic-krch
Copy link
Contributor Author

Sorry guys, i'm on vacation with limited access to PC (back Thursday) #34279 changed kernel CMakeLists.txt to not include multithreading files when CONFIG_MULTITHREADING=n. Previously, this test was compile and was actually fetching multithreading.

Not sure how to filter ok platforms. I checked that test compiles for nrf boards except boards like Thingy (#34468) which has sensors and uses i2c and spi.

@ioannisg @galak

This needs to run on hardware, we cannot do it like this.

I can add some nrf boards? Others i want be able to test.

Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the right thing to do.
CONFIG_MULTITHREADING=n was de-deprecated, so it needs to run in some set of hardware.
I'll file an issue about this.

@ioannisg ioannisg merged commit d34a289 into zephyrproject-rtos:master May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARM ARM (32-bit) Architecture area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tests/arch/arm/arm_no_multithreading/arch.arm.no_multithreading fails to build on a number of platforms
6 participants