arduino/sketches: build sketches as a module by cladmi · Pull Request #7654 · RIOT-OS/RIOT

cladmi · 2017-09-28T10:58:52Z

Generate a module for arduino sketches in a subfolder of BINDIR.
This prevents issues when doing concurrent builds or out of tree build with readonly sources.

This fixes #5848

The concept is instead of generating a cpp file in the source directory, it is generated into the build directory. However, this requires defining a new module for this directory.
It is now generated during build and not parsing.
I did not addressed the "write atomically" as I think it is not necessary anymore.

To do this, I replaced the previous bash script by a Makefile script which creates the module as link target prerequisite.

Testing procedure

As it was not asked before, I will give a testing procedure:

Setup:

Clean
create bin
remove write permission in the application directory

git clean -xdf examples/arduino_hello-world
mkdir -p examples/arduino_hello-world/bin
chmod -w examples/arduino_hello-world
make -C examples/arduino_hello-world clean all BOARD=arduino-mega2560

Cleanup

chmod u+w examples/arduino_hello-world

It works with this PR.

It fails on master, as we try to write in the directory and so the main function is never declared.
You can also notice that the Permission denied are there before even starting to build. make clean also tries to create the file.

Fixes #5848

cladmi · 2017-11-14T14:26:37Z

Finally updated the documentation.
I rebased with the current master branch.

My questions are:

what do you think about the concept?
do you think it would be better that Makefile.arduino_sketches.inc defined a function that we call after to create the rules or defining stuff directly in it is fine?

jnohlgard

It makes sense to generate the sketch code in a separate module under BINDIR. Can we rename the Makefile.arduino_sketches.inc file though? We were moving away from the Makefile.* pattern and to a *.inc.mk pattern. Would it make sense to move these makefiles to /makefiles?

jnohlgard · 2017-11-14T20:30:02Z

+	@mkdir -p $@
+
+# Make everything rebuild if current makefile changes
+CURMK = $(word $(words $(MAKEFILE_LIST)),$(MAKEFILE_LIST))


You could use the lastword function instead

Better use a different name than CURMK. This file is included from the Makefile.include which means it will pollute the name space.

I will change to lastword I learned it after writing this, and update to a namespaced variable name.

cladmi · 2017-11-15T10:31:52Z

I will change the Makefile name too. Do I just add .mk or do you have a better naming idea?

However, I do not want to move it to makefiles. I think the content of makefiles should only contain files that are related to the general build system, but not module specific stuff.

jnohlgard · 2017-11-15T15:05:55Z

+	@mkdir -p $@
+
+# Make everything rebuild if current makefile changes
+_ARDUINO_SKETCHES_MAKEFILE = $(lastword $(MAKEFILE_LIST))


I think this needs an immediate assignment (:=)to avoid the situation where MAKEFILE_LIST is updated by including another file later.

The value is used directly and only in the two following lines, so following imports will not impact.
However, as I am not using immediate assignment, lastword is executed twice, so I will change.

jnohlgard · 2017-11-15T15:06:58Z

@cladmi sounds reasonable. 👍 for arduino_sketches.inc.mk

cladmi · 2017-11-24T17:29:03Z

Can I rebase and squash ?

jnohlgard

One question and one minor nit

jnohlgard · 2017-11-24T20:17:23Z

@@ -0,0 +1,31 @@
+# Compile together the Arduino sketches of the application
+# They are declared a as new module $(SKETCH_MODULE) in $(SKETCH_MOD_DIR)


typo SKETCH_MOD_DIR -> SKETCH_MODULE_DIR

Thanks, I missed this one.

jnohlgard · 2017-11-24T20:22:15Z

+# Building the module files
+#   Do not use $^ in receipes as Makefile is also a prerequisite
+$(SKETCH_MODULE_DIR)/Makefile: $(SKETCH_MODULE_DIR)/$(SKETCH_CPP)
+	$(Q)echo 'SRCXX = $(SKETCH_CPP)'               > $@


Is there any way this recipe can be called multiple times during the same build? (race condition clobbering the destination file)
For example if building multiple sketches in different source directories?

You cannot put sketches in multiple directories as post.snip is defining the main function.
As I understood it, its to replace the code you would put in your application directory by this arduino sketches thing. That's why SKETCHES are in APPDIR which is basically CURDIR.

jnohlgard · 2017-11-24T20:25:23Z

OK to squash

cladmi · 2017-11-30T10:16:37Z

@smlng @haukepetersen do you agree with this version ?

smlng · 2017-11-30T10:26:47Z

untested ACK

jnohlgard · 2018-02-12T08:00:31Z

I forgot about this one, I think it is pretty much ready for merge.

cladmi · 2018-02-20T13:16:52Z

I noticed this one suffers from the same problem as all modules where deleting one of the SKETCHES would not trigger a rebuild.
I will force re-building the .cpp everytime to get the same behaviour as before.

There are different ways to properly fix it:

either generate an equivalent of .d file included by make
change the sketches.cpp to directly include the .sketch files and GCC would do the job of generating the .d file.

cladmi · 2018-04-12T12:25:40Z

I needs to modify this as now "link" is not the right target to depend anymore

cladmi · 2018-04-19T15:24:03Z

I was thinking on putting this in APPDEPS but it has been put in BASELIBS by #4906 I will need to do a proper PR for fixing this.

miri64 · 2018-12-18T13:56:29Z

This was ACK'd almost a year ago. What's the hold-up?

cladmi · 2018-12-18T17:43:32Z

As my previous comment said, as other PRs were merged I changed things in this one after it was ACK.

And I just noticed that, as we had the issue recently in another PR, the directory target will not work on mac as is as it is using make 3.81.
It may also have issues with parallel make -j clean all execution.

I will look into these ones when I find time.

cladmi · 2019-08-12T13:41:22Z

I removed the directory creation target and included it in the .cpp file creation.
I also now declare all generated files as BUILDDEPS to prevent any issues with clean all in parallel.

The handling of re-building is still done with a FORCE and doing it with .d files would be better handled globally.
I would squash the two commits together at the end.

tcschmidt · 2019-08-16T23:58:53Z

@smlng this update needs a review

smlng

tested ACK for arduino-uno, configuration and documentation looks good.

One question: why sys/arduino/arduino_sketches.inc.mk and not makefiles/arduino/arduino_sketches.inc.mk - and (as it is already in a subdir) call the file sketches.inc.mk?

Otherwise this is good to be merged.

smlng · 2019-08-21T06:23:30Z

also: please squash

cladmi · 2019-08-21T09:56:51Z

One question: why sys/arduino/arduino_sketches.inc.mk and not makefiles/arduino/arduino_sketches.inc.mk - and (as it is already in a subdir) call the file sketches.inc.mk?

I find it really stupid as organization to not be in the sys/arduino directory.
Having a module specific handling in makefiles is putting specific code in your general purpose library.

For the arduino_sketches, I named it as I created the module with the name arduino_sketches. But indeed, there is already the arduino directory. I will push the change first (so you can see the diff) and then squash.

Generate a module for arduino sketches in a subfolder of BINDIR. This prevents issues when doing concurrent builds or out of tree build with readonly sources. Declare all generated files as `BUILDDEPS` to be re-created after `clean` on parrallel `clean all`.

cladmi · 2019-08-21T10:01:32Z

Why is it not showing the commit in the text description? it was this one to help you review: c3035df

smlng · 2019-08-21T10:15:40Z

I find it really stupid as organization to not be in the sys/arduino directory.
Having a module specific handling in makefiles is putting specific code in your general purpose library.

agreed, thanks for the explanation!

smlng

retested ACK

cladmi · 2019-08-21T13:35:08Z

Thank you for the review.

This now does not generate files while parsing dependencies anymore !

smlng requested review from haukepetersen and jnohlgard September 28, 2017 15:22

smlng added Area: arduino API Area: Arduino wrapper API Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Sep 28, 2017

smlng reviewed Sep 28, 2017

View reviewed changes

Comment thread sys/arduino/doc.txt Outdated

miri64 assigned jnohlgard, haukepetersen and smlng Oct 4, 2017

cladmi force-pushed the pr/arduino_sketches.cpp branch from f3be607 to 424420e Compare November 14, 2017 14:19

jnohlgard requested changes Nov 14, 2017

View reviewed changes

jnohlgard requested changes Nov 15, 2017

View reviewed changes

cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 16, 2017

jnohlgard reviewed Nov 24, 2017

View reviewed changes

cladmi force-pushed the pr/arduino_sketches.cpp branch from cf6969f to 04756e5 Compare November 27, 2017 13:29

jnohlgard approved these changes Feb 12, 2018

View reviewed changes

cladmi force-pushed the pr/arduino_sketches.cpp branch from 04756e5 to cb5b52d Compare February 20, 2018 13:25

cladmi 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 Feb 27, 2018

cladmi added this to the Release 2018.07 milestone Apr 12, 2018

jia200x modified the milestones: Release 2018.10, Release 2019.01 Oct 25, 2018

gschorcht mentioned this pull request Dec 11, 2018

sys/arduino: Added Wire (I2C) interface #10592

Merged

cladmi 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 Dec 18, 2018

aabadie modified the milestones: Release 2019.01, Release 2019.04 Jan 25, 2019

cladmi modified the milestones: Release 2019.04, Release 2019.07 Apr 23, 2019

cladmi modified the milestones: Release 2019.07, Release 2019.10 Jun 14, 2019

cladmi force-pushed the pr/arduino_sketches.cpp branch from 3cdbe11 to d683826 Compare August 12, 2019 13:27

cladmi mentioned this pull request Aug 12, 2019

arduino: Race condition in sys/arduino/Makefile.include #5848

Closed

smlng added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Aug 21, 2019

smlng suggested changes Aug 21, 2019

View reviewed changes

cladmi force-pushed the pr/arduino_sketches.cpp branch from c3035df to be30f07 Compare August 21, 2019 09:58

smlng approved these changes Aug 21, 2019

View reviewed changes

smlng merged commit 8e08748 into RIOT-OS:master Aug 21, 2019

cladmi deleted the pr/arduino_sketches.cpp branch August 21, 2019 13:34

gschorcht mentioned this pull request Sep 5, 2019

sys/arduino: Added SPI interface #12118

Merged

		@@ -0,0 +1,31 @@
		# Compile together the Arduino sketches of the application
		# They are declared a as new module $(SKETCH_MODULE) in $(SKETCH_MOD_DIR)

Uh oh!

Conversation

cladmi commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing procedure

Uh oh!

Uh oh!

cladmi commented Nov 14, 2017

Uh oh!

jnohlgard left a comment

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!

cladmi commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Nov 15, 2017

Uh oh!

cladmi commented Nov 24, 2017

Uh oh!

jnohlgard left a comment

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!

jnohlgard commented Nov 24, 2017

Uh oh!

cladmi commented Nov 30, 2017

Uh oh!

smlng commented Nov 30, 2017

Uh oh!

jnohlgard commented Feb 12, 2018

Uh oh!

cladmi commented Feb 20, 2018

Uh oh!

cladmi commented Apr 12, 2018

Uh oh!

cladmi commented Apr 19, 2018

Uh oh!

miri64 commented Dec 18, 2018

Uh oh!

cladmi commented Dec 18, 2018

Uh oh!

cladmi commented Aug 12, 2019

Uh oh!

tcschmidt commented Aug 16, 2019

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

smlng commented Aug 21, 2019

Uh oh!

cladmi commented Aug 21, 2019

Uh oh!

cladmi commented Aug 21, 2019

Uh oh!

smlng commented Aug 21, 2019

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Aug 21, 2019

Uh oh!

Reviewers

Assignees

Labels

cladmi commented Sep 28, 2017 •

edited

Loading