make: add blob utility header by kaspar030 · Pull Request #11870 · RIOT-OS/RIOT

kaspar030 · 2019-07-19T10:38:11Z

Contribution description

This PR adds a utility makefile that allows arbitrary files to be made available as headers.
This is supposed to be used with, e.g., lua, jerryscript, micropython for scripting language source files.

Testing procedure

Contains a test application. Check CI output.

Also, try changing the blobs and see if everything gets rebuilt correctly. Hammer this using make -j ....
Do the same with BUILD_IN_DOCKER=1.

Issues/PRs references

Simpler alternative to #11497.

emmanuelsearch · 2019-09-09T10:31:30Z

@smlng ping ;)

kaspar030 · 2019-09-09T10:40:17Z

To provide context vs #11497, this PR unifies current blob handling (which is duplicated in examples/javascript, examples/lua_REPL and examples/lua_basic, and one more coming in #2968) to a shared makefile. It is still using xxd & sed, which #11497 tries to replace with a custom make-based implementation.

IMO, while I disagree with handling this using make itself, even should we agree on that, this PR provides a clean entry-point to do that. Thus the guts of #11497 could be hooked in easily by replacing the one xxd+sed line of this PR. But that should be a different discussion.

tests/blob/tests/01-run.py

aabadie · 2019-09-10T11:49:59Z

tests/blob/Makefile

+
+BLOBS += blobtest.txt
+
+TEST_ON_CI_WHITELIST += all


This is not needed anymore, right ?!

yup, removed

makefiles/blob.inc.mk

kaspar030 · 2019-09-10T11:54:44Z

@aabadie I accidentally over-force-pushed and then re-applied your suggestion commit.

aabadie · 2019-09-10T12:30:40Z

makefiles/application.inc.mk

 DIRS += $(RIOTCPU)/$(CPU) $(RIOTBOARD)/$(BOARD)
 DIRS += $(RIOTBASE)/core $(RIOTBASE)/drivers $(RIOTBASE)/sys

+BLOBS = $(APPLICATION_BLOBS)


Why not move this line to makefiles/blob.inc.mk ? Since this file is also included by Makefile.base, it will be more consistent to handle this in the related file. This is untested so maybe this doesn't work because I'm missing something.

If that line would be in blob.inc.mk, it would be used in every recursive make call (e.g., for every module). We only want it to be used for the application module. All other modules can directly use BLOBS.

From the corresponding commit description:

@kaspar030 make: pass BLOBS to makefiles/application.inc.mk For regular modules, adding files to BLOBS is sufficient to create the corresponding headers. Application modules are different, as they use a minimal makefile (makefiles.application.inc.mk) to build, thus application level variables are not available. This commit makes Makefile.include pass BLOBS to the application Makefile as APPLICATION_BLOBS, and application.inc.mk use that variable as value for BLOBS. The indirection is necessary so submakefiles (e.g., those visited by DIRS) do not hard override BLOBS.

Is it possible to add a comment about that here instead of inside the commit message ?

Now it is here, is that sufficient?

I've added some doc to applications.inc.mk.

aabadie · 2019-09-11T09:31:32Z

I was asked to have a look at this PR but I don't feel confident enough to give a valuable review on the Makefile changes. Especially because they touch Makefile.base. Maybe @cladmi or @jcarrano could be of more help here ?

I have more general comments btw:
From the commit message linked above:

For regular modules, adding files to BLOBS is sufficient to create the
corresponding headers.

So this means that this PR allows to embed blob defined in modules. The problem is that the test application doesn't show this. It's only applied to application blobs.

Another aspect is that the current implementation only generates a header from a text file and include this to the build. Conceptually, this is not really a blob.

jcarrano · 2019-09-13T09:56:19Z

I'll summarize a discussion we had AFK with @emmanuelsearch. Sorry for the long comment, and I don't mean to bash this PR- I feel that many of the ideas expressed here apply equally to other instances where we may be wanting to "abstract away" common functionality.

All this would achieve is remove the need for the following snippet to exist in the application's makefile:

JS_PATH = $(BINDIR)/js/$(MODULE)

# add directory of generated *.js.h files to include path
CFLAGS += -I$(JS_PATH)

# generate .js.h header files of .js files
JS = $(wildcard *.js)
JS_H = $(JS:%.js=$(JS_PATH)/%.js.h)

BUILDDEPS += $(JS_H) $(JS_PATH)/

include $(RIOTBASE)/Makefile.include

$(JS_PATH)/:
	$(Q)mkdir -p $@

$(JS_H): $(JS_PATH)/%.js.h: %.js | $(JS_PATH)/
	$(Q)xxd -i $< | sed 's/^unsigned/const unsigned/g' > $@

That was taken from the jerryscript package. Lua has something similar. Note that header files will end up in a js-specific path.

The propose module removes the need for this code, at the price of:

Reduced flexibility.
- User cannot have different paths for different file types.
- The datatype (const unsigned) is fixed.
  - In fact, the whole output template is fixed.
The code is now "officially" part of the RIOT build system, which means people will depend on in and each change is an API change.

The reduced flexibility means that any deviation from the assumption that this new module does would require reverting back to the above snippet.

Now, there are some very real problems which are not being solved by this or by anything currently in use. Many of these are actually limitations of our build system, unrelated to this PR.

Making files into arrays is easy, but the interesting part is indexing them (i.e. putting them into some sort of structure or table)
- I have come to believe that this is best done in an application specific way.
We still have to come up with a way to generate just the required dependencies.
- $(OBJC) $(OBJCXX): $(BLOB_H) (or builddeps...) is overkill, but we currently know no better way
What if I want to generate an object instead of a header.
- Needed, for example, to properly integrate stuff like vendor supplied binaries/ bootloaders /etc.
- Compiling a generated C file and is tricky.
- makefiles/blobs: Add rules for binary blob embedding. #11497 does that, but I did not insist too much on that because the rest of the complaints in this PR apply to it as well (my justification for that PR was that it is something tricky to get right).

In conclusion, I'm against too much convenience functions, since these have to be maintained too.

kaspar030 · 2019-09-13T10:12:02Z

User cannot have different paths for different file types.

If there's a use-case for that, it can be added. (e.g., by properly dealing with subfolders so blobs don't have to be in the module's folder.)

The datatype (const unsigned) is fixed.

Did you encounter this use case before?
Anyhow, this would be easy to add, maybe using BLOBS_VAR in addition to BLOBS.

In fact, the whole output template is fixed.
I have come to believe that this is best done in an application specific way.

I agree.

What if I want to generate an object instead of a header.

Well, linking an object is a different function than creating data that is accessible from C from a binary.

kaspar030 · 2019-09-13T10:43:57Z

If there's a use-case for that, it can be added.

Well, I did, now BLOBS += $(wildcard js/*.js) should work correctly.

kaspar030 · 2019-09-13T10:47:05Z

So this means that this PR allows to embed blob defined in modules. The problem is that the test application doesn't show this. It's only applied to application blobs.

added a module using a blob

Another aspect is that the current implementation only generates a header from a text file and include this to the build. Conceptually, this is not really a blob.

added a binary blob

aabadie · 2019-09-13T11:36:40Z

tests/blob/blob_module/Makefile

@@ -0,0 +1,2 @@
+BLOBS += blobtest.bin


This file is missing ;)

thanks! It was blanked out by .gitignore...

kaspar030 · 2019-09-23T14:32:32Z

added some possible testing methods

emmanuelsearch · 2019-10-03T13:28:20Z

and @cladmi are you actually planning to review?

aabadie · 2019-10-04T14:45:22Z

@kaspar030, do you think it's possible to achieve the same result without using SECONDEXPANSION ?
In the current state, all targets defined after are impacted, even the ones that comes after the include of blob.inc.mk in Makefile.base. This means for all those targets we'll have to be very careful not escaping them.
@cladmi, do you think you'll have time to do a proper review and also give your feedback soon ?

bergzand · 2019-10-07T09:25:58Z

@bergzand so is that an ACK for real now ? ;)

Seeing the concerns raised by @cladmi, I'm no longer sure. I'm not confident enough in my makefile skills to merge this right before the hard freeze. Are there any issues with merging this after the hard freeze?

cladmi · 2019-10-07T11:50:21Z

makefiles/blob.inc.mk

+
+# make C and C++ objects of this module depend on generated headers, so they
+# get re-build on changes to the blob files.
+$(OBJC) $(OBJCXX): $(BLOB_H)


The comment is wrong, rebuilding would be done automatically from the .d generated files as the compilation is done with -MD -MP.
This also does not account for deleted files that the .d does.

This is only currently required to have the headers generated before compiling the C files, so with a : |$(BLOB_H) it would work too.

I've updated the comment.

Using : |$(BLOB_H), no rebuild is triggered when changing a blob.

cladmi · 2019-10-07T12:17:46Z

It looks like the SECONDEXPANSION is only required to allow having subdirectories for headers inside blob. Is this a required feature?

If it really is, the SECONDEXPANSION is not really required here.
The $$(@D) handling is only needed if the targets list is unknown to have a generic handling or pattern rules.

Here as all targets are basically known and could be handled with a stupid handling that all targets depend on having created all the directories first:

BLOB_DIRECTORIES = $(addsuffix .,$(dir $(BLOB_H)))

$(BLOB_H): $(BLOB_HDR_DIR)/%.h: % $(BLOBS) | $(BLOB_DIRECTORIES)

Or if you want one to one mapping:

set_dir_dependency = $(foreach target,$(1),$(eval $1: | $(dir $(target)).))
$(call set_dir_dependency,$(BLOB_H))

IIRC, the /. syntax is to handle make <4 as currently used in OSx.
It should not be necessary anymore after 2020.01 as make <4 is deprecated in RIOT.

For the naming, I would not say it generates blobs as #11497 did, it is an xxd wrapper than only gives read only outputs.
From what I get this is only to replace:

RIOT/examples/javascript/Makefile

Lines 58 to 59 in 255ce7d

    
           $(JS_H): $(JS_PATH)/%.js.h: %.js | $(JS_PATH)/ 
        
           	$(Q)xxd -i $< | sed 's/^unsigned/const unsigned/g' > $@

Adding directory variables creation is something that would be great globally in Makefile.base and Makefile.include added on its own instead of added here in the middle of another things that does not really need it.

cladmi · 2019-10-07T11:52:35Z

makefiles/blob.inc.mk

+$(BLOB_HDR_DIR)/.:
+	@mkdir -p $@
+
+$(BLOB_HDR_DIR)%/.:


Why not just %/.: it is a directory creation rule ?
Is it because of some make versions? I know some have issues with implicit rules sometime.

This also apply for the .PRECIOUS line.

The directory creation rule could also be put in common in Makefile.base instead of here.

It could, but as is it is self-contained.

cladmi · 2019-10-07T11:52:56Z

makefiles/blob.inc.mk

+
+XXD ?= xxd
+
+$(BLOB_H): $(BLOB_HDR_DIR)/.


Why this line?

not needed, I removed it.

kaspar030 · 2019-10-10T11:29:22Z

It looks like the SECONDEXPANSION is only required to allow having subdirectories for headers inside blob. Is this a required feature?

It was asked for, and it makes sense, as e.g. jerryscript uses a "js/" subfolder.

kaspar030 · 2019-10-10T11:29:40Z

If it really is, the SECONDEXPANSION is not really required here.

Does it hurt?

kaspar030 · 2019-10-10T11:30:01Z

IIRC, the /. syntax is to handle make <4 as currently used in OSx.

correct.

kaspar030 · 2019-10-18T09:50:06Z

Does it hurt?

@cladmi, other than issues with variables containing dollarsigns, do you know any downsides of enabling SECONDEXPANSION globally?

kaspar030 · 2019-11-14T11:45:56Z

split out the SECONDEXPANSION change (make: enable SECONDEXPANSION for module/application builds #12708), rebased on that.

aabadie · 2019-11-18T15:11:26Z

#12708, please rebase.

kaspar030 · 2019-11-18T15:40:03Z

rebased

aabadie

Just minor remaining things in the test. Otherwise that looks good. You can squash directly.

aabadie · 2019-11-18T15:48:42Z

tests/blob/Makefile

@@ -0,0 +1,10 @@
+DEVELHELP ?= 0


Why disabling DEVELHELP by default? I would keep it.

yup, removed the line (default is 1)

aabadie · 2019-11-18T15:50:11Z

tests/blob/tests/01-run.py

+
+
+if __name__ == "__main__":
+    sys.exit(run(testfunc, timeout=120))


I don't think the custom timeout is required. The test is running fast enough.

removed the timeout

For regular modules, adding files to BLOBS is sufficient to create the corresponding headers. Application modules are different, as they use a minimal makefile (makefiles.application.inc.mk) to build, thus application level variables are not available. This commit makes Makefile.include pass BLOBS to the application Makefile as APPLICATION_BLOBS, and application.inc.mk use that variable as value for BLOBS. The indirection is necessary so submakefiles (e.g., those visited by DIRS) do not hard override BLOBS.

aabadie

ACK!

aabadie · 2019-11-18T19:43:10Z

@bergzand, we need your approval here. Are your concerns addressed ?

bergzand

Ack

bergzand · 2019-11-18T20:54:57Z

@bergzand, we need your approval here. Are your concerns addressed ?

Yes

kaspar030 · 2019-11-18T20:55:21Z

Thanks everyone!

kaspar030 added Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jul 19, 2019

kaspar030 requested review from miri64 and smlng July 19, 2019 10:42

kaspar030 mentioned this pull request Jul 19, 2019

pkg: micropython: initial commit #2968

Merged

1 task

kaspar030 force-pushed the blobs branch from 2af0b20 to ab87f5e Compare July 19, 2019 11:50

miri64 mentioned this pull request Jul 30, 2019

makefiles/blobs: Add rules for binary blob embedding. #11497

Closed

aabadie reviewed Sep 9, 2019

View reviewed changes

tests/blob/tests/01-run.py Outdated Show resolved Hide resolved

aabadie reviewed Sep 10, 2019

View reviewed changes

makefiles/blob.inc.mk Outdated Show resolved Hide resolved

kaspar030 force-pushed the blobs branch from c80b2d7 to ddfe912 Compare September 10, 2019 11:53

aabadie reviewed Sep 10, 2019

View reviewed changes

kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 10, 2019

aabadie reviewed Sep 13, 2019

View reviewed changes

cladmi reviewed Oct 7, 2019

View reviewed changes

kaspar030 mentioned this pull request Nov 14, 2019

make: enable SECONDEXPANSION for module/application builds #12708

Merged

kaspar030 force-pushed the blobs branch from 3b8fc6b to cff4e03 Compare November 14, 2019 11:44

kaspar030 force-pushed the blobs branch from cff4e03 to a1ba77a Compare November 18, 2019 15:33

aabadie requested changes Nov 18, 2019

View reviewed changes

kaspar030 added 3 commits November 18, 2019 17:53

makefiles/blob.inc.mk: introduce blob -> hdr utility makefile

a100c1c

tests/blob: add test application for blob.inc.mk

3091de3

kaspar030 force-pushed the blobs branch from a1ba77a to 3091de3 Compare November 18, 2019 16:53

aabadie approved these changes Nov 18, 2019

View reviewed changes

bergzand approved these changes Nov 18, 2019

View reviewed changes

bergzand merged commit 76b9642 into RIOT-OS:master Nov 18, 2019

kaspar030 deleted the blobs branch November 18, 2019 20:55

kaspar030 mentioned this pull request Nov 18, 2019

examples/javascript: make use of blob.inc.mk #12742

Merged

fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019



		if __name__ == "__main__":
		sys.exit(run(testfunc, timeout=120))

Conversation

kaspar030 commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

emmanuelsearch commented Sep 9, 2019

Uh oh!

kaspar030 commented Sep 9, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kaspar030 commented Sep 10, 2019

Uh oh!

aabadie Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Sep 11, 2019

Uh oh!

jcarrano commented Sep 13, 2019

Uh oh!

kaspar030 commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspar030 commented Sep 13, 2019

Uh oh!

kaspar030 commented Sep 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Sep 23, 2019

Uh oh!

emmanuelsearch commented Oct 3, 2019

Uh oh!

aabadie commented Oct 4, 2019

Uh oh!

bergzand commented Oct 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Oct 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Oct 10, 2019

Uh oh!

kaspar030 commented Oct 10, 2019

Uh oh!

kaspar030 commented Oct 10, 2019

Uh oh!

kaspar030 commented Oct 18, 2019

Uh oh!

kaspar030 commented Nov 14, 2019

Uh oh!

aabadie commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

kaspar030 commented Jul 19, 2019 •

edited

Loading

aabadie Sep 10, 2019 •

edited

Loading

kaspar030 commented Sep 13, 2019 •

edited

Loading

aabadie commented Nov 18, 2019 •

edited

Loading