WIP CLEANUP TESTING NEEDED pkg/pkg.mk: use intermediate state files by cladmi · Pull Request #11553 · RIOT-OS/RIOT

cladmi · 2019-05-20T14:42:44Z

Contribution description

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

Somehow the base is that patching can only be done on the correct commit, so lets do it all at once.

Infos:

This is a one commit change to first see if it makes sense at all.
This currently do not use the 'smart fetch' from pkg/pkg.mk: Avoid doing a git fetch if we already have the commit. #11491
Some parts may be possible to not re-do. Maybe the .prepare should use the content of .git-downloaded instead of the variable.
Did not remove the :: rules but should maybe be the occasion to clean this.

Should git-downloaded be updated only with lazy_sponge to not trigger more rebuild ?
I think .git-prepare must be always re-done as the package makefile may do further modifications afterwards… so not sure it would change anything

TODO:

Change so that packages include pkg/pkg.mk earlier. Some packages use PKG_BUILDDIR before it is defined.

Update to use $(PKG_PREPARED) instead of the .PHONY target.

Testing/Reviewing procedure

Build all packages (lets see CI)
See if it is indeed incremental
See if it detects package deletion
Any ideas on what to test ?

Issues/PRs references

Discussions coming out of #11491

cladmi · 2019-05-20T14:53:59Z

This should address the issue shown in #11129 too as preparing now always checkouts and patch if there are patches.

The fix for #11129 should have be to only re-do .git-downloaded when $(PKG)/Makefile changed as it is the base state file there.

In here, .git-downloaded only takes care of the fetch where checkout is a separate task.

cladmi · 2019-05-20T15:34:20Z

Fixed the fail because of PKG_VERSION = 60ea749837362c226e8501718f505ab138e5c19d # v0.98 that led to PKG_VERSION having a space.

cladmi · 2019-06-03T09:59:51Z

After thinking about it more, I think it would be good to add an intermediate .git-patched and .git-prepared only creating the file. So packages could do the extra modifications between patching and being prepared with a file target of course.

Then all would always depend on .git-prepared which should do nothing as already up to date as done during prepare.

fjmolinas · 2019-10-01T20:15:54Z

This needs a rebase.

fjmolinas · 2019-10-01T20:21:34Z

I did a quick round of testing and this fixed some issue I had where some packages kept beeing recompiled all the time. Does this still need to be wip?

cladmi · 2019-10-02T09:13:40Z

It was still WIP in my mind but I can do another pass.

fjmolinas · 2019-10-02T09:25:13Z

It was still WIP in my mind but I can do another pass.

Well, I'm going to start rebasing my branch's on this most of the time now so I'll be testing continuously :).

And if you need help let me know, I'm very interested in getting this in.

cladmi · 2019-10-02T09:42:00Z

There are things I should split I guess even if they are done before the real fix like moving pkg/pkg.mk earlier in the file as several targets use PKG_BUILDDIR and refactoring.

cladmi · 2019-10-02T09:47:40Z

One thing that I may need help too, is seeing if other packages must be updated too. This takes a lot of time.
But the current documentation may lack things too.

cladmi · 2019-10-02T09:54:02Z

I am also in a place where, either I change the minimal things in a package but keep it in a non reliable state, or I do the work for the migration correctly.
But that requires a lot of cleaning.

cladmi · 2019-10-02T10:57:55Z

Somehow for the implementation I also have a question.

When having a $(PREPARED) file, every targets in a package should depend on it. Currently packages do not do it constantly.

So I do not know, should I do it automatically for all in the pkg/pkg.mk but then, when anybody needs another sub-target, it should discover that this target must also depend of it, it could be documented but different than the all target in the minimal packages or should every package repeat all: $(PKG_STATE_PREPARED) ?

fjmolinas · 2019-10-02T12:11:36Z

One thing that I may need help too, is seeing if other packages must be updated too. This takes a lot of time

I can help with this.

fjmolinas · 2019-10-02T15:03:29Z

Somehow for the implementation I also have a question.

When having a $(PREPARED) file, every targets in a package should depend on it. Currently packages do not do it constantly.

So I do not know, should I do it automatically for all in the pkg/pkg.mk but then, when anybody needs another sub-target, it should discover that this target must also depend of it, it could be documented but different than the all target in the minimal packages or should every package repeat all: $(PKG_STATE_PREPARED) ?

To be honest I got a little confused here with the difference between $(PREPARED), $(PKG_STATE_PREPARED) and what I see in code $(PKG_PREPARED).

What is an example of a "minimal package"?

cladmi · 2019-10-02T15:30:47Z

It's all PKG_PREPARED I put the wrong name in my comment ><

cladmi · 2019-10-02T15:40:47Z

I am thinking that I should refactor to split the git handling from the pkg handling. This way, even if we plan to change the interface, the low level could be correctly abstracted already.

cladmi · 2019-10-04T10:22:46Z

The minimal one is this one:

RIOT/pkg/Makefile.git

Lines 1 to 10 in 7d7596e

    
           PKG_NAME=		# name of the package 
        
           PKG_URL=		# source url of the package's git repository 
        
           PKG_VERSION=	# version of the package to use e.g. a git commit/ref 
        
           .PHONY: all 
        
           all: 
        
           	$(MAKE) -C $(CURDIR)/$(PKG_NAME) 
        
           include $(RIOTBASE)/pkg/pkg.mk

To be consistent, I would move the include at the beginning to have all the variables defined when using the value immediately. Which is already the case that the PKG_BUILDDIR variable is used before it is set right now.

When adding states, the targets would then all need to depend on $(PKG_PREPARED) which I currently did in the pkg/pkg.mk so the basic packages would not even change. (I even added .PHONY: all to remove this one).
So basic packages do not even change. I noticed almost all packages would not change right now. I will move the pkg/pkg.mk before in a separate commit.

Hovewer, if a package needs to do additional steps before compiling, then, these steps would need to depend on $(PKG_PREPARED) to be re-run if the preparation was re-done.

$(PKG_BUILDDIR)/bin/Makefile: $(PKG_PREPARED) $(TOOLCHAIN_FILE) | ..cmake_version_supported
	cmake -B$(PKG_BUILDDIR)/bin -H$(PKG_BUILDDIR)/src \
	      -DCMAKE_TOOLCHAIN_FILE=$(TOOLCHAIN_FILE) \
	      -DCCNL_RIOT=1 -DRIOT_CFLAGS="$(RIOT_CFLAGS)" -DBUILD_TESTING=OFF

But it comes out of nowhere as it was not required for all.

It is a question of balance between, do we make the default simpler but then the different one requiring a lot of new handling, or do we remove the magic and make the dependency explicit in the default case to teach how to handle the more advanced ones.

For the first version I could keep the magic to not modify them too much.

This "ISC License" is the value found by github https://github.com/atomicobject/heatshrink/blob/7d419e1fa4830d0b919b9b6a91fe2fb786cf3280/LICENSE A permissive license lets people do anything with your code with proper attribution and without warranty. The ISC license is functionally equivalent to the BSD 2-Clause and MIT licenses, removing some language that is no longer necessary.

The 'BSD 3-Clause "New" or "Revised" License' is the value found by github https://github.com/openthread/openthread/blob/3a248f649acd16448c7f27fe7f17f6f0bb6696f6/LICENSE > A permissive license similar to the BSD 2-Clause License, but with a 3rd > clause that prohibits others from using the name of the project or its > contributors to promote derived products without written consent.

Ensure the required variables are defined before including 'pkg.mk'.

The file must not change the default goal otherwise it could change packages behavior.

Prepare for handling pkg state with files. So it requires having the path defined before declaring targets.

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

This should prevent issues when the directory is not a git repository which would for example checkout force or clean in the RIOT repository instead.

fjmolinas · 2019-10-18T09:30:22Z

@cladmi from talking IRL I think you have not pushed some chages :)

basilfx · 2020-01-06T22:59:32Z

@cladmi @fjmolinas Created #13036 as an updated version.

fjmolinas · 2020-01-07T18:52:33Z

pkg/nanocbor/Makefile

-.PHONY: all
+include $(RIOTBASE)/pkg/pkg.mk

 all: git-download


Suggested change

all: git-download

all:

fjmolinas · 2020-01-08T21:57:45Z

Closing now that #13036 is in

cladmi added this to the Release 2019.07 milestone May 20, 2019

cladmi requested review from jcarrano and kaspar030 May 20, 2019 14:42

cladmi force-pushed the pr/pkg/save_state_and_dependencies branch from f0e6756 to dd7271e Compare July 8, 2019 18:44

MrKevinWeiss modified the milestones: Release 2019.07, Release 2019.10 Jul 16, 2019

cladmi force-pushed the pr/pkg/save_state_and_dependencies branch from af0baa7 to 8570a6f Compare October 2, 2019 09:21

cladmi force-pushed the pr/pkg/save_state_and_dependencies branch from 8570a6f to 2876d4c Compare October 2, 2019 09:45

fjmolinas mentioned this pull request Oct 8, 2019

examples/suit_update: add ble documentation in README #12391

Merged

cladmi added 5 commits October 11, 2019 20:24

pkg/pkg.mk: add checks for required variables

fca28bb

Ensure the required variables are defined before including 'pkg.mk'.

pkg/pkg.mk: prepare to move include at the beginning

a04bb18

The file must not change the default goal otherwise it could change packages behavior.

pkgs: update to include 'pkg/pkg.mk' at the beginning

e6bb50e

Prepare for handling pkg state with files. So it requires having the path defined before declaring targets.

cladmi force-pushed the pr/pkg/save_state_and_dependencies branch from 2876d4c to 617f671 Compare October 11, 2019 18:31

cladmi added 5 commits October 11, 2019 20:44

pkg/pkg.mk: ensure file is included at the beginning

9b6a3be

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.

WIP: add '.patched' files and to add '.prepared' step

855817e

pkg/pkg.mk: explicit set --git-dir and --work-tree for git

cd6d223

This should prevent issues when the directory is not a git repository which would for example checkout force or clean in the RIOT repository instead.

WIP tests: try updating packages

14e275f

cladmi force-pushed the pr/pkg/save_state_and_dependencies branch from 617f671 to 14e275f Compare October 11, 2019 18:45

fixup! pkg/pkg.mk: explicit set --git-dir and --work-tree for git

0170a3c

This was referenced Jan 5, 2020

pkg: always rebuilt when patches are applied #13030

Closed

pkg: minor improvements to metadata handling #13033

Merged

pkg/pkg.mk: use intermediate state files #13036

Merged

fjmolinas reviewed Jan 7, 2020

View reviewed changes

fjmolinas closed this Jan 8, 2020

Conversation

cladmi commented May 20, 2019

Contribution description

Infos:

TODO:

Testing/Reviewing procedure

Issues/PRs references

Uh oh!

cladmi commented May 20, 2019

Uh oh!

cladmi commented May 20, 2019

Uh oh!

cladmi commented Jun 3, 2019

Uh oh!

fjmolinas commented Oct 1, 2019

Uh oh!

fjmolinas commented Oct 1, 2019

Uh oh!

cladmi commented Oct 2, 2019

Uh oh!

fjmolinas commented Oct 2, 2019

Uh oh!

cladmi commented Oct 2, 2019

Uh oh!

cladmi commented Oct 2, 2019

Uh oh!

cladmi commented Oct 2, 2019

Uh oh!

cladmi commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas commented Oct 2, 2019

Uh oh!

fjmolinas commented Oct 2, 2019

Uh oh!

cladmi commented Oct 2, 2019

Uh oh!

cladmi commented Oct 2, 2019

Uh oh!

cladmi commented Oct 4, 2019

Uh oh!

fjmolinas commented Oct 18, 2019

Uh oh!

basilfx commented Jan 6, 2020

Uh oh!

fjmolinas Jan 7, 2020

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Jan 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cladmi commented Oct 2, 2019 •

edited

Loading