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

Tidying up the make hierarchy #2839

Closed
TerryE opened this issue Jul 21, 2019 · 9 comments
Closed

Tidying up the make hierarchy #2839

TerryE opened this issue Jul 21, 2019 · 9 comments
Labels

Comments

@TerryE
Copy link
Collaborator

TerryE commented Jul 21, 2019

This is a component of #2816 and relates to #2811

The current make hierarchy is a sloppy mess which I want to clean up but staying within the current make framework. If we look at a typical compile line generated in the make

Type Option
Compiler Warnings -Wimplicit -Wpointer-arith -Wundef -Werror
Optimisation stuff -O2 -ffunction-sections -fno-jump-tables -fdata-sections -fno-inline-functions -mlongcalls -mtext-section-literals
Language Spec -std=gnu11
Linker stuff -Wl,-EL -nostdlib
Preprocesssor defines `__ets__` `EBUF_LWIP` `ICACHE_FLASH` `LWIP_OPEN_SRC` `MBEDTLS_USER_CONFIG_FILE="user_mbedtls.h"` MEM_DEFAULT_USE_DRAM PBUF_RSV_FOR_WLAN USE_OPTIMIZE_PRINTF ``
Type Include List (not nec in this order)
Dirs don't exist / contain headers <root>/ <root>/include ../smart ../../include/eagle
Defaults ./ include ../include``../../include
Core stuff ../libc ../lua ../platform
Why expose? ../spiffs ../fatfs
Packages ../coap ../dht ../http ../mqtt ../pcm ../pm ../sjson ../sqlite3 ../u8g2lib/u8g2/src/clib ../ucglib/ucg/src/clib ../websocket

This all need cleaning up:

  • The warnings and optimisation stuff are fine, except that we will be moving directories to -Wall -Wextra over time.
  • Defines should be moved into a config header file or down into the sub make that needs them
  • Pathing in directories that don't exist or don't validly contain includes should be removed
  • We have some standard roots: ROOTDIR APPDIR LUADIR INCDIR
  • Most of the package includes are used in very specific instances so there is no point in including app/http (for example) in the search list; the one reference in app/http/httpclient.c should simply refer to #include "http/httpclient.h" instead.

This is all pretty mechanistic tidying. I just don't want any hard wired reference to app or app/lua in any Makefiles.

@jmattsson
Copy link
Member

Genuine question, is it worth spending time on this, or would we be better off doing it as part of moving to something that can be used together with the IDF? The answer might be "yes", in that we get cleaner code before we're co-existing with the (much stricter) IDF build. Or it might be no, in that it ends up being double-work.

I'm still interested in seeing if I (or someone else) can fit the non-OS SDK into an IDF framework, since that would mean the least amount of on-going maintenance between the 8266/32 as the config methods would be shared, as would directory structure and make framework.

@nwf
Copy link
Member

nwf commented Jul 22, 2019

As someone who generally thinks make is complicated and overrated... how essential is it that we use make specifically? Do the Espressif toolkits more or less require the use of make or is it straightfoward to point other tools at their files and scripts?

@jmattsson
Copy link
Member

make is wonderful, once you've managed to become its friend, but I'll grant you it takes a while and some investment. It's effectively functional programming around dependencies.

Espressif has historically only supported make, but with the IDF they're also moving towards cmake support (and that's a tool which to me is both overrated and mis-named - it does not provide dependency management like the other makes do (make, pmake, gmake, heck even Microsoft's nmake); it's glorified macro'ing imnsho).

@TerryE
Copy link
Collaborator Author

TerryE commented Jul 22, 2019

I'll post the PR later. You will see that it's nearly all tidying up crap introduced by contributors that didn't understand how the callback make system worked. I've removed the hardwired /app/ stuff as well just to make aligning to the IDF directory structure a lot easier as well.

@cwrseck
Copy link

cwrseck commented Jul 22, 2019

@TerryE I'm glad Wall Wextra are getting a look-in. Is it worth moving from gnu11 to C11? Particularly in embedded stuff I tend to use:

CFLAGS = -std=c11 -pedantic -Wall -Wextra -Wstrict-prototypes-initializers -Wconversion

but that may be overkill. Wconversion certainly produces a lot of errors, but every so often it turns up something that will bite you.
Nice to see the code cleaned up, though. I gave up on Arduino libraries after compiling them with the flags set out above - I think I got 1300 or so warnings.

C W Rose
(And please stick with Make if possible - it's the one build system, afaik, that's universally available.)

@TerryE TerryE mentioned this issue Jul 22, 2019
3 tasks
@TerryE
Copy link
Collaborator Author

TerryE commented Jul 22, 2019

There are still some botches remaining:

  • <root>/Makefile pre-build target compiles a certificate at <root>server-ca.crt into app/modules/server-ca.crt.h This is a cludge. The app/modules uses this to compile tls.c, if the top level -D define HAVE_SSL_SERVER_CRT points to it. This stuff if we really have to keep it should be in the app/modules/Makefile where it belongs.
  • various host SPIFFS tools make cross references to app/spiffs files. We probable need to keep these, but I've now added a symbol to do this reference.
  • We really cludge the include list order because our forked LwIP implementation is now different from the Espressif maintained 3rd party source and we need our version of espconn.h to be linked instead of the sdk one. IMO, we should just do a symlink in sdk_overrides/espconn.h and use our standard override mechanism instead of jumping through hoops to do this by slotting in an app/includesdirectory between sdk_overrides and the SDK

@TerryE
Copy link
Collaborator Author

TerryE commented Jul 22, 2019

@TerryE I'm glad Wall Wextra are getting a look-in. I ... think I got 1300 or so warnings.

It is really a chore doing this, so it needs tackling directory by directory. If the errors were all just a case of being binary-neutral source code fixes, e.g. an int return that is always ignored being changed to a void, but just occasionally you find one that is an error and needs further exploration in code that you weren't the maintainer for.

@TerryE
Copy link
Collaborator Author

TerryE commented Jul 22, 2019

Genuine question, is it worth spending time on this, or would we be better off doing it as part of moving to something that can be used together with the IDF? The answer might be "yes", in that we get cleaner code before we're co-existing with the (much stricter) IDF build.

@jmattsson, this is worth thinking about, but let's do it downstream. Sorting out mess now is good. The tidier the current make hierarchy perhaps the easier to move to the IDF. But let's not even spend too much time one this just yet.

@jmattsson
Copy link
Member

Having slept on it, I'm all for you doing this clean-up now. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants