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

zephyr/Makefile: Rework to use modern, official build integration. #2980

Closed
wants to merge 1 commit into from

Conversation

pfalcon
Copy link
Contributor

@pfalcon pfalcon commented Mar 27, 2017

Build happens in 3 stages:

  1. Zephyr config header and make vars are generated from prj.conf.
  2. libmicropython is built using them.
  3. Zephyr is built and final link happens.

@pfalcon
Copy link
Contributor Author

pfalcon commented Mar 27, 2017

@daniel-thompson : I guess this is right and tested it more or less, but as you consistently see more than I, please review if you have some time. Thanks!

Copy link
Contributor

@daniel-thompson daniel-thompson left a comment

Choose a reason for hiding this comment

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

You flatter me... however I've done my best to fetch some of the context from long term storage!

@@ -71,15 +70,10 @@ $(GENERIC_TARGETS): $(LIBMICROPYTHON)
$(CLEAN_TARGETS): clean

$(GENERIC_TARGETS) $(KCONFIG_TARGETS) $(CLEAN_TARGETS):
$(RM) -f outdir/$(OUTDIR_PREFIX)/zephyr.lnk
Copy link
Contributor

Choose a reason for hiding this comment

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

What has replaced this?

We've discussed before that doing the $(RM) here is over-zealous but we do need to remove zephyr.lnk when libmicropython.a is rebuilt and I can't see any logic to do that. Without that then when we change uPy C files they won't make it into the final zephyr application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn. Didn't you submit some patch to Z to address this?.. I'll submit new version which is going to execute $(RM) only if the library was actually updated...

Copy link
Contributor

Choose a reason for hiding this comment

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

I did but since what I wrote was hopelessly broken Anas pulled it just before v1.6 went out of the door (I did have a fix but is was pretty ugly).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I posted updated patch which addresses this. Testes all of: 1) build from scratch; 2) empty build afterwards; 3) touching a file and building. Behaves as expected (links in 1) and 3) cases, minimal extra command noise in 2) case).

zephyr/Makefile Outdated
$(Z_SYSGEN_H):
rm -f $(LIBMICROPYTHON)
-$(MAKE) -f Makefile.zephyr BOARD=$(BOARD) CONF_FILE=$(CONF_FILE)
$(LIBMICROPYTHON): | $(Z_EXPORTS)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this line do? $(Z_EXPORTS) is made a target by an include directive... so order-only prerequisite or not, I can't really see any way it could ever run after the $(LIBMICROPYTHON) rule.

#include <untested_idea.h> ... it occurs to me we should say what we actually mean and make mpconfigport.h depend on sysgen.h (e.g. the config file may inherit values from the config). Once we do that I think everything else will flow correctly (i.e. when we change the config and the uPy C files rebuild and the library will be relinked).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make order-only deps are a bit weird, they don't work oftentime in DWIM manner. But here they did. The idea is that I'm telling that libmicropython should be be only after Z_EXPORTS is generated (which generates autoconf.h as a by-product, on which source files for libmicropython depend). But if i use a normal dependency, Z_EXPORTS (Makefile.exports) ends up inside .a file, which isn't cute. So, with order-only syntax I say: "depend on it, but not include in prerequisite list", and this time, it works as expected (it doesn't always ;-) ).

it occurs to me we should say what we actually mean and make mpconfigport.h depend on sysgen.h

Well, sysgen.h is gone completely after dbb2aea , that's why refs to it are removed . And otherwise logic is per above and appear to work as expected.

Build happens in 3 stages:

1. Zephyr config header and make vars are generated from prj.conf.
2. libmicropython is built using them.
3. Zephyr is built and final link happens.
@pfalcon
Copy link
Contributor Author

pfalcon commented Mar 29, 2017

Ok, I'm definitely sure this patch is a big leap in the right direction, so even if there're some issue, we can fix them as a follow-up. Merging.

@pfalcon pfalcon closed this Mar 29, 2017
@pfalcon pfalcon deleted the zephyr-revamp-make branch March 29, 2017 21:08
@pfalcon
Copy link
Contributor Author

pfalcon commented Mar 30, 2017

LOL, this broke all the DTS boards. Will need to ping @andygross about it.

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Jun 1, 2020
.../devices.h: Add GigaDevices GD25S512MD 64 MiB flash chip support.
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.

None yet

2 participants