| Message ID | 1417188306-9035-2-git-send-email-rdkehn@yahoo.com |
|---|---|
| State | Accepted |
| Headers | show |
Doug, All, Sorry to come back to this so late... On 2014-11-28 09:25 -0600, Doug Kehn spake thusly: > Add reinstall targets for host, target, staging, and images variants. > clean-for-reinstall targets added to remove package > .stamp_target_install file to allow package install. Additionally, when > OVERRIDE_SRCDIR is provided, .stamp_rsynced is removed to ensure pakcage > is up to date before reinstalling. > > Signed-off-by: Doug Kehn <rdkehn@yahoo.com> I have some comments below... > --- > package/pkg-generic.mk | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index 9643a30..0fe7059 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -480,34 +480,51 @@ $(1): $(1)-install > > ifeq ($$($(2)_TYPE),host) > $(1)-install: $(1)-install-host > +$(1)-reinstall: $(1)-reinstall-host Indentation here (and in other places in that file) is off, but that's not your fault: the existing code is already borked. > else > $(1)-install: $(1)-install-staging $(1)-install-target $(1)-install-images > +$(1)-reinstall: $(1)-reinstall-staging $(1)-reinstall-target $(1)-reinstall-images > endif > > ifeq ($$($(2)_INSTALL_TARGET),YES) > $(1)-install-target: $$($(2)_TARGET_INSTALL_TARGET) > +$(1)-reinstall-target: $(1)-clean-for-reinstall-target $$($(2)_TARGET_INSTALL_TARGET) > +$(1)-clean-for-reinstall-target:$(1)-clean-for-reinstall > + rm -f $$($(2)_TARGET_INSTALL_TARGET) I am not very happy with the fact that we add new actions here; the rest of the code block is so far only concerned about ordering, not the actual commands to execute. The only other similar make target that removes stuff is the $(1)-dirclean target, that uses a fake stamp-file (that it actually does not touch) to actually does the cleanup. I agree this is a bit convoluted, but I'd prefer we keep the current layout: the code around here gets concerned about ordering, and we actually implement the rules above (around line 248-250), using a fake stamp file (obviously, one for each new -reinstall target). Are you still interested in this? Would you mind looking at that? Again, sorry for the delay in reviewing this series... :-/ Regards, Yann E. MORIN.
Dear Yann E. MORIN, On Sun, 1 Feb 2015 11:55:01 +0100, Yann E. MORIN wrote: > I am not very happy with the fact that we add new actions here; the rest > of the code block is so far only concerned about ordering, not the > actual commands to execute. > > The only other similar make target that removes stuff is the > $(1)-dirclean target, that uses a fake stamp-file (that it actually does > not touch) to actually does the cleanup. I am not sure what you mean here. Look at the existing $(1)-clean-for-rebuild and $(1)-clean-for-reconfigure targets. They also do some commands to remove the stamp files. So what Doug has done here is to simply replicate the same logic, but removing less stamp files to only retrigger installation and not configuration and/or compilation. So I believe the implementation proposed by Doug is well-aligned with what we already do. Unless I misunderstood your comment, obviously. Best regards, Thomas
Thomas, Doug, All, On 2015-02-01 11:59 +0100, Thomas Petazzoni spake thusly: > On Sun, 1 Feb 2015 11:55:01 +0100, Yann E. MORIN wrote: > > I am not very happy with the fact that we add new actions here; the rest > > of the code block is so far only concerned about ordering, not the > > actual commands to execute. > > > > The only other similar make target that removes stuff is the > > $(1)-dirclean target, that uses a fake stamp-file (that it actually does > > not touch) to actually does the cleanup. > > I am not sure what you mean here. Look at the existing > $(1)-clean-for-rebuild and $(1)-clean-for-reconfigure targets. They > also do some commands to remove the stamp files. So what Doug has done > here is to simply replicate the same logic, but removing less stamp > files to only retrigger installation and not configuration and/or > compilation. Ah, indeed, I missed it as it was below the code touched for the reinstall targets. > So I believe the implementation proposed by Doug is well-aligned with > what we already do. Unless I misunderstood your comment, obviously. No, that's Ok, I did not look further than the part actually touched by the patch. So, obviously, I withdraw my comment. Regards, Yann E. MORIN.
Dear Doug Kehn, On Fri, 28 Nov 2014 09:25:04 -0600, Doug Kehn wrote: > Add reinstall targets for host, target, staging, and images variants. > clean-for-reinstall targets added to remove package > .stamp_target_install file to allow package install. Additionally, when > OVERRIDE_SRCDIR is provided, .stamp_rsynced is removed to ensure pakcage > is up to date before reinstalling. > > Signed-off-by: Doug Kehn <rdkehn@yahoo.com> > --- > package/pkg-generic.mk | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) Thanks, we applied your patch, but with a much simplified version. Since having <pkg>-reinstall-{images,host,staging,target} was probably not necessary, we just implemented one <pkg>-reinstall target, that reinstalls whatever the package wants to install. This allows to make the implementation a lot more similar to the <pkg>-rebuild and <pkg>-reconfigure implementations. See http://git.buildroot.net/buildroot/commit/?id=b7bc44d22d0fddde7c964a2fd259775ea8d0bf9f for the final commit. Thanks! Thomas
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index 9643a30..0fe7059 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -480,34 +480,51 @@ $(1): $(1)-install ifeq ($$($(2)_TYPE),host) $(1)-install: $(1)-install-host +$(1)-reinstall: $(1)-reinstall-host else $(1)-install: $(1)-install-staging $(1)-install-target $(1)-install-images +$(1)-reinstall: $(1)-reinstall-staging $(1)-reinstall-target $(1)-reinstall-images endif ifeq ($$($(2)_INSTALL_TARGET),YES) $(1)-install-target: $$($(2)_TARGET_INSTALL_TARGET) +$(1)-reinstall-target: $(1)-clean-for-reinstall-target $$($(2)_TARGET_INSTALL_TARGET) +$(1)-clean-for-reinstall-target:$(1)-clean-for-reinstall + rm -f $$($(2)_TARGET_INSTALL_TARGET) $$($(2)_TARGET_INSTALL_TARGET): $$($(2)_TARGET_BUILD) else $(1)-install-target: +$(1)-reinstall-target: endif ifeq ($$($(2)_INSTALL_STAGING),YES) $(1)-install-staging: $$($(2)_TARGET_INSTALL_STAGING) +$(1)-reinstall-staging: $(1)-clean-for-reinstall-staging $$($(2)_TARGET_INSTALL_STAGING) +$(1)-clean-for-reinstall-staging: $(1)-clean-for-reinstall + rm -f $$($(2)_TARGET_INSTALL_STAGING) $$($(2)_TARGET_INSTALL_STAGING): $$($(2)_TARGET_BUILD) # Some packages use install-staging stuff for install-target $$($(2)_TARGET_INSTALL_TARGET): $$($(2)_TARGET_INSTALL_STAGING) else $(1)-install-staging: +$(1)-reinstall-staging: endif ifeq ($$($(2)_INSTALL_IMAGES),YES) $(1)-install-images: $$($(2)_TARGET_INSTALL_IMAGES) +$(1)-reinstall-images: $(1)-clean-for-reinstall-images $$($(2)_TARGET_INSTALL_IMAGES) +$(1)-clean-for-reinstall-images:$(1)-clean-for-reinstall + rm -f $$($(2)_TARGET_INSTALL_IMAGES) $$($(2)_TARGET_INSTALL_IMAGES): $$($(2)_TARGET_BUILD) else $(1)-install-images: +$(1)-reinstall-images: endif $(1)-install-host: $$($(2)_TARGET_INSTALL_HOST) +$(1)-reinstall-host: $(1)-clean-for-reinstall-host $$($(2)_TARGET_INSTALL_HOST) +$(1)-clean-for-reinstall-host: $(1)-clean-for-reinstall + rm -f $$($(2)_TARGET_INSTALL_HOST) $$($(2)_TARGET_INSTALL_HOST): $$($(2)_TARGET_BUILD) $(1)-build: $$($(2)_TARGET_BUILD) @@ -545,6 +562,8 @@ $$($(2)_TARGET_EXTRACT): $$($(2)_TARGET_SOURCE) $(1)-depends: $$($(2)_FINAL_DEPENDENCIES) $(1)-source: $$($(2)_TARGET_SOURCE) + +$(1)-clean-for-reinstall: else # In the package override case, the sequence of steps # source, by rsyncing @@ -563,6 +582,9 @@ $(1)-extract: $(1)-rsync $(1)-rsync: $$($(2)_TARGET_RSYNC) $(1)-source: $$($(2)_TARGET_RSYNC_SOURCE) + +$(1)-clean-for-reinstall: + rm -f $$($(2)_TARGET_RSYNC) endif $(1)-show-depends:
Add reinstall targets for host, target, staging, and images variants. clean-for-reinstall targets added to remove package .stamp_target_install file to allow package install. Additionally, when OVERRIDE_SRCDIR is provided, .stamp_rsynced is removed to ensure pakcage is up to date before reinstalling. Signed-off-by: Doug Kehn <rdkehn@yahoo.com> --- package/pkg-generic.mk | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)