sys/posix: make posix module provide only headers.

[jcarrano]

Contribution description

The build system contains several instances of

 INCLUDES += -I$(RIOTBASE)/sys/posix/include

This is bypassing the module management system, by directly accesing headers without depending on a module. The module is the posix module.

That line is also added when one of the posix_* modules is requested.

According to the docs, the posix module provides headers only, but in reality there is also inet.c.

This patch:

• Moves inet.c into posix_inet, thus leaving posix as a headers-only module.
• Makes posix_* modules depend on posix, thus removing the explicit INCLUDES+=... in sys/Makefile.include.
• Ocurrences of INCLUDES+=... are replaced by an explicit dependency on posix.

Testing procedure

This is a build system change, that should not change any behaviour. If I screwed up the CI should catch any modules that fail to build.

[jcarrano]

@gschorcht Why is this INCLUDES += needed? The "use vfs" should have the headers to be included anyways. Is it some build system weirdness?

[gschorcht]

@jcarrano Honestly? I can not remember anymore. Probably, these includes were needed during the port sometime in between for the compilation before I could solve all dependencies.

[gschorcht]

@jcarrano BTW, I have tested it without this include, it seems to be compilable.

[gschorcht]

@jcarrano Please, let me take a deeper look tomorrow or the day after tomorrow, with command

USEMODULE=esp_now make flash BOARD=esp32-wroom-32 -C examples/gnrc_networking 

I get now compile errors. This is new 😟

[gschorcht]

I could solve this problem. I will provide a pull request which removes these INCLUDES.

[jcarrano]

@gschorcht Same thing here. Why was it necessary to manually edit "INCLUDES"? I want to know if I can take out the INCLUDES += -I$(RIOTBASE)/sys/posix/include

[gschorcht]

@jcarrano Unfortunatly, this is much more tricky. If you take a look at line 8, you will find

INCLUDES  = -I$(RIOTCPU)/$(CPU)/vendor/esp-idf/wpa_supplicant/port/include

verwrites all includes defined by $(RIOTBASE)/Makefile.base. This was necessary to change the order of include pathes. Otherwise, header files from $(RIOTBASE)/sys/include/crypto would be used instead of the header files from $(ESP32_SDK_DIR)/components/wpa_supplicant/include/crypto which are in conflict. Maybe there is a better approach, but I couldn't figure out one in the very complex make structure of RIOT when I was porting RIOT to ESP32.

I'm not really sure, I tried to comment out both include lines from $(RIOTBASE)/sys/. It seems to be compilabe without them. Maybe, there not needed anymore and can be removed.

[gschorcht]

Finally, I figured out what the reason for these INCLUDES was. Let's suppose you try to compile an application for ESP32 with activated WiFi netdev, e.g. the esp_now module, together with the spiffs package with command:

USEMODULE="spiffs esp_now" make BOARD=esp32-wroom-32 -C tests/shell

• Package spiffs depends on modules mtd and vfs.
• If module mtd is used, the board definition includes mtd.h to declare the default MTD device mtd0. This is done in the same way in native and mulle board definitions.
• mtd.h includes vfs.h if module vfs is enabled.
• vfs.h includes sys/statvfs.h which is found in sys/posix/include/sys/statvfs.h.

Because of conflicts between the header files $(ESP32_SDK_DIR)/components/wpa_supplicant/include/crypto used by the ESP32 WiFi modules and $(RIOTBASE)/sys/include/crypto, we have to ensure that the include order is the following:

-I$(ESP32_SDK_DIR)/components/wpa_supplicant/include/crypto
-I$(RIOTBASE)/sys/include/

Unfortunately, I could not find a way to get the include paths of a module in front of RIOT's standard include paths in the INCLUDES variable. Therefore, I had to set INCLUDES for this module from scratch including $(RIOTBASE)/sys/posix/include 😟

Maybe you, @cladmi or @kaspar030 have an idea how to deal with such a case.

[gschorcht]

@jcarrano I could find a workaround provided with PR #10362.

[cladmi]

@gschorcht I have seen this inconsistency a while a go but did not have any applied reason to change it except "it looks weird", now there is one :)
For me it feels like the https://github.com/RIOT-OS/RIOT/blob/master/Makefile.include#L239 line should be after processing board and cpu Makefile.include, even after the toolchain and newlib headers (that basically trick this and overwrites the deferred value of INCLUDES

RIOT/makefiles/libc/newlib.mk

Lines 84 to 89 in 98f42ee

	# Newlib includes should go before GCC includes. This is especially important
	# when using Clang, because Clang will yield compilation errors on some GCC-
	# bundled headers. Clang compatible versions of those headers are already
	# provided by Newlib, so placing this directory first will eliminate those problems.
	# The above problem was observed with LLVM 3.9.1 when building against GCC 6.3.0 headers.
	INCLUDES := $(NEWLIB_INCLUDES) $(INCLUDES)

) even put them first as BSP should go first.

[cladmi]

I just noticed that your issue is for one module makefile and that the header is required for this module only.
For me it is the same case as we have for the NATIVEINCLUDES where implementing the board specific modules require specific includes and do not even need to see RIOT headers.
You basically want to define a ESP32_VENDORMODULE_INCLUDES.

[cladmi]

I already noticed this issue with dependencies on headers only.
I was thinking about having a posix_headers module, but your change looks more consistent with the other posix_ modules.

As these functions are defined in sys/posix/include/arpa/inet.h maybe the module should be more called pthread_arpa.

@miri64 you may have more background on this.

I agree with the direction, just want some more feedback.

[cladmi]

It looks like also more modules should depend on the new module:

dist/tools/tunslip/tunslip6.c:        inet_ntop(p->ai_family, get_in_addr((struct sockaddr *)p->ai_addr),
Binary file pkg/libcoap/patches/0006-debug-do-not-misuse-NDEBUG.patch matches
Binary file pkg/oonf_api/patches/0001-add-RIOT-support.patch matches
sys/net/application_layer/uhcp/uhcp.c:    inet_ntop(AF_INET6, src, addr_str, INET6_ADDRSTRLEN);
sys/net/application_layer/uhcp/uhcp.c:    inet_ntop(AF_INET6, src, addr_str, INET6_ADDRSTRLEN);
sys/net/application_layer/uhcp/uhcp.c:    inet_ntop(AF_INET6, prefix, prefix_str, INET6_ADDRSTRLEN);
sys/net/network_layer/ipv4/addr/ipv4_addr_to_str.c:/* based on inet_ntop4() by Paul Vixie */
sys/net/network_layer/ipv6/addr/ipv6_addr_to_str.c:/* based on inet_ntop6() by Paul Vixie */
sys/net/sock/sock_util.c:    if (!inet_ntop(endpoint->family, addr_ptr, addr_str, INET6_ADDRSTRLEN)) {
sys/posix/include/arpa/inet.h:const char *inet_ntop(int af, const void *restrict src, char *restrict dst,
sys/posix/inet/inet.c:const char *inet_ntop(int af, const void *restrict src, char *restrict dst,
tests/gnrc_sock_dns/main.c:        inet_ntop(res == 4 ? AF_INET : AF_INET6, addr, addrstr, sizeof(addrstr));

[jcarrano]

From your list:

• dist/tools/tunslip/tunslip6.c not a module
• sys/net/application_layer/uhcp/uhcp.c: uhcp module, already depends on posix_inet.
• sys/net/sock/sock_util.c: sock_util, done.
• tests/gnrc_sock_dns/main.c maybe the dependency should be made explicit here.

[miri64]

The change is making sense mostly. Just one dependency is missing (which would make one addition redundant)

[miri64]

Also for clarity, maybe name the module posix_headers? That's also how the section defining them in the POSIX specs is called.

[jcarrano]

Also for clarity, maybe name the module posix_headers? That's also how the section defining them in the POSIX specs is called.

Wouldn't that be too big of an API change?

[miri64]

Wouldn't that be too big of an API change?

Mh isn't it that already... What I see is that you are currently introducing a lot of dependencies on a "new" pseudo-module posix and moving the sources of the existing module posix to a new name (posix_inet). I am not proposing that you do so (or maybe split do it and put the rename to posix_inet in its own commit) but keeping the name posix instead of posix_inet and introducing the posix_headers module would actually be the smaller change.

[jcarrano]

I changed it to posix_headers and squashed.

[miri64]

ACK. Though I agree that the CI should catch all relevant errors if they exist, I sampled some applications, just to be sure:

• examples/ccn-lite-relay
• examples/posix_sockets
• tests/gnrc_sock_dns
• tests/pkg_libcoap