| Message ID | 20230818151608.988723-1-bernd@kuhls.net |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] package/pkg-generic.mk: fix typo in make target .stamp_built | expand |
On Fri, 18 Aug 2023 17:16:08 +0200 Bernd Kuhls <bernd@kuhls.net> wrote: > This fix can be backported because the offending line was added in 2009. > > Signed-off-by: Bernd Kuhls <bernd@kuhls.net> > --- > 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 5d1c1da128..9d40754939 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -286,7 +286,7 @@ $(BUILD_DIR)/%/.stamp_configured: > $(Q)touch $@ > > # Build > -$(BUILD_DIR)/%/.stamp_built:: > +$(BUILD_DIR)/%/.stamp_built: Note that double colon rules have a specific meaning in make: https://www.gnu.org/software/make/manual/html_node/Double_002dColon.html I've read a few times the page, but I'm not sure I fully grasp what it says, so I don't know if the double colon was intended (and needed at the time) or was a typo. Thomas
On 2023-08-18 22:03 +0200, Thomas Petazzoni via buildroot spake thusly: > On Fri, 18 Aug 2023 17:16:08 +0200 > Bernd Kuhls <bernd@kuhls.net> wrote: > > > This fix can be backported because the offending line was added in 2009. What is actualy broken? As you noticed, the does has been there for moer than 14 years now, so if something obvious was broken, we'd have notriced by now. So, if this is broken, it is not obvious, and we need to udnerstand how it is broken and why the fix is correct. Regards, Yann E. MORIN. > > Signed-off-by: Bernd Kuhls <bernd@kuhls.net> > > --- > > 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 5d1c1da128..9d40754939 100644 > > --- a/package/pkg-generic.mk > > +++ b/package/pkg-generic.mk > > @@ -286,7 +286,7 @@ $(BUILD_DIR)/%/.stamp_configured: > > $(Q)touch $@ > > > > # Build > > -$(BUILD_DIR)/%/.stamp_built:: > > +$(BUILD_DIR)/%/.stamp_built: > > Note that double colon rules have a specific meaning in make: > > https://www.gnu.org/software/make/manual/html_node/Double_002dColon.html > > I've read a few times the page, but I'm not sure I fully grasp what it > says, so I don't know if the double colon was intended (and needed at > the time) or was a typo. > > Thomas > -- > Thomas Petazzoni, co-owner and CEO, Bootlin > Embedded Linux and Kernel engineering and training > https://bootlin.com > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
On Fri, 18 Aug 2023 22:24:34 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > On 2023-08-18 22:03 +0200, Thomas Petazzoni via buildroot spake thusly: > > On Fri, 18 Aug 2023 17:16:08 +0200 > > Bernd Kuhls <bernd@kuhls.net> wrote: > > > > > This fix can be backported because the offending line was added in 2009. > > What is actualy broken? > > As you noticed, the does has been there for moer than 14 years now, so > if something obvious was broken, we'd have notriced by now. > > So, if this is broken, it is not obvious, and we need to udnerstand how > it is broken and why the fix is correct. I don't think Bernd claimed anywhere that something is broken. He claims there is a typo. I've also seen this double colon several times, and it isn't consistent across the other similar make targets, and I always wondered if it was needed or not. If it is not needed, we should remove it to make this rule consistent with others. If it's needed, it should be explained in a comment above it because it's not obvious what's going on. Thomas
Hi Thomas, Am Fri, 18 Aug 2023 22:03:14 +0200 schrieb Thomas Petazzoni via buildroot: > Note that double colon rules have a specific meaning in make: ah, I did not know that. > so I don't know if the double colon was intended (and needed at > the time) or was a typo. Digging further into what first appeared to me as a typo I found your commit adding the double colon to package/Makefile.package.in: https://git.buildroot.net/buildroot/commit/package? id=e11fe847b2f545446fc3300dd2ba88fd5da05756 At that time there was another file, package/Makefile.autotools.in, which also included a "$(BUILD_DIR)/%/.stamp_built:" single-colon target: https://git.buildroot.net/buildroot/tree/package/Makefile.autotools.in? id=e11fe847b2f545446fc3300dd2ba88fd5da05756#n264 Today only one file, package/pkg-generic.mk, contains a .stamp_built target. Maybe, I am only guessing, the double colon was necessary back then, due to two Makefiles containing the same target, and lost their justification when the single-colon target "$(BUILD_DIR)/%/.stamp_built:" was removed with commit https://git.buildroot.net/buildroot/commit/? id=d8b55b99095e5eed46d2b95bdaab2e69ccbd7170 Regards, Bernd
Thomas, All, On 2023-08-18 22:44 +0200, Thomas Petazzoni via buildroot spake thusly: > On Fri, 18 Aug 2023 22:24:34 +0200 > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > > On 2023-08-18 22:03 +0200, Thomas Petazzoni via buildroot spake thusly: > > > On Fri, 18 Aug 2023 17:16:08 +0200 > > > Bernd Kuhls <bernd@kuhls.net> wrote: > > > > > > > This fix can be backported because the offending line was added in 2009. > > What is actualy broken? > > As you noticed, the does has been there for moer than 14 years now, so > > if something obvious was broken, we'd have notriced by now. > > So, if this is broken, it is not obvious, and we need to udnerstand how > > it is broken and why the fix is correct. > > I don't think Bernd claimed anywhere that something is broken. Right. But if that had just been a typo, then that would have been invalid Makefile syntax, which it obviously is not, as this code does not trigger an error from make. So it is not a typo (so, see below). > He > claims there is a typo. I've also seen this double colon several times, > and it isn't consistent across the other similar make targets, and I > always wondered if it was needed or not. If it is not needed, we should > remove it to make this rule consistent with others. If it's needed, it > should be explained in a comment above it because it's not obvious > what's going on. Exactly. You expressed the same reasoning I was trying to convey, but which I failed to express properly. But I agree that either wway, needed or not, the explanations are going to be... difficult to come up with, too. As an aside, we use that :: construct in one other place: support/kconfig/Makefile.br:42:$(obj)/%:: $(src)/%_shipped which is something we inherited from the 2.6.21.5 kernel in a665ed34960bf1423cba303276161bf44a9851f4... No need to fix that one. Regards, Yann E. MORIN.
On Fri, 18 Aug 2023 22:47:15 +0200 Bernd Kuhls <bernd@kuhls.net> wrote: > Digging further into what first appeared to me as a typo I found your > commit adding the double colon to package/Makefile.package.in: > https://git.buildroot.net/buildroot/commit/package? > id=e11fe847b2f545446fc3300dd2ba88fd5da05756 > > At that time there was another file, package/Makefile.autotools.in, which > also included a "$(BUILD_DIR)/%/.stamp_built:" single-colon target: > https://git.buildroot.net/buildroot/tree/package/Makefile.autotools.in? > id=e11fe847b2f545446fc3300dd2ba88fd5da05756#n264 > > Today only one file, package/pkg-generic.mk, contains a .stamp_built > target. > > Maybe, I am only guessing, the double colon was necessary back then, due > to two Makefiles containing the same target, and lost their justification > when the single-colon target "$(BUILD_DIR)/%/.stamp_built:" was removed > with commit https://git.buildroot.net/buildroot/commit/? > id=d8b55b99095e5eed46d2b95bdaab2e69ccbd7170 Thanks for the additional research. However, I don't think your explanation that the double colon was necessary back then because there was a $(BUILD_DIR)/%/.stamp_built rule in package/Makefile.package.in and another one in package/Makefile.autotools.in really makes sense: this was also the case for all the other $(BUILD_DIR)/%/.stamp_<something> rules, and only the .stamp_built one had this double colon added. I can really only think about this really being a typo. Here is a highly convincing explanation: up to a few days ago before you sent this patch, I had no idea what this "double colon" was doing exactly. And I really doubt that back in 2009 when I contributed this change I had a much better idea about it. So I don't see how it could have been intentional from me, and therefore I believe it indeed is a typo. I looked up the history of this patch, and it was posted only once to the mailing list by me, I then did a pull request to Peter, who merged the whole thing. So there was no particular discussion about this double colon. So, unless Yann disagrees, I think I'm going to apply Bernd's patch with a slightly more detailed explanation. Thomas
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index 5d1c1da128..9d40754939 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -286,7 +286,7 @@ $(BUILD_DIR)/%/.stamp_configured: $(Q)touch $@ # Build -$(BUILD_DIR)/%/.stamp_built:: +$(BUILD_DIR)/%/.stamp_built: @$(call step_start,build) @$(call MESSAGE,"Building") $(foreach hook,$($(PKG)_PRE_BUILD_HOOKS),$(call $(hook))$(sep))
This fix can be backported because the offending line was added in 2009. Signed-off-by: Bernd Kuhls <bernd@kuhls.net> --- package/pkg-generic.mk | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)