posix: Kconfig: Allow to enable individual components, POSIX_API just enables all

[pfalcon]

llow to enable individual POSIX components, like Pthreads.
CONFIG_POSIX_API now just enables of individual POSIX components, and
sets up environment suitable to easily port POSIX applications to
Zephyr.

Fixes: #12965

Signed-off-by: Paul Sokolovsky [email protected]

[pfalcon]

For context this patch (or similar functionality) is now required e.g. to get cc3220sf_launchxl working, see #18736 (comment).

@vanti: Please give this a try, remove select POSIX_API from Kconfig.simplelink, and see if select PTHREAD_IPC will still have effect (our aim is to make it so). You may need to adjust include paths, etc.

[vanti]

@pfalcon I have verified that http_get builds on cc3220sf_launchxl after fixing some link errors that require this line to be added to lib/CMakeLists.txt:

add_subdirectory_ifdef(CONFIG_PTHREAD_IPC posix)

Could you add this and I'll submit the changes specific to SimpleLink in Kconfig.simplelink? Thanks.

[vanti]

I changed the milestone to 2.0 in hope that we can get this in for the release, so that http_get doesn't stay broken for cc3220sf_launchxl due to #18736 which was recently merged.

[pfalcon]

@vanti

I changed the milestone to 2.0 in hope that we can get this in for the release

I'm happy to try ;-).

that require this line to be added to lib/CMakeLists.txt:
add_subdirectory_ifdef(CONFIG_PTHREAD_IPC posix)

I'm not sure if that may raise questions why the same dir is added by 2 config options to the build list. And it's definitely just a workaround for Pthreads, somebody may want to enable just FS, mqueue, etc.

[pfalcon]

@vanti: As an update: I of course hoped to post cheerful "i fixed it!" much earlier, but instead fighting whole day with the usual suspects: ARCH_POSIX and posix vs newlib headers conflicts. This last build finally seems to be going over the summit, keeping fingers crossed. I'll check it later anyway.

If you'd target this for 2.0, probably add Priority: high, and look for someone else to approve it (2 votes required) ;-). Let me also know if you think it would be helpful to merge #18810 into this one (usually it's not a good idea, but otherwise, I guess it would be easier to get 1 PR into the release than 2.)

[pfalcon]

Ok, indeed it passed. Squashing what's possible together...

[pfalcon]

So, I actually included #18810 here. And bad news is:

/opt/sdk/zephyr-sdk-0.10.3/arm-zephyr-eabi/arm-zephyr-eabi/sys-include/sys/timespec.h:58:8: error: redefinition of 'struct itimerspec'
 struct itimerspec {
        ^~~~~~~~~~
In file included from ../../../../../../include/posix/pthread.h:12,
                 from /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/lib/posix/pthread_cond.c:10:
../../../../../../include/posix/time.h:58:8: note: originally defined here
 struct itimerspec {

So, there's always something else around the corner to fix in the POSIX subsys... And I've seen the issue above, it's actually why samples/net/sockets/echo_async_select wasn't included in #18736, because it exposed it, and I didn't want to risk putting off PR with big'ish patches. I believe I have a fix now, but it needs to be CIed, and it all bloats the scope of what was supposed to be a simple change.

[pfalcon]

I believe I have a fix now

And still there was following:

/home/pfalcon/opt/zephyr-sdk-0.10.3-pr-101/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/8.3.0/../../../../arm-zephyr-eabi/bin/ld: ../modules/ti/simplelink/lib..__modules__hal__ti__simplelink.a(device.c.obj): in function `sl_Stop':
/home/pfalcon/projects-3rdparty/Embedded/Zephyr/modules/hal/ti/simplelink/source/ti/drivers/net/wifi/source/device.c:353: undefined reference to `usleep'
/home/pfalcon/opt/zephyr-sdk-0.10.3-pr-101/arm-zephyr-eabi/bin/../lib/gcc/arm-zephyr-eabi/8.3.0/../../../../arm-zephyr-eabi/bin/ld: ../modules/ti/simplelink/lib..__modules__hal__ti__simplelink.a(cc_pal.c.obj): in function `Semaphore_pend_handle':
/home/pfalcon/projects-3rdparty/Embedded/Zephyr/modules/hal/ti/simplelink/source/ti/drivers/net/wifi/porting/cc_pal.c:414: undefined reference to `clock_gettime'
collect2: error: ld returned 1 exit status

Which I'm fixing by introducing CONFIG_POSIX_CLOCK (which I squash into the very first commit). With that and corresponding update to #18810 (which is again, included here, squashed), cc3220sf_launchxl samples finally build for me.

[jukkar]

LGTM

[pabigot]

My comments are isolated to the header/libc commit.

newlib presumably is re-using <sys/timespec.h> in POSIX configurations, but fundamentally it is a BSD header: POSIX doesn't describe it. I'm not really happy about making minimal libc follow that practice by adding another non-private header that somebody might start including.

Much of the complexity of the POSIX subsystem headers seems to be because it's trying to override and then reconstruct the contents of <time.h>. Now that we have existing demonstrations of #include_next I would like to see the POSIX subsystem try to move there too.

I don't think that needs to be done in this commit, so I've added #18892 for it. If you don't really need that change in this PR in order to solve #12965 I'd rather it not be included.

[pfalcon]

@pabigot

newlib presumably is re-using <sys/timespec.h> in POSIX configurations, but fundamentally it is a BSD header: POSIX doesn't describe it.

Nobody says it does.

I'm not really happy about making minimal libc follow that practice by adding another non-private header that somebody might start including.

I'm sorry, you're bit late with this. We already have established practice of resolving that kind of issue, used for a few headers already, and this patch just follows the practice.

Much of the complexity of the POSIX subsystem headers

I'm not sure what complexity you're talking about. In either case, it solved complexity. The real complexity comes when we're trying to make a revolution on empty grounds, e.g. throw away APIs which were there for years for no good reason. On this side, work is done along the way of establishing solutions for missing things (in sustainable, not revolutionary, manner).

If you don't really need that change in this PR in order to solve #12965 I'd rather it not be included.

I'm not sure what you mean. This PR doesn't contain unneeded changes (or I wouldn't make them). The changes in this PR follow the same pattern as in #16626. Kindly please get acquainted with it if you're interested in maintenance of of POSIX subsystem. (Short summary: try to make one-line change, and see cascade of failures. I'm firefighting since the beginning year to converge it to a stable state, and sparks of instability of still happen, as this PR shows. From that, you may imagine why I'm not exactly fancy suggestions of passers-by along the lines of "just splash petroleum into it, it will surely put out fire!").

[pabigot]

sigh.

The real complexity comes when we're trying to make a revolution on empty grounds, e.g. throw away APIs which were there for years for no good reason.

There's a very good reason: a large number of Zephyr target applications have no need for POSIX APIs.

You do, and that's fine. My intent in #18892 is to simplify your task by suggesting a way to re-use the existing declarations from the underlying libc without having to jump through hoops trying to reconstruct what's already available (minimal libc by design excludes some features, but newlib does not).

I don't really care how you maintain the POSIX system, though the need for #17706 suggests there's a wider concern about it from a Zephyr project perspective. I do care about libc. If you need this hack to have something that works with both newlib and minimal libc go ahead--I'm not blocking this PR.

But if/when you get the POSIX subsystem include files a little less ad hoc the minimal libc declaration of interval timer data structures should move directly into <time.h> where it belongs.

[pfalcon]

sigh.

To not sigh in response, let me just ask a simple question: which was the last POSIX application you ported to Zephyr?

My intent in #18892 is to simplify your task by suggesting

Keeping up with simple questions: which POSIX applications have you tested it with? And may I immediately suggest that to be sure about it, it would be nice to test it with 10-20 more?

There's a very good reason: a large number of Zephyr target applications have no need for POSIX APIs.
If you need this hack

Here we again differ in our perception: I'm working on establishing best practices and interfaces/contracts on how to integrate different Zephyr subsystems (e.g. a libc (which we have multiple!) and POSIX subsys), and suspect your idea of being a premature optimization. While you suggest that the convention being established is a hack, and suggest that hammer you used elsewhere most definitely applies here too, because it all nails. Let the number of ported (and still working!) apps speak, that should be the best way to convince each other (well, me definitely).

But if/when you get the POSIX subsystem include files a little less ad hoc the minimal libc declaration of interval timer data structures should move directly into <time.h> where it belongs.

What you're suggesting is that minimal libc should be treated in adhoc way. What I'm suggesting that there should be conventions which libc's (any libc!) should follow to integrate with the rest of Zephyr (e.g. POSIX subsys) seamlessly. Indeed, let's see how it goes.

I'm not blocking this PR.

Most importantly, thanks for this. And what I'm trying to emphasize in my replies, that there should be more formal and general criteria for reviewing patches than "I like/don't like".

[nashif]

agree with @pabigot

I'm sorry, you're bit late with this. We already have established practice of resolving that kind of issue, used for a few headers already, and this patch just follows the practice.

It is not late and it is definitely not an established practice. just because you managed to introduce a bug does not mean it should be seen as an established practice. It should be fixed instead.

[pfalcon]

@nashif

agree with @pabigot

Agree with what exactly? As my notifications show, ten minutes before posting this comment, you have posted following comment on the factored-out issue, #18892: "while include_next was used for posix subsys already, I think we should avoid it.". So, in fact, you do not agree with @pabigot. Unless you kindly tell exactly what you agree with (this and other times, thanks in advance!).

It is not late and it is definitely not an established practice.

This pattern is used 3rd time (and yes, it's not supposed to be used too often, only exactly when needed and not a single case beyond that, which are few).

just because you managed to introduce a bug does not mean it should be seen as an established practice.

Excuse me? What are you talking about, can you kindly elaborate again? If I introduced a bug somewhere, please open a separate ticket with relevant details, we'll get it triaged and fixed. This particular PR fixes a bug, using the same pattern as used a few times before. And no bugs are known to be introduced, unless you can pinpoint the exact location (please use inline code comments). Thanks!

[vanti]

@pfalcon @nashif @pabigot I understand there is some disagreement in terms of the direction where we wish to take minimal libc. Is this something that can be hashed out separately as @pabigot suggested? A sample on cc3220sf_launchxl is broken due to the changes from #18736, and it'd be good to at least fix that by merging this PR to unblock the end users. Fixing #12965 is also a good incremental improvement.

[pfalcon]

@vanti: I fully agree. And this PR fixes matters not only for cc3220sf_launchxl, careful reviewer of #18736 would not notice that I had to skip the echo_async_select sample, because it hit the very same issue fixed here.

Again, this PR does everything in the same way as was done a few times before - reviewed, discussed, approved. It fixes one more missed previously case. We need to merge it, enable missing CI cases, and with better CI coverage, process any further ideas.

[pfalcon]

Rebased and fixed conflicts otherwise. Re-requested review from @nashif .

[pabigot]

Added to dev-review to discuss whether this can proceed despite a change request.

No clear direction on what to do. Please provide more concrete change requests