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

Modularise makefiles #1724

Merged
merged 45 commits into from
Jun 30, 2019
Merged

Conversation

mikee47
Copy link
Contributor

@mikee47 mikee47 commented Jun 19, 2019

Further to #1684 (comment) et. al. I'm opening this PR now so we can discuss details away from CMake. The main issues addressed by this PR are:

  1. Improve the user experience when building so that dependencies and rebuilding are handled automatically. No swapping between project directory and Sming directory.
  2. Make parallel building work without issue
  3. Improve maintainability and portability by moving the build logic out of the main makefiles and into the related Components.
  4. Build out of source tree
  5. Effectively document the build system
  6. Only pull in external sources required for the project being built. There are quite a number of submodules, etc. and that is only likely to grow.
  7. As far as possible, keep Components independent and avoid 'upward dependencies'. For example, a driver Component shouldn't use any framework definitions like String, etc.

All of this should have no bearing on which build system is actually used (make / CMake).

Further details can be found in the building.md file which has been updated as part of this PR.


New build system

  • Drive build entirely from project directory
  • Build out of source tree, put all generated files under out directory
  • Build Component libraries with clib- prefix for a simpler linker script
  • All Components built using sub-make, including application (App) library. Final linker stage and custom targets are still built by top-level makefile (now project.mk)
  • Re-build Components as required using variants based on hash of configuration variables
  • All Components, including the project, are configured using an optional component.mk file
  • Projects using Arduino libraries must specify via ARDUINO_LIBRARIES variable in component.mk. Note that sub-dependencies are handled automatically (via library's component.mk file and Libraries/components.mk).
  • Write build config to file with target binary file(s) on successful build

Cache configuration variables

  • Changes to variables via make command line override project settings and will be remembered for next make
  • Variables are cached separately for each release/debug build type, and by arch
  • Cache is not affected by 'clean' or 'rebuild' targets

make changes

  • Rename user-lib-clean target to components-clean
  • Rename spiff_update and spiff_clean targets to spiffs-image-update and spiffs-image-clean
  • Add COM_PORT_ESPTOOL variable for use with Esp8266 flash targets (defaults to COM_PORT)
  • Add COM_PORT_GDB variable for use with make gdb (defaults to COM_PORT)
  • Use SPIFF_BIN as config input variable, but retain SPIFF_BIN_OUT as resulting file path
  • Revise list-config target to show current config variables
  • Add config-clean target to wipe cached values and revert to project defaults
  • Add list-components target to show details of Components (use V=1 for details)
  • Add telnet target to Host arch

Use absolute file paths with IMPORT_FSTR

  • Projects are no longer built in source directory, so relative paths don't work - better to use absolute paths anywa
  • Add PROJECT_DIR and SMING_HOME pre-processor definitions for code use

Host UART emulation

  • If no ports are enabled then redirect UART0 output to console (for simple applications and test applications avoids the need to run telnet to see debug output)

Bugs fixed:

  • Crashes if Host LWIP initialisation fails: Don't call host_lwip_service() unless initialisation succeeded

samples and tests

  • Add Eclipse project files to samples where missing
  • Change to new Makefile - component.mk replaces Makefile-user.mk
  • Simplify component.mk files, only include options which change defaults or relate to a specific feature of the sample
  • Use Basic_Blink and Basic_rboot samples to detail available options
  • Add ARDUINO_LIBRARIES definition where required
  • Add SharedComponent test

Integration testing

  • All samples and tests moved out of Sming into a 'projects' directory before building to ensure there are no relative references to Sming, and that submodule fetching works as intended

@slaff slaff added this to the 3.9.0 milestone Jun 19, 2019
@slaff
Copy link
Contributor

slaff commented Jun 20, 2019

I tried to build the Basic_Blink sample using the following commands:

cd $SMING_HOME
make dist-clean
cd ../samples/Basic_Blink
make 

That did not work for me and ended up with the following error:

Building  /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming//out/Esp8266/debug/lib/clib-ArduinoJson.a
AR /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming//out/Esp8266/debug/lib/clib-ArduinoJson.a

/home/slavey/dev/esp8266.dev.box/dev/Sming/Sming//component.mk:23: ! SSL support is not enabled. To enable it type: 'make clean; make ENABLE_SSL=1'
Building  /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming//out/Esp8266/debug/lib/clib-sming.a
C+ /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/Arch/Esp8266/Core/HardwarePWM.cpp
C+ /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/Arch/Esp8266/Core/SPISoft.cpp
C+ /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/Arch/Esp8266/Core/ESP8266EX.cpp
C+ /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/Arch/Esp8266/Core/Interrupts.cpp
C+ /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/Arch/Esp8266/Core/core_esp8266_si2c.cpp
C+ /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/Arch/Esp8266/Core/Digital.cpp
C+ /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/Arch/Esp8266/Core/HardwareTimer.cpp
C+ /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/Arch/Esp8266/Core/SPI.cpp
In file included from /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/Core/Network/HttpClient.h:22:0,
                 from /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/Arch/Esp8266/Core/Network/rBootHttpUpdate.h:19,
                 from /home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/Arch/Esp8266/Core/Network/rBootHttpUpdate.cpp:16:
/home/slavey/dev/esp8266.dev.box/dev/Sming/Sming/Core/Network/Http/HttpCommon.h:30:37: fatal error: http-parser/http_parser.h: No such file or directory
 #include "http-parser/http_parser.h"
                                     ^
compilation terminated.

git shows that the http-parser subodule is not fetched

git submodule 
 6b2fd4c4615d66cbd5e7e787b0b707230b451146 ../../Sming/Arch/Esp8266/Components/axtls-8266/axtls-8266 (heads/master)
...
-edeedb1b4d2f34e4c7d8045ac8b92adbc35e7ed7 ../../Sming/Components/http-parser
...
-a7d4770dd7eb7436c17a7c843a8b36be055c05db ../../docs/wiki

@mikee47 Can you check if you encounter the same issue?

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 20, 2019

I've re-checked this under Linux and works OK for me. You should get a list of Fetching submodule... statements right at the start. Could you try git submodule deinit --all see if that has any effect?

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 20, 2019

Does your SMING_HOME path have a / at the end? I just tried doing that and get the same error. I'll add a check for that in the makefiles.

@slaff
Copy link
Contributor

slaff commented Jun 20, 2019

Does your SMING_HOME path have a / at the end?

I tried using brand new clone of the repository in completely different direction and it worked. For the repo where it worked I did not have trailing slash - /tmp/Sming/Sming. After that I have added a trailing slash - /tmp/Sming/Sming/, make dist-clean and make again and this time it failed with the same error.

So I guess the trailing slash is the one causing the problem.

@slaff
Copy link
Contributor

slaff commented Jun 20, 2019

Second thing: (Re)-compiling libraries with different options.
I have compiled Basic_Blink with the default settings. After that set ENABLE_LWIPDEBUG=1 and was expecting that the build system won't use the already compiled library but will create a new one. But that did not happen:

Building  /path/to/Sming/Sming/out/Esp8266/debug/lib/clib-lwip_open.a
make[1]: Nothing to be done for 'build'.

Wouldn't it be better to calculate a hashsum based on all directives used to compile a library and add that hashsum to the name of the compiled library file?

@slaff
Copy link
Contributor

slaff commented Jun 20, 2019

Third thing: Please, run dos2unix.

## Configure flash parameters (for ESP12-E and other new boards):^M
# SPI_MODE = dio^M
^M
## SPIFFS options^M
DISABLE_SPIFFS = 1^M
# SPIFF_FILES = files^M

@slaff
Copy link
Contributor

slaff commented Jun 20, 2019

More: The Basic_Ssl sample does not compile when using clean source.

/path/to/Sming/Sming/Core/Network/TcpConnection.h:19:44: fatal error: axtls-8266/compat/lwipr_compat.h: No such file or directory
 #include "axtls-8266/compat/lwipr_compat.h"

SMING_HOME does not have trailing slash. I am compiling it using the following commands:

cd $SMING_HOME/../samples/Basic_Ssl
make dist-clean
make

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 20, 2019

Re. Basic_Ssl, I haven't added SSL support in for Host yet, I was intending to deal with that after this PR.

$(info )
$(info Fetching submodule '$(dir $@)' ...)
$(Q) mkdir -p $(dir $@)
$(Q) $(GIT) submodule update --init --force --recursive $(dir $@)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not work when the application code is outside of the sming framework folder. One simple fix would be to replace the command above with:

$(Q) (cd $(SMING_HOME) && $(GIT) submodule update --init --force --recursive $(dir $@))

Copy link
Contributor

Choose a reason for hiding this comment

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

You have changed the line to something different but it still does not work.

cd /path/to/Sming/Sming/Components/spiffs/spiffs/ && git submodule --quiet update --init --force --recursive .
error: pathspec '.' did not match any file(s) known to git.

Please, use the command line that I have suggested. Also update the travis test to copy one of the samples to a folder which is outside of the Sming worktree. This way we should be able to notice issues with the build mechanism.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I want to do here is support Component repositories which don't live in the Sming repo, hence not using cd $(SMING_HOME). I'm guessing it's the missing () subshell bit which fails, so does this work for you?

$(Q) (cd $(dir $@) && $(GIT) submodule --quiet update --init --force --recursive .)

Copy link
Contributor

@slaff slaff Jun 21, 2019

Choose a reason for hiding this comment

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

What I want to do here is support Component repositories which don't live in the Sming repo,

Ok, that makes sense. I would love to have that working. But at the moment you are already using $(SMING_HOME) in that code block and the block is SMING_HOME only.

	$(info Fetching submodule '$(patsubst $(SMING_HOME)/%,%,$(dir $@))' ...)
	$(Q) mkdir -p $(dir $@)
	$(Q) cd $(dir $@) && $(GIT) submodule --quiet update --init --force --recursive .

Which means that line 1 also needs to be updated. So for me it seemed like that code block is SMING framework only. And does not include components which are outside of Sming.

so does this work for you?

No, that did not work either.

@slaff
Copy link
Contributor

slaff commented Jun 20, 2019

Re. Basic_Ssl, I haven't added SSL support in for Host yet, I was intending to deal with that after this PR.

I was not able to compile it using the default settings. The "Host" architecture was not involved here. But it might be another small quirk on my system.

DEBUG_VARS
- APP_LIBDIR = out/Esp8266/debug/lib
- ARCH_BASE = /path/to/Sming/Sming/Arch/Esp8266
- BUILD_BASE = out/Esp8266/debug/build
- FW_BASE = out/Esp8266/debug/firmware
- OS = 
- SMING_ARCH = Esp8266
- SMING_HOME = /path/to/Sming/Sming
- TOOLS_BASE = /path/to/Sming/Sming/out/Esp8266/debug/tools
- UNAME = Linux
- USER_LIBDIR = /path/to/Sming/Sming/out/Esp8266/debug/lib

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 20, 2019

OK, just a few things to sort then! Thanks for taking a look slaff.

@slaff
Copy link
Contributor

slaff commented Jun 20, 2019

OK, just a few things to sort then! Thanks for taking a look slaff.

We should provide also information how to migrate to the new modular system.

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 20, 2019

@slaff I've cloned a fresh worktree to /path/to/Sming, setup SMING_HOME and it builds fine. Could you dump the full list-config from Basic_Ssl?

@slaff
Copy link
Contributor

slaff commented Jun 20, 2019

setup SMING_HOME and it builds fine.

With the suggested change:

$(Q) (cd $(SMING_HOME) && $(GIT) submodule update --init --force --recursive $(dir $@))

that problem is gone for me.

@slaff
Copy link
Contributor

slaff commented Jun 21, 2019

@mikee47 I saw that you have added improvements to your dev/modmake-test. Can you add those changes also to your update/modularise_makefiles so that they appear in this PR and can be tested from others?

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 21, 2019

Yep, just needs a little tidying.

@slaff
Copy link
Contributor

slaff commented Jun 21, 2019

Overall the new build system looks quite promising and exciting. @mikee47 Thanks for your hard work.
There will be often a question asking how to add a new library to an application that is not part of the framework? Let's say we use a simple C library that compiles on all platforms and comes with its own Makefile. And has variants based on the ENABLE_THIS directive. How to add this library to the build mechanism ? How also to rename it to clib-.a so that its code uses the default linker settings?

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 21, 2019

One issue I've encountered - but unable to pin down - is using high figures for parallel building. CI seems especially senstive to this, e.g. https://travis-ci.org/mikee47/Sming/jobs/547897873. Sticking to figures <= (cores + 1) seems fine though, so -j3 is the max. for CI. Cannot get it to break on Windows or Linux (VM) builds though...

Error is invariably 'no such file or directory' error during generation of the dependency files (.c.d, .cpp.d),
I cannot see any obvious flaws in the makefile dependencies, and have tried changing to -MF instead of -o and making sure paths are identical (not just logically the same). The mkdir statements also appear in debug logs ahead of the build. Any thoughts welcome!

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 21, 2019

@slaff Thanks. Have you tried parallel building? In my Linux VM I now get Basic_Ssl to build in 26 seconds from a dist-clean. By comparison, Windows takes 1:46, and 36 seconds just to fetch and patch the submodules... yikes.

...how to add a new library to an application that is not part of the framework?

With the change to hashed library builds we're now telling the Component where to build it's library and what name to use, which should help. If you have a particular library in mind I could use that to work up an example?

@slaff
Copy link
Contributor

slaff commented Jun 21, 2019

If you have a particular library in mind I could use that to work up an example?

That would be great! You can get the source code of the JerryScript library from here: https://github.com/attachix/ukit-esp8266-firmware/tree/master/lib/jerryscript .

And this is how I used to compile up to now: https://github.com/attachix/ukit-esp8266-firmware/blob/master/Makefile#L84

@slaff
Copy link
Contributor

slaff commented Jun 21, 2019

Have you tried parallel building?

Yep, works quite nicely for me. I like this PR a lot. It's definitely an improvement in terms of compilation speed and modularity. People will have to get used to it at first but the advantages overcome any disadvantages.

So for now at least for me there is one showstopper and that is the fetching of Sming Components. Either use the solution that I have proposed or make the code work also with components outside the Sming repo.

@slaff
Copy link
Contributor

slaff commented Jun 21, 2019

One small suggestion for correction:

For consistency with the other targets, please, rename the spiff_update and spiff_clean targets to spiff-update and spiff-clean.

Usage:
  make <target>

Testing
  otaserver          Launch a simple python HTTP server for testing OTA updates

Building
  components         Build all Component (user) libraries
  rebuild            Re-build application

Cleaning
  dist-clean         Clean everything (all arch/build types)
  spiff_clean        Remove SPIFFS image file
  submodules-clean   Reset state of all third-party submodules
  user-lib-clean     Remove generated Component (user) libraries
  clean              Remove all generated build files

Tools
  decode-stacktrace  Open the stack trace decoder ready to paste dump text. Alteratively, use `make decode-stacktrace TRACE=/path/to/crash.stack`
  terminal           Open the serial terminal

Help
  spiff_update       Rebuild the SPIFFS filesystem image
  gdb                Run the debugger console
  help               Show this help summary
  list-config        Print the contents of internal build variables

Flashing
  flashconfig        Erase the rBoot config sector
  flashinit          Erase your device's flash memory and reset system configuration area to defaults
  flashboot          Write just the rBoot boot sector
  flashapp           Write just the application image
  flashfs            Write just the SPIFFS filesystem image
  flash              Write the rBoot boot sector, application image and (if enabled) SPIFFS image

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 21, 2019

I've renamed the targets as you suggest to something more meaningful, but needs a more thorough review. I'm keeping the list of outstanding issues (top of this PR) updated, some of them are just questions to consider.

@slaff
Copy link
Contributor

slaff commented Jun 21, 2019

The build generally works OK, but there are some issues still to be addressed:

  • Should Services be compiled as Components? If so, should they should be moved into Components?

Yes, that sounds like a good idea. But I would prefer it to be part of a separate PR. This way we can focus on Services and remove some compile time directives related to CommandHandler that will not be needed any longer when the application can specify which optional components are required.

  • Consider breaking Sming into smaller Components. For example, network stuff could be separated.

Maybe leave it for later. But I am open to suggestions.

  • What about 'boards' support for pre-configured hardware information? How would this fit into the build system? Can we make use of existing Arduino metadata?

Later.

  • SSL support is not enabled warning - is it still required?

The SSL stuff probably will need some refactoring if we want to allow other SSL libraries to be used (based on support per Architecture). So I would suggest to leave this for the moment and discuss it again when it is time for have SSL for Host architecture.

  • Update all sample projects to use new style of Makefile, with component.mk file if required. Consider renaming Makefile-app.mk to project.mk. (Note: Basic_Blink has been updated as a demo.)

That should be done in this PR.

  • All sub-makes are invoked to ensure Components are all up to date, but this does make the output a bit verbose. Could make these 'silent' unless explicitly requested.

At the moment it looks good to me. So leave it as it is.

  • Document every component.mk file with standard header

What do you suggest as a standard. What information should be included? Mandatory fields, optional ones? Is there plan to parse that information from other tools and generate human readable documentation?

  • Components without any source code produce empty library. This is because, for simplicity, we don't want to add a component.mk to every Arduino library.

That's fine. We can improve it later.

  • Consider implementing COMPONENT_EMBED_FILES, COMPONENT_EMBED_TXTFILES?

As a separate PR, please.

  • Some samples use COM_SPEED_SERIAL, but most use SERIAL_BAUD_RATE. Why are there two different variables?

One was used for the flash upload speed and another one was used for the serial I/O speed. They might have got mixed in between. Have to double check BUT that is again task deserving a separate PR.

  • Switching between LWIP versions doesn't work because Sming also needs rebuilding, so that'll need a variant. Not ideal. Can the headers be made compatible between versions?

LWIP 2 and LWIP 1 have differences so we cannot make the headers compatible. We should make LWIP 1 - the open source version and the binary LWIP 1 compatible.

Ideally we should support only one LWIP version. Preferably latest LWIP 2 version. But I am not sure if SmartConfig will work with LWIP 2.

  • LWIP2 doesn't seem to work. Investigate.
  • After a make rebuild, etc. need to use same options (e.g. ENABLE_GDB) with make flash. If build options are written to a file then they can be recalled for other operations.
  • Need to determine when App requires rebuilding because of an option change.
  • Sometimes desirable to use UART_1 by default instead of UART_0. Find a way to change the default UART_ID for Serial, e.g. SERIAL_UART_ID.

Leave it as it is for this PR. Such a change will need better communication with the Sming users.

  • Review all build targets:
  • Split list-config up, there's too much output

Maybe you can add an option to specify which section is requested. For example

make list-config SECTION=CONFIG_VARS 

can show only the configuration variables.

  • Add separate target for listing Components, their libnames and dependent variables

That can be part of this PR.

  • By default, only invoke sub-makes if targets don't exist. To check if they're up to date, use rebuild target.

That can be part of this PR.

@slaff slaff mentioned this pull request Jun 25, 2019
mikee47 added 5 commits June 28, 2019 21:37
Provided to complement the `terminal` target, but it would be more user-friendly perhaps if any required terminals/telnets launched automatically when the application is run.
.travis/build.sh Show resolved Hide resolved
@slaff
Copy link
Contributor

slaff commented Jun 30, 2019

@mikee47 The Windows Host build fails. That needs to be addressed.

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 30, 2019

See #1724 (comment) above. If you re-run the appveyor test it'll probably succeed. This only happens very occasionally, Could try dropping to -j2 or back to -j1 if too unreliable.

@slaff
Copy link
Contributor

slaff commented Jun 30, 2019

Could try dropping to -j2 or back to -j1 if too unreliable.

Yes, please. We would like to have a reliable build system :)

@slaff
Copy link
Contributor

slaff commented Jun 30, 2019

@mikee47 Any last words before I merge this PR?

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 30, 2019

@slaff Just one minor fix, otherwise good to go.

@slaff
Copy link
Contributor

slaff commented Jun 30, 2019

Just one minor fix, otherwise good to go.

Ok, tell me when you are ready.

@mikee47
Copy link
Contributor Author

mikee47 commented Jun 30, 2019

Just pushed it. You'll see it's pretty trivial but quite important.

@slaff slaff merged commit fdee78c into SmingHub:develop Jun 30, 2019
@slaff slaff removed the 0 - Backlog label Jun 30, 2019
@mikee47 mikee47 deleted the update/modularise_makefiles branch June 30, 2019 19:57
@mikee47 mikee47 mentioned this pull request Jul 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants