pkg/pkg.mk: use intermediate state files#13036
Conversation
basilfx
added
State: WIP
|
Some tests I performed, that turned out to work OK. Git package with patches:
Git package without patches:
|
basilfx
added
the
CI: ready for build
basilfx
force-pushed
the
feature/save_state_and_dependencies
branch
2 times, most recently
from
9da176b to
4b4edd3
Compare
|
@basilfx are you basing you squash commits on testing for every pkg? Maybe we can split up testing? |
I'm very interested int his one as well and would like to see it in for the release. |
|
@fjmolinas Yeah, I was squashing them directly, sorry ;-) I'll add new commits for next fixes. But I hope that most of them are solved right now. We'll see in a few minutes. |
|
I merged #12805 without thinkins of this PR, sorry.. will stall other pkgs until this one gets in. |
|
@basilfx you can apply this patch to fix the wakama issue https://gist.github.com/fjmolinas/cd7cfd541dde80c0db16f4bfd3757f5d. |
basilfx
force-pushed
the
feature/save_state_and_dependencies
branch
from
4b4edd3 to
a336810
Compare
|
@fjmolinas Yes, on it. Will also rebase on master to include utensor. |
|
|
|
@basilfx how should we proceed with testing? make per pkg test? We have already tested a bunch anyway. |
|
@fjmolinas I'm not sure what we should test more. I'm quite sure that every package has at least one application to test it, and they all seem to work. Maybe @kaspar030 can give some suggestions? |
|
@fjmolinas If you agree then I will squash it, clean up the messages and remove the WIP tag. Then we have a clean slate again for reviewing. |
Yep go ahead! |
|
@fjmolinas Before I do, do you know how I could improve The additional steps in |
I'm not sure I understand Or by improve you mean you want to move them out of the |
Currently, when recompiling, the files are copied again. One of these files is |
|
Also, I think the whole |
Yes it what I was saying when commenting on |
|
Ok, cool. I though you were only talking about the changes I made to some of the dist/tools/* files :-) |
Ohh ok, hmm... I'm not sure.., how do you check that it is compiled every time? |
Oh this I do see, but that was not the case before right? I seem to recall it not being the case at one point. |
|
If I remove a patch, it keeps recompiling. The contents of /home/basilfx/RIOT/RIOT/tests/pkg_u8g2/bin/pkg/native/u8g2/.pkg-state.git-patched: /home/basilfx/RIOT/RIOT/pkg/u8g2/patches/0001-add-RIOT-OS-interface.patch /home/basilfx/RIOT/RIOT/tests/pkg_u8g2/bin/pkg/native/u8g2/.pkg-state.git-downloaded Makefile /home/basilfx/RIOT/RIOT/pkg/pkg.mk /home/basilfx/RIOT/RIOT/pkg/u8g2/patches/0002-test.patch
/home/basilfx/RIOT/RIOT/pkg/u8g2/patches/0001-add-RIOT-OS-interface.patch:
/home/basilfx/RIOT/RIOT/tests/pkg_u8g2/bin/pkg/native/u8g2/.pkg-state.git-downloaded:
Makefile:
/home/basilfx/RIOT/RIOT/pkg/pkg.mk:
/home/basilfx/RIOT/RIOT/pkg/u8g2/patches/0002-test.patch:Note that it still depends on the So I guess this file should be updated whenever the folder with the patches change? |
I was getting to the same thing: this seems to fix it But not sure why |
|
|
I think the file just needs to be deleted, its overwriting but not cleaning the old content. |
|
Ok, I bumped my head against the wall and said a lot of stupid things. But now I understand (the following helped me as well http://make.mad-scientist.net/papers/advanced-auto-dependency-generation/). When we generate The first line is important since it adds the old pre-requisites as a pre-requisite for The Issue was that we were generating the new requistes with all pre-requisites: including the old ones we just added!!! so of course it gets re-built all the time, duh. So instead of doing that lets only use the current pre-requisites, edit: and add some prints. index eff633a6f..cedc654ba 100644
--- a/pkg/pkg.mk
+++ b/pkg/pkg.mk
@@ -69,23 +69,31 @@ GIT_IN_PKG = git -C $(PKG_BUILDDIR) --git-dir=.git --work-tree=.
# $1: output file name
gen_dependency_files = $(file >$1,$@: $2)$(foreach f,$2,$(file >>$1,$(f):))
+# When $(PKG_PATCHED).d is included $(PKG_PATCHED) pre-requisites will include
+# the old pre-requisites forcing a rebuild on pre-requisite removal, but we do
+# not want to generate $(PKG_PATCHED).d with the old pre-requisites
+PKG_PATCHED_PRE_REQUISITES = $(PKG_PATCHES) $(PKG_DOWNLOADED) $(MAKEFILE_LIST)
+
# Patch the package
# * create dependencies files
# * clean, without removing the 'state' files
# * checkout the wanted base commit
# * apply patches if there are any. (If none, it does nothing)
-$(PKG_PATCHED): $(PKG_PATCHES) $(PKG_DOWNLOADED) $(MAKEFILE_LIST)
- $(call gen_dependency_files,[email protected],$^)
+$(PKG_PATCHED): $(PKG_PATCHED_PRE_REQUISITES)
+ $(info [INFO] patch $(PKG_NAME))
+ $(call gen_dependency_files,[email protected],$(PKG_PATCHED_PRE_REQUISITES))
$(Q)$(GIT_IN_PKG) clean $(GIT_QUIET) -xdff '**' $(PKG_STATE:$(PKG_BUILDDIR)/%=':!%*')
$(Q)$(GIT_IN_PKG) checkout $(GIT_QUIET) -f $(PKG_VERSION)
$(Q)$(GIT_IN_PKG) $(GITFLAGS) am $(GITAMFLAGS) $(PKG_PATCHES) </dev/null
@touch $@
$(PKG_DOWNLOADED): $(MAKEFILE_LIST) | $(PKG_BUILDDIR)/.git
+ $(info [INFO] updating $(PKG_NAME) $(PKG_DOWNLOADED))
$(Q)$(GIT_IN_PKG) fetch $(GIT_QUIET) $(PKG_URL) $(PKG_VERSION)
echo $(PKG_VERSION) > $@
$(PKG_BUILDDIR)/.git:
+ $(info [INFO] cloning $(PKG_NAME))
$(Q)rm -Rf $(PKG_BUILDDIR)
$(Q)mkdir -p $(PKG_BUILDDIR)
$(Q)$(GITCACHE) clone $(PKG_URL) $(PKG_VERSION) $(PKG_BUILDDIR) |
|
Testing:
make -C tests/pkg_u8g2/
make -C tests/pkg_u8g2/
make -C tests/pkg_u8g2/
make -C tests/pkg_u8g2/
QUIET=1 make -C tests/pkg_u8g2/ |
|
Thanks for looking into this issue! I can confirm it works as well. Also confirmed that changing the |
The file must not change the default goal otherwise it could change packages behavior.
This should prevent issues where the Makefile use 'PKG_BUILDDIR' before them being defined. This will also allow changing the state targets to be file targets.
Rely on file creation and dependencies instead of .PHONY targets. Files will be rebuilt when changing version as the main `Makefile` will have been updated. All steps are re-done on version change. When deleting patches, the '.prepare' step should be redone thanks to the included 'patch-dep.inc' file (TODO TEST ME). Implementation in order: * '.git': means the repository has been cloned. * '.git-downloaded': Fetches the wanted version * '.git-prepared': will clean checkout the version and apply patches
Prepare for handling pkg state with files. So it requires having the path defined before declaring targets. In addition, it cleans up the old git-download target.
Prepare for handling pkg state with files. So it requires having the path defined before declaring targets. In addition, it cleans up the old git-download target.
|
Murdock looks OK. Shall I give it another rebase? I'm not sure which things we should test more. I guess that we covered most of it already. |
I think we should be ok, there are some needed cleanups I have noticed here and there, but the main functionality is OK:
I think yous should go ahead with the rebase :) |
basilfx
force-pushed
the
feature/save_state_and_dependencies
branch
from
e29370d to
f08116f
Compare
|
Thank you as well for all the help with reviewing, testing and Makefile-fixxing! |
|
Seems like this PR broke |
5 participants
Contribution description
This is an updated version of #11553 by @cladmi . I did my best to fix all the merge errors. It might be a fix for #13030.
I don't have that much Makefile experience, so I hope someone can help me to get this sorted out.
Testing procedure
Copy pasted:
Issues/PRs references
#11553
#13030