| Message ID | 20210621141130.48654-5-herve.codina@bootlin.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Overwritten file detection and fixes, one more step to TLP build | expand |
On Mon, 21 Jun 2021 16:11:19 +0200 Herve Codina <herve.codina@bootlin.com> wrote: > During per-package build, original .la files are modified by > fixup-libtool-files calls. > But since fixup-libtool-files modifies files using sed --in-place, > these modification are done using a temporary file and a call to > rename. Rename breaks the hardlink to the original file and leave the > temporary file in per-package TARGET dir. > As the original file is not modified, this is no longer considered as > an overwrite. > > To fix this detection, this patch simply considers the what is done "the what" -> "that what" > by fixup-libtool-files is part of the original snapshot used to > detect overwrites. And so, the original snapshot is taken after > fixup-libtool-files call. > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> With that: Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> Thomas
On 2021-06-21 16:11 +0200, Herve Codina spake thusly: > During per-package build, original .la files are modified by > fixup-libtool-files calls. > But since fixup-libtool-files modifies files using sed --in-place, > these modification are done using a temporary file and a call to > rename. Rename breaks the hardlink to the original file and leave the > temporary file in per-package TARGET dir. > As the original file is not modified, this is no longer considered as > an overwrite. > > To fix this detection, this patch simply considers the what is done > by fixup-libtool-files is part of the original snapshot used to > detect overwrites. And so, the original snapshot is taken after > fixup-libtool-files call. Then this should be squashed together with the first patch, to avoid introducing the issue just to fix it a few patches down the series. You should however add a note about that in the commit log of the first patch, of course, to explain why the overwrite ifnra is inserted after the .la tweaks. So, I agree with the explanations, which make sense, but I disagree that it should be a separate patch... Regards, Yann E. MORIN. > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > --- > package/pkg-generic.mk | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index 2499c94746..f9564831cc 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -254,9 +254,9 @@ $(BUILD_DIR)/%/.stamp_configured: > @$(call pkg_size_before,$(TARGET_DIR)) > @$(call pkg_size_before,$(STAGING_DIR),-staging) > @$(call pkg_size_before,$(HOST_DIR),-host) > + $(call fixup-libtool-files,$(NAME),$(STAGING_DIR)) > @$(call pkg_detect_overwrite_before,$(TARGET_DIR)) > @$(call pkg_detect_overwrite_before,$(HOST_DIR),-host) > - $(call fixup-libtool-files,$(NAME),$(STAGING_DIR)) > $(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep)) > $($(PKG)_CONFIGURE_CMDS) > $(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep)) > -- > 2.31.1 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hi, On Mon, 21 Jun 2021 23:42:23 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > On 2021-06-21 16:11 +0200, Herve Codina spake thusly: > > During per-package build, original .la files are modified by > > fixup-libtool-files calls. > > But since fixup-libtool-files modifies files using sed --in-place, > > these modification are done using a temporary file and a call to > > rename. Rename breaks the hardlink to the original file and leave the > > temporary file in per-package TARGET dir. > > As the original file is not modified, this is no longer considered as > > an overwrite. > > > > To fix this detection, this patch simply considers the what is done > > by fixup-libtool-files is part of the original snapshot used to > > detect overwrites. And so, the original snapshot is taken after > > fixup-libtool-files call. > > Then this should be squashed together with the first patch, to avoid > introducing the issue just to fix it a few patches down the series. > > You should however add a note about that in the commit log of the first > patch, of course, to explain why the overwrite ifnra is inserted after > the .la tweaks. > > So, I agree with the explanations, which make sense, but I disagree that > it should be a separate patch... > Well, I have seen this when I created the patches. I kept them separate because on the first patch, I introduced the tool to check the overwrites and i would like it to take its snapshot as soon as possible in the build sequence (ie right after collecting dependencies files and taking snapshots for current package statistics). Then I fixed the issue seen by the overwrites detection and I put at the same level fixing host-e2fsprogs, fixing .la files or a bit later fixing python with one patch per fix to detail (or try to detail) the issue and the way I fixed it. Squashing the 2 patches leads to one patch that introduces the tool and fixes one of the issues detected by the tool. What about the others issues detected ? Squash also together with the first patch ? I think it will produce a huge patch quite complicate to understand even with all individual commit message squashed. However, that being said, I can squash this patch (Fix .la files overwrite detection) with the 1st one (detect files overwritten in TARGET_DIR and HOST_DIR) if you still think it will be better. Regards, Hervé Codina
Hervé, All, On 2021-06-22 11:31 +0200, Herve Codina spake thusly: > On Mon, 21 Jun 2021 23:42:23 +0200 > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > > On 2021-06-21 16:11 +0200, Herve Codina spake thusly: > > > During per-package build, original .la files are modified by > > > fixup-libtool-files calls. > > > But since fixup-libtool-files modifies files using sed --in-place, > > > these modification are done using a temporary file and a call to > > > rename. Rename breaks the hardlink to the original file and leave the > > > temporary file in per-package TARGET dir. > > > As the original file is not modified, this is no longer considered as > > > an overwrite. > > > > > > To fix this detection, this patch simply considers the what is done > > > by fixup-libtool-files is part of the original snapshot used to > > > detect overwrites. And so, the original snapshot is taken after > > > fixup-libtool-files call. > > Then this should be squashed together with the first patch, to avoid > > introducing the issue just to fix it a few patches down the series. > > > > You should however add a note about that in the commit log of the first > > patch, of course, to explain why the overwrite ifnra is inserted after > > the .la tweaks. > > > > So, I agree with the explanations, which make sense, but I disagree that > > it should be a separate patch... > > > Well, I have seen this when I created the patches. > I kept them separate because on the first patch, I introduced the tool > to check the overwrites and i would like it to take its snapshot as soon > as possible in the build sequence (ie right after collecting dependencies > files and taking snapshots for current package statistics). > Then I fixed the issue seen by the overwrites detection and I put at the > same level fixing host-e2fsprogs, fixing .la files or a bit later fixing > python with one patch per fix to detail (or try to detail) the issue and > the way I fixed it. But really, it *is* the first patch that introduces the issue, so it should be fixed from the onset, rather than after-the-fact. > Squashing the 2 patches leads to one patch that introduces the tool and > fixes one of the issues detected by the tool. Sorry, but this patch (4/15) is fixing an issue introduced by the first patch, with: @$(call pkg_size_before,$(TARGET_DIR)) @$(call pkg_size_before,$(STAGING_DIR),-staging) @$(call pkg_size_before,$(HOST_DIR),-host) + @$(call pkg_detect_overwrite_before,$(TARGET_DIR)) + @$(call pkg_detect_overwrite_before,$(HOST_DIR),-host) $(call fixup-libtool-files,$(NAME),$(STAGING_DIR)) $(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep)) $($(PKG)_CONFIGURE_CMDS) ... when it should directly be: @$(call pkg_size_before,$(STAGING_DIR),-staging) @$(call pkg_size_before,$(HOST_DIR),-host) $(call fixup-libtool-files,$(NAME),$(STAGING_DIR)) + @$(call pkg_detect_overwrite_before,$(TARGET_DIR)) + @$(call pkg_detect_overwrite_before,$(HOST_DIR),-host) $(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep)) $($(PKG)_CONFIGURE_CMDS) $(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep)) And as I said, the commit log should explain why the 'overwrite' calls are inserted after the .la fixup. > What about the others issues > detected ? Squash also together with the first patch ? I think it will > produce a huge patch quite complicate to understand even with all individual > commit message squashed. Ideally, I would say a series should first fix the issues, then introduce the tooling. Otherwise, if only the first patch(es) are applied, the tree is broken: indeed the tooling has been applied, and thus is used, but the issues are still there. Also, it is usually easier and less controversial to get fixes applied, than new tooling. > However, that being said, I can squash this patch (Fix .la files overwrite > detection) with the 1st one (detect files overwritten in TARGET_DIR and > HOST_DIR) if you still think it will be better. Yes, I still think that it is better. Regards, Yann E. MORIN.
On Tue, 22 Jun 2021 11:56:09 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > Sorry, but this patch (4/15) is fixing an issue introduced by the first > patch, with: I agree in principle. But here, it's really a new requirement that we are adding on packages, and we *know* that even with Hervé patches there will be some other packages that will need fixing. So to me it's not really like introducing a regression in a patch, and fixing it later. > > What about the others issues > > detected ? Squash also together with the first patch ? I think it will > > produce a huge patch quite complicate to understand even with all individual > > commit message squashed. > > Ideally, I would say a series should first fix the issues, then > introduce the tooling. No, please keep one series, but having the fixes *before* we introduce the check. And as stated above, the fixes will not fix all problems, they will only fix the problems we know about. More problems will be detected by the autobuilders thanks to the overwrite check. > > However, that being said, I can squash this patch (Fix .la files overwrite > > detection) with the 1st one (detect files overwritten in TARGET_DIR and > > HOST_DIR) if you still think it will be better. > > Yes, I still think that it is better. No, please don't squash, have fixes added before the overwrite check instead. Thomas
Thomas, All, On 2021-06-22 12:12 +0200, Thomas Petazzoni spake thusly: > On Tue, 22 Jun 2021 11:56:09 +0200 > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > > Sorry, but this patch (4/15) is fixing an issue introduced by the first > > patch, with: > I agree in principle. But here, it's really a new requirement that we > are adding on packages, and we *know* that even with Hervé patches > there will be some other packages that will need fixing. Sorry, but this patch is fixing the infra, not the packages. > So to me it's not really like introducing a regression in a patch, and > fixing it later. I never said it was a regression. ;-) > > > What about the others issues > > > detected ? Squash also together with the first patch ? I think it will > > > produce a huge patch quite complicate to understand even with all individual > > > commit message squashed. > > > > Ideally, I would say a series should first fix the issues, then > > introduce the tooling. > > No, please keep one series, but having the fixes *before* we introduce > the check. Exactly; I never said to split the series into two series. It should still be one. > And as stated above, the fixes will not fix all problems, > they will only fix the problems we know about. More problems will be > detected by the autobuilders thanks to the overwrite check. And this is totally OK. [0] > > > However, that being said, I can squash this patch (Fix .la files overwrite > > > detection) with the 1st one (detect files overwritten in TARGET_DIR and > > > HOST_DIR) if you still think it will be better. > > > > Yes, I still think that it is better. > > No, please don't squash, have fixes added before the overwrite check > instead. Well, I still disagree, because this patch really fixes an issue introduced *in the infra* by the first patch. Regards, Yann E. MORIN. [0] but brace for impact! ;-] > Thomas > -- > Thomas Petazzoni, co-owner and CEO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Mon, 21 Jun 2021 22:54:11 +0200 Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > On Mon, 21 Jun 2021 16:11:19 +0200 > Herve Codina <herve.codina@bootlin.com> wrote: > > > During per-package build, original .la files are modified by > > fixup-libtool-files calls. > > But since fixup-libtool-files modifies files using sed --in-place, > > these modification are done using a temporary file and a call to > > rename. Rename breaks the hardlink to the original file and leave the > > temporary file in per-package TARGET dir. > > As the original file is not modified, this is no longer considered as > > an overwrite. > > > > To fix this detection, this patch simply considers the what is done > > "the what" -> "that what" Fixed. > > > by fixup-libtool-files is part of the original snapshot used to > > detect overwrites. And so, the original snapshot is taken after > > fixup-libtool-files call. > > > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > > With that: > > Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> Thanks for this first review. Hervé
Hi, On Tue, 22 Jun 2021 12:30:39 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > On 2021-06-22 12:12 +0200, Thomas Petazzoni spake thusly: > > On Tue, 22 Jun 2021 11:56:09 +0200 > > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > > > Sorry, but this patch (4/15) is fixing an issue introduced by the first > > > patch, with: > > I agree in principle. But here, it's really a new requirement that we > > are adding on packages, and we *know* that even with Hervé patches > > there will be some other packages that will need fixing. > > Sorry, but this patch is fixing the infra, not the packages. > > > So to me it's not really like introducing a regression in a patch, and > > fixing it later. > > I never said it was a regression. ;-) > > > > > What about the others issues > > > > detected ? Squash also together with the first patch ? I think it will > > > > produce a huge patch quite complicate to understand even with all individual > > > > commit message squashed. > > > > > > Ideally, I would say a series should first fix the issues, then > > > introduce the tooling. > > > > No, please keep one series, but having the fixes *before* we introduce > > the check. > > Exactly; I never said to split the series into two series. It should > still be one. > > > And as stated above, the fixes will not fix all problems, > > they will only fix the problems we know about. More problems will be > > detected by the autobuilders thanks to the overwrite check. > > And this is totally OK. [0] > > > > > However, that being said, I can squash this patch (Fix .la files overwrite > > > > detection) with the 1st one (detect files overwritten in TARGET_DIR and > > > > HOST_DIR) if you still think it will be better. > > > > > > Yes, I still think that it is better. > > > > No, please don't squash, have fixes added before the overwrite check > > instead. > > Well, I still disagree, because this patch really fixes an issue > introduced *in the infra* by the first patch. > Well, my bad. I think that the better thing to do is to fully rework the history. First fixing overwrites and then introducing the overwrite detection tooling (ie the actual PATCH 1 will be moved just before actual PATCH 12 and so actual PATCH 4 simply disappears). The patch introducing the tooling will explain in its commit message why the calls to fixup-libtool-files and fixup-python-files are performed before taking the overwrite snapshot and why it is safe (sed --in-place). The variable <PKG>_PER_PACKAGE_TWEAK_HOOKS (or other name) will be introduced before the tooling. The patches changing some packages to use this variable (move tweaks from <PKG>_POST_CONFIGURE_HOOKS to <PKG>_PER_PACKAGE_TWEAK_HOOKS) will also be introduced before the tooling even if these changes really make sense after the tooling introduction. Indeed, the notion of action done before taking the overwrite detection snapshot (<PKG>_PER_PACKAGE_TWEAK_HOOKS) and action done after (<PKG>_PRE_CONFIGURE_HOOKS, configure and <PKG>_POST_CONFIGURE_HOOKS) makes sense only with the overwrite detection tool. Is that seem better for everyone ? Regards, Hervé
Hervé, All, On 2021-06-24 17:44 +0200, Herve Codina spake thusly: > On Tue, 22 Jun 2021 12:30:39 +0200 > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: [--SNIP--] > > Well, I still disagree, because this patch really fixes an issue > > introduced *in the infra* by the first patch. > Well, my bad. No worries, really! This series is quite the complex one. ;-) > I think that the better thing to do is to fully rework the history. > First fixing overwrites and then introducing the overwrite detection tooling > (ie the actual PATCH 1 will be moved just before actual PATCH 12 and so > actual PATCH 4 simply disappears). > > The patch introducing the tooling will explain in its commit message > why the calls to fixup-libtool-files and fixup-python-files are performed > before taking the overwrite snapshot and why it is safe (sed --in-place). Exactly! :-) > The variable <PKG>_PER_PACKAGE_TWEAK_HOOKS (or other name) will be introduced > before the tooling. The patches changing some packages to use this variable > (move tweaks from <PKG>_POST_CONFIGURE_HOOKS to <PKG>_PER_PACKAGE_TWEAK_HOOKS) > will also be introduced before the tooling even if these changes really make > sense after the tooling introduction. > Indeed, the notion of action done before taking the overwrite detection > snapshot (<PKG>_PER_PACKAGE_TWEAK_HOOKS) and action done after > (<PKG>_PRE_CONFIGURE_HOOKS, configure and <PKG>_POST_CONFIGURE_HOOKS) > makes sense only with the overwrite detection tool. > > Is that seem better for everyone ? It looks a sane ordering to me, yes. THanks for working on this! Regards, Yann E. MORIN.
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index 2499c94746..f9564831cc 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -254,9 +254,9 @@ $(BUILD_DIR)/%/.stamp_configured: @$(call pkg_size_before,$(TARGET_DIR)) @$(call pkg_size_before,$(STAGING_DIR),-staging) @$(call pkg_size_before,$(HOST_DIR),-host) + $(call fixup-libtool-files,$(NAME),$(STAGING_DIR)) @$(call pkg_detect_overwrite_before,$(TARGET_DIR)) @$(call pkg_detect_overwrite_before,$(HOST_DIR),-host) - $(call fixup-libtool-files,$(NAME),$(STAGING_DIR)) $(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep)) $($(PKG)_CONFIGURE_CMDS) $(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))
During per-package build, original .la files are modified by fixup-libtool-files calls. But since fixup-libtool-files modifies files using sed --in-place, these modification are done using a temporary file and a call to rename. Rename breaks the hardlink to the original file and leave the temporary file in per-package TARGET dir. As the original file is not modified, this is no longer considered as an overwrite. To fix this detection, this patch simply considers the what is done by fixup-libtool-files is part of the original snapshot used to detect overwrites. And so, the original snapshot is taken after fixup-libtool-files call. Signed-off-by: Herve Codina <herve.codina@bootlin.com> --- package/pkg-generic.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)