lib: posix: Switch to use zephyr_interface_library_named cmake directive #15937

pfalcon · 2019-05-06T14:22:45Z

Similar to how other sub-libraries are defined in Zephyr tree, e.g.
"fs", "lgvl", etc. This is supposed to help with the need to
explicitly add posix include path to each and every application using
POSIX subsys.

Fixes: #15627

SebastianBoe · 2019-05-06T14:32:28Z

We are also going to need a APP_LINK_WITH_PTHREAD Kconfig option as shown here:

4c60c51

This is to allow the user to opt-out of this feature.

pfalcon · 2019-05-06T14:34:38Z

These changes are for #15627, and currently don't work as expected (and thus, should not be merged). Discussion of the issue with these changes are at: #15627 (comment)

pfalcon · 2019-05-06T14:36:59Z

We are also going to need a APP_LINK_WITH_PTHREAD Kconfig option as shown here:

So, current issue is that it simply doesn't work as expected. So, let's leave discussion of opt-out for later. (It would start with the fact that there's opt-in with CONFIG_POSIX_API in the first place.)

SebastianBoe · 2019-05-06T14:40:10Z

Sorry, I meant the design-decision for why this option pattern exists is to allow the user to opt-out of the feature.

The feature itself does require the option to be present for it to at all work.

This is documented in the usage documentation of the function here:

https://github.com/zephyrproject-rtos/zephyr/blob/master/cmake/extensions.cmake#L485

I see now that the wrong word is used in the documentation, it says "should", but should say "must".

pfalcon · 2019-05-06T16:33:12Z

The feature itself does require the option to be present for it to at all work.

Thanks, that helped, but I now see include order conflicts between libc vs POSIX headers (before they were in right order, now they're in wrong). Investigating...

pfalcon · 2019-05-07T16:29:17Z

@PiotrZierhoffer, @tgorochowik: This is an example of trying to improve a little bit build process of POSIX apps.

SebastianBoe · 2019-06-07T08:42:12Z

tests/posix/common/CMakeLists.txt

Code should not be commented out.

Fixed, and also removed this directive from the other posix test.

galak · 2019-06-17T16:53:44Z

Sorry for taking a while to review, can you rebase and resolve the conflicts.

nashif · 2019-06-17T20:10:23Z

lib/posix/Kconfig

should be APP_LINK_WITH_POSIX

should be APP_LINK_WITH_POSIX

No, as explained by @SebastianBoe, this should match the existing config symbols, that's why it's APP_LINK_WITH_PTHREAD.

which existing config symbols? If anything is still using PTHREAD, change it POSIX as well.

If anything is still using PTHREAD, change it POSIX as well.

But this is not subject of this change. The subject of this change is "lib: posix: Switch to use zephyr_interface_library_named cmake directive".

Exploration of more deep changes to POSIX subsys was done in e.g. #12984 . Feel free to merge it if you like it.

But this is not subject of this change. The subject of this change is "lib: posix: Switch to use zephyr_interface_library_named cmake directive".

it becomes a subject of this change when you introduce a new Kconfig using the old name and we will be stuck with it.

we will be stuck with it.

Why would we be stuck with? We'll change it (and hundreds of other mistakes and bugs in Zephyr) when it comes to it.

All in all, I'll defer to @galak, who assigned me to work on POSIX subsys improvements, regarding whether that should be part of this PR or not. In the meantime, working on other assignment from him, and can't work further on this, beyond keeping the build green.

pfalcon · 2019-06-18T17:53:23Z

@galak

can you rebase and resolve the conflicts.

Done, build is green.

pfalcon · 2019-07-01T18:57:00Z

Rebased on the latest master.

Historically, it used to be "PTHREAD", which is no longer true, as POSIX subsys offers much more functionality than just Pthreads. Use detailed name, like "posix_subsys", to avoid possible confusion with ARCH_POSIX-related matters. Signed-off-by: Paul Sokolovsky <[email protected]>

To let any other zephyr_system_include_directories() directives act first and get these paths in an order allowing Zephyr's stuff override compiler headers. Signed-off-by: Paul Sokolovsky <[email protected]>

This is consistent with how newlib headers are treated, and will have effect of ninlibc headers to be further down in the include order. This is important, because some POSIX subsys headers override those of libc. Without this change, we can't streamline POSIX build config using zephyr_interface_library_named() cmake directive, because includes will be in wrong order. Signed-off-by: Paul Sokolovsky <[email protected]>

Similar to how other sub-libraries are defined in Zephyr tree, e.g. "fs", "lgvl", etc. This is supposed to help with the need to explicitly add posix include path to each and every application using POSIX subsys. Fixes: zephyrproject-rtos#15627 Signed-off-by: Paul Sokolovsky <[email protected]>

This is no longer needed after using zephyr_interface_library_named cmake directive in the POSIX subsys source itself. Signed-off-by: Paul Sokolovsky <[email protected]>

pfalcon · 2019-08-08T11:21:30Z

should be APP_LINK_WITH_POSIX

Ok, made this PR start with a patch to rename CMake library PTHREAD -> posix_subsys.

Rebased on master, CI passes. @nashif, as your feedback is addressed, and I'm not sure if you may be on vacation or away, dismissing your review requesting the rename.

Request CMake library rename is integrated.

pfalcon · 2019-08-08T11:23:06Z

@carlescufi, @galak, @ioannisg: Please consider merging this long-waiting, long-approved PR.

ioannisg · 2019-08-08T11:25:01Z

@carlescufi, @galak, @ioannisg: Please consider merging this long-waiting, long-approved PR.

Thanks. No rush on this, anyways (it's a bug fix and can be done after feature freeze)

pfalcon added area: Build System area: POSIX POSIX API Library labels May 6, 2019

pfalcon requested a review from SebastianBoe May 6, 2019 14:22

pfalcon assigned SebastianBoe May 6, 2019

pfalcon mentioned this pull request May 6, 2019

Application compiled with CONFIG_POSIX_API doesn't have access to POSIX headers #15627

Closed

pfalcon force-pushed the posix-zephyr_interface_library branch from de40e1a to 4a51b7e Compare May 6, 2019 16:35

pfalcon force-pushed the posix-zephyr_interface_library branch from 4a51b7e to a0bd9ee Compare June 6, 2019 14:32

pfalcon requested review from andrewboie and nashif as code owners June 6, 2019 14:32

pfalcon changed the title ~~[DNM] lib: posix: Switch to use zephyr_interface_library_named cmake directive~~ lib: posix: Switch to use zephyr_interface_library_named cmake directive Jun 6, 2019

SebastianBoe reviewed Jun 7, 2019

View reviewed changes

pfalcon force-pushed the posix-zephyr_interface_library branch from db70f68 to 2380a2a Compare June 7, 2019 09:46

This was referenced Jun 7, 2019

[DNM][brokencase] Posix zephyr interface library broken inc order #16679

Closed

[RFC] Missing parts of libc required for CivetWeb #16683

Closed

SebastianBoe approved these changes Jun 11, 2019

View reviewed changes

galak approved these changes Jun 17, 2019

View reviewed changes

andrewboie approved these changes Jun 17, 2019

View reviewed changes

nashif previously requested changes Jun 17, 2019

View reviewed changes

pfalcon force-pushed the posix-zephyr_interface_library branch from 2380a2a to df4176f Compare June 18, 2019 09:48

zephyrbot added area: C Library C Standard Library area: Tests Issues related to a particular existing or missing test labels Jun 18, 2019

pfalcon force-pushed the posix-zephyr_interface_library branch from df4176f to 5c38c0f Compare June 18, 2019 16:24

pfalcon mentioned this pull request Jun 19, 2019

Boost default tick rate to double digit kHz on tickless hardware #16782

Merged

pfalcon force-pushed the posix-zephyr_interface_library branch from 5c38c0f to 20da625 Compare July 1, 2019 18:56

pfalcon mentioned this pull request Jul 22, 2019

[RFC] Proposed development plan for Zephyr's POSIX subsystem #17706

Closed

pfalcon force-pushed the posix-zephyr_interface_library branch from 20da625 to 0880977 Compare August 8, 2019 06:30

pfalcon added 4 commits August 8, 2019 12:08

CMakeLists.txt: Call toolchain_cc_nostdinc() at the end

b94ef4d

To let any other zephyr_system_include_directories() directives act first and get these paths in an order allowing Zephyr's stuff override compiler headers. Signed-off-by: Paul Sokolovsky <[email protected]>

tests: posix: No longer use target_include_directories

a40cc0d

This is no longer needed after using zephyr_interface_library_named cmake directive in the POSIX subsys source itself. Signed-off-by: Paul Sokolovsky <[email protected]>

pfalcon force-pushed the posix-zephyr_interface_library branch from 0880977 to a40cc0d Compare August 8, 2019 09:14

ioannisg added the bug The issue is a bug, or the PR is fixing a bug label Aug 8, 2019

pfalcon mentioned this pull request Aug 8, 2019

lib: posix: Use "posix_subsys" as the CMake lib for the subsystem. #18087

Closed

carlescufi approved these changes Aug 8, 2019

View reviewed changes

carlescufi merged commit 16c1db9 into zephyrproject-rtos:master Aug 8, 2019

pabigot mentioned this pull request Aug 14, 2019

rework newlib directory prioritization to fix include processing for non-Zephyr toolchains #18242

Merged

lib: posix: Switch to use zephyr_interface_library_named cmake directive #15937

lib: posix: Switch to use zephyr_interface_library_named cmake directive #15937

Uh oh!

Conversation

pfalcon commented May 6, 2019

Uh oh!

SebastianBoe commented May 6, 2019

Uh oh!

pfalcon commented May 6, 2019

Uh oh!

pfalcon commented May 6, 2019

Uh oh!

SebastianBoe commented May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfalcon commented May 6, 2019

Uh oh!

pfalcon commented May 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galak commented Jun 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfalcon commented Jun 18, 2019

Uh oh!

pfalcon commented Jul 1, 2019

Uh oh!

pfalcon commented Aug 8, 2019

Uh oh!

pfalcon commented Aug 8, 2019

Uh oh!

ioannisg commented Aug 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

SebastianBoe commented May 6, 2019 •

edited

Loading