Skip to content

Conversation

@jcarrano
Copy link
Contributor

Contribution description

There are some hacky hacks in the build systems so that one can do

make -j clean all

that is, clean and all at the same time with parallelism, and not have both conflicts.

Script demos (lua, js) have to add new targets to compile scripts into the app. These were not correctly declared and caused simultaneous cleaning and building to fail.

Aside from this, the declarations were not well done in general. Adding the script targets to RIOTBUILD_CONFIG_HEADER is a hack, the correct variable is BUILDDEPS.

Testing procedure

For each package, run make -j clean all. It should build.

Issues/PRs references

This bug is exposed by #10344 (not because of any logical reason, only because of the hackyness required to have "make clean all" work).

@jcarrano jcarrano added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) 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 labels Nov 22, 2018
@jcarrano jcarrano requested a review from cladmi November 22, 2018 15:36
@jcarrano jcarrano changed the title examples/{javascript/lua}: correctly decare rules to embed scripts. examples/{javascript/lua}: correctly declare rules to embed scripts. Nov 22, 2018
@cladmi
Copy link
Contributor

cladmi commented Nov 22, 2018

To give some context, clean all in parallel means delete and create at the same time, so has no reason to work.
The current work around to make this work, is to enforce doing clean before other targets if required.
So to do the same trick for the directory and the created file as what is globally done:

RIOT/Makefile.include

Lines 484 to 487 in 7816497

# The `clean` needs to be serialized before everything else.
ifneq (, $(filter clean, $(MAKECMDGOALS)))
all $(BASELIBS) $(USEPKG:%=$(RIOTPKG)/%/Makefile.include) $(BUILDDEPS): clean
endif

BUILDDEPS was introduced for this type of intermediate files and already handles being executed after clean. It also removes the trick of putting it as a dependency of RIOTBUILD_CONFIG_HEADER_C to trigger the build.


However it cannot really be tested alone, as right now, there is the pkg/*/Makefile.include target that forces an order.

To test it I added this diff:

diff --git a/Makefile.include b/Makefile.include
index cd6ed166b..86f371c21 100644
--- a/Makefile.include
+++ b/Makefile.include
@@ -491,6 +491,7 @@ pkg-build-%: $(BUILDDEPS)
        $(QQ)"$(MAKE)" -C $(RIOTPKG)/$*

 clean:
+       sleep 5
        -@for i in $(USEPKG) ; do "$(MAKE)" -C $(RIOTPKG)/$$i clean ; done
        -@rm -rf $(BINDIR)
        -@rm -rf $(SCANBUILD_OUTPUTDIR)

And tested with #10344 , (based on this PR) it works when doing.

APPLICATION="examples/javascript"; rm -rf ${APPLICATION}/bin; make -j --no-print-directory clean all -C ${APPLICATION}

for the 3 applications.

When using only the last commit of the PR, which removes the pkg/*/Makefile.include targets, it fails for all 3.

APPLICATION="examples/lua_basic/"; rm -rf ${APPLICATION}/bin; make -j --no-print-directory clean all -C ${APPLICATION}
sleep 5
Building application "lua_basic" for "native" with MCU "native".

xxd -i main.lua | sed 's/^unsigned/const unsigned/g' > /home/harter/work/git/RIOT/examples/lua_basic/bin/native/lua/main.lua.h
rm -Rf /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/lua
mkdir -p /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/lua
/home/harter/work/git/RIOT/dist/tools/git/git-cache clone "https://github.com/lua/lua.git" "e354c6355e7f48e087678ec49e340ca0696725b1" "/home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/lua"
git-cache: cloning from cache. tag=commite354c6355e7f48e087678ec49e340ca0696725b1
touch /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/lua/.git-downloaded
git -C /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/lua checkout -f e354c6355e7f48e087678ec49e340ca0696725b1
HEAD is now at e354c6355e small updates
git -c user.email=buildsystem@riot -c user.name="RIOT buildsystem" -C /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/lua am --no-gpg-sign --ignore-whitespace "/home/harter/work/git/RIOT/pkg/lua"/patches/*.patch
Applying: Remove luaL_newstate.
Applying: Allow LUAL_BUFFERSIZE to be defined in the command line.
Applying: Make size of LoadF buffer configurable.
Applying: Remove os.tmpname.
Applying: Do not allocate buffers on the stack.
Applying: Cleanup test module.
Applying: Add a proper makefile.
Applying: Default to 32 bit build and small buffer size.
touch /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/lua/.git-patched
rm -Rf /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/tlsf
mkdir -p /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/tlsf
/home/harter/work/git/RIOT/dist/tools/git/git-cache clone "https://github.com/mattconte/tlsf" "a1f743ffac0305408b39e791e0ffb45f6d9bc777" "/home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/tlsf"
git-cache: cloning from cache. tag=commita1f743ffac0305408b39e791e0ffb45f6d9bc777
touch /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/tlsf/.git-downloaded
git -C /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/tlsf checkout -f a1f743ffac0305408b39e791e0ffb45f6d9bc777
HEAD is now at a1f743ffac Merge pull request #3 from velvitonator/large-alloc-corruption
git -c user.email=buildsystem@riot -c user.name="RIOT buildsystem" -C /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/tlsf am --no-gpg-sign --ignore-whitespace "/home/harter/work/git/RIOT/pkg/tlsf"/patches/*.patch
Applying: Fix warnining on implicit pointer conversion.
touch /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/tlsf/.git-patched
"make" -C /home/harter/work/git/RIOT/pkg/lua
"make" -C /home/harter/work/git/RIOT/pkg/tlsf
"make" -C /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/lua -f Makefile.lua
"make" -C /home/harter/work/git/RIOT/examples/lua_basic/bin/pkg/native/tlsf
cc1: error: /home/harter/work/git/RIOT/examples/lua_basic/bin/native/lua: No such file or directory [-Werror=missing-include-dirs]
cc1: all warnings being treated as errors
/home/harter/work/git/RIOT/Makefile.base:83: recipe for target '/home/harter/work/git/RIOT/examples/lua_basic/bin/native/tlsf/tlsf.o' failed
make[2]: *** [/home/harter/work/git/RIOT/examples/lua_basic/bin/native/tlsf/tlsf.o] Error 1
Makefile:9: recipe for target 'all' failed
make[1]: *** [all] Error 2
/home/harter/work/git/RIOT/examples/lua_basic/../../Makefile.include:491: recipe for target 'pkg-build-tlsf' failed
make: *** [pkg-build-tlsf] Error 2
make: *** Waiting for unfinished jobs....

@cladmi
Copy link
Contributor

cladmi commented Nov 22, 2018

@smlng could you also give a try with mac, just for good measures. There should not be issues with the directory targets as they are explicit, but you never know.

@cladmi cladmi added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Nov 22, 2018
@smlng
Copy link
Member

smlng commented Nov 22, 2018

@cladmi works!

Copy link
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

Minor final nitpicks as you are changing things in the files:

  • You changed some of the @ to $(Q) and added some $(Q) but not all. Please either change none or change all. Would even be better in separate commits.
  • I would prefer if it stays consistent with the empty lines, I like the way you did it in the javascript Makefile with JS and JS_H together. But this one is really as you are changing the files together.

@jcarrano
Copy link
Contributor Author

I added the missing $(Q)

Custom targets should be added to BUILDDEPS. Without this patch
`make -j clean all" fails because of weird race condition (trying
to clean while building is kind of contradictory anyways.)
Custom targets should be added to BUILDDEPS. Without this patch
`make -j clean all" fails because of weird race condition (trying
to clean while building is kind of contradictory anyways.)
Custom targets should be added to BUILDDEPS. Without this patch
`make -j clean all" fails because of weird race condition (trying
to clean while building is kind of contradictory anyways.)
@cladmi cladmi added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Nov 23, 2018
Copy link
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

ACK this solves the issue when used with #10344 and does it using the correct way.

@cladmi 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 Nov 23, 2018
@cladmi
Copy link
Contributor

cladmi commented Nov 23, 2018

Rerunning murdock as build was failing.

@cladmi cladmi merged commit 29acd40 into RIOT-OS:master Nov 23, 2018
@aabadie aabadie added this to the Release 2019.01 milestone Dec 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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 Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants