examples/{javascript/lua}: correctly declare rules to embed scripts.#10456
Merged
cladmi merged 3 commits intoRIOT-OS:masterfrom Nov 23, 2018
Merged
examples/{javascript/lua}: correctly declare rules to embed scripts.#10456cladmi merged 3 commits intoRIOT-OS:masterfrom
cladmi merged 3 commits intoRIOT-OS:masterfrom
Conversation
Contributor
|
To give some context, Lines 484 to 487 in 7816497
However it cannot really be tested alone, as right now, there is the 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. for the 3 applications. When using only the last commit of the PR, which removes the |
Contributor
|
@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. |
Member
|
@cladmi works! |
cladmi
suggested changes
Nov 23, 2018
Contributor
cladmi
left a comment
There was a problem hiding this comment.
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
javascriptMakefile withJSandJS_Htogether. But this one is really as you are changing the files together.
4 tasks
3e17123 to
64fcfff
Compare
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.)
Contributor
|
Rerunning murdock as build was failing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contribution description
There are some hacky hacks in the build systems so that one can do
that is,
cleanandallat 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).