| Message ID | 1428856685-4403-11-git-send-email-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
Thomas, All, On 2015-04-12 18:37 +0200, Thomas Petazzoni spake thusly: > make source-check is here to check whether the remote sources for the > current selection of packages are still available. So it cannot be a > noconfig_targets, since it depends on a configuration being > available. The very fact that 'source-check' is basically the same as > 'source', and one is a noconfig_target and not the other is a clear > indication that the current implementation is wrong. > > So this commit moves source-check to no longer be a noconfig_target. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr> However, I noticed that source-check tries to go to the mirror. For example, cjson fails to download from svn for me here, and it falls back to looking on the mirror, and thus concludes it exists. Shouldn't source-check be limited to looking at the upstream locations? However, not a blocker, since it;s already the behaviour we have. Regards, Yann E. MORIN. > --- > Makefile | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/Makefile b/Makefile > index 40ee2e2..6937dd3 100644 > --- a/Makefile > +++ b/Makefile > @@ -75,7 +75,7 @@ export BR2_VERSION_FULL := $(BR2_VERSION)$(shell $(TOPDIR)/support/scripts/setlo > noconfig_targets := menuconfig nconfig gconfig xconfig config oldconfig randconfig \ > defconfig %_defconfig allyesconfig allnoconfig silentoldconfig release \ > randpackageconfig allyespackageconfig allnopackageconfig \ > - source-check print-version olddefconfig > + print-version olddefconfig > > # Strip quotes and then whitespaces > qstrip = $(strip $(subst ",,$(1))) > @@ -422,7 +422,7 @@ world: target-post-image > > .PHONY: all world toolchain dirs clean distclean source outputmakefile \ > legal-info legal-info-prepare legal-info-clean printvars help \ > - target-finalize target-post-image > + target-finalize target-post-image source-check > > ################################################################################ > # > @@ -616,6 +616,10 @@ _external-deps: $(foreach p,$(PACKAGES),$(p)-all-external-deps) > external-deps: > @$(MAKE1) -Bs $(EXTRAMAKEARGS) _external-deps | sort -u > > +# check if download URLs are outdated > +source-check: > + $(MAKE1) DL_MODE=SOURCE_CHECK $(EXTRAMAKEARGS) source > + > legal-info-clean: > @rm -fr $(LEGAL_INFO_DIR) > > @@ -794,10 +798,6 @@ savedefconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile > --savedefconfig=$(if $(DEFCONFIG),$(DEFCONFIG),$(CONFIG_DIR)/defconfig) \ > $(CONFIG_CONFIG_IN) > > -# check if download URLs are outdated > -source-check: > - $(MAKE1) DL_MODE=SOURCE_CHECK $(EXTRAMAKEARGS) source > - > .PHONY: defconfig savedefconfig > > ################################################################################ > -- > 2.1.0 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Dear Yann E. MORIN, On Mon, 13 Apr 2015 22:49:16 +0200, Yann E. MORIN wrote: > Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > However, I noticed that source-check tries to go to the mirror. For > example, cjson fails to download from svn for me here, and it falls back > to looking on the mirror, and thus concludes it exists. > > Shouldn't source-check be limited to looking at the upstream locations? > > However, not a blocker, since it;s already the behaviour we have. Yes, the behavior you're observing is indeed the current behavior as far as I know, so my patches are not changing this. Indeed, we could discuss whether source-check should check only primary site + upstream site, or primary site + upstream site + sources.b.o. From a BR maintenance point of view, checking only primary site + upstream is probably better as it means we can get notified when an upstream has disappeared. But from a BR user point of view, what's important is that the source code remains available *somewhere*, be it from upstream or sources.b.o. So I'm not very decided on this. Opinions welcome. Thomas
Thomas, All, On 2015-04-13 23:06 +0200, Thomas Petazzoni spake thusly: > On Mon, 13 Apr 2015 22:49:16 +0200, Yann E. MORIN wrote: > > > Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > > > However, I noticed that source-check tries to go to the mirror. For > > example, cjson fails to download from svn for me here, and it falls back > > to looking on the mirror, and thus concludes it exists. > > > > Shouldn't source-check be limited to looking at the upstream locations? > > > > However, not a blocker, since it;s already the behaviour we have. > > Yes, the behavior you're observing is indeed the current behavior as > far as I know, so my patches are not changing this. Indeed, we could > discuss whether source-check should check only primary site + upstream > site, or primary site + upstream site + sources.b.o. > > From a BR maintenance point of view, checking only primary site + > upstream is probably better as it means we can get notified when an > upstream has disappeared. > > But from a BR user point of view, what's important is that the source > code remains available *somewhere*, be it from upstream or sources.b.o. > > So I'm not very decided on this. Opinions welcome. Well, I have a slightly different opinion. First, I agree that for Buildroot maintenance, we only care about upstream, not even primary or backup sites. We do not even have a primary. Second, for a user that wants to be serious, the only thing that would really matter in the end is the existence of the package on the primary site. Let me explain... In an enterprise-grade project, one can not rely on external resources to be always available; one can only rely on internal resources. Thus, in that case, source-check should only look at the primary. However, that primary has to filled in to begin with, and that is often done by just running "make source" and then copying those sources to the primary. If an upstream source is missing, it is the moment one wants to be notified. There's no reason to run a source-check onto upstream, even less so on the mirror. So, in my opinion, source-check should behave as such: - if primary is set, only check on primary - if primary is not set, only check upstream - never check on the mirror That would cover the use-cases above. Regards, Yann E. MORIN.
Yann, On Mon, Apr 13, 2015 at 4:58 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: [...] > Well, I have a slightly different opinion. > > First, I agree that for Buildroot maintenance, we only care about > upstream, not even primary or backup sites. We do not even have a > primary. > > Second, for a user that wants to be serious, the only thing that would > really matter in the end is the existence of the package on the primary > site. > > Let me explain... > > In an enterprise-grade project, one can not rely on external resources > to be always available; one can only rely on internal resources. Thus, > in that case, source-check should only look at the primary. I agree with this statement and would also like to add that they are probably also sitting being a firewall so having the primary site be the only source of downloads helps avoid errors and confusion. > However, that primary has to filled in to begin with, and that is often > done by just running "make source" and then copying those sources to the > primary. If an upstream source is missing, it is the moment one wants to > be notified. There's no reason to run a source-check onto upstream, even > less so on the mirror. > > So, in my opinion, source-check should behave as such: > > - if primary is set, only check on primary > - if primary is not set, only check upstream > - never check on the mirror The only reason I could see someone in large enterprise needing to be able to reach the mirror is if there are behind a firewall where getting an opening to the mirror (sources.b.o) is easier for them to get access to than others. However, I as I typed the above sentence, I thought it would then make more sense for them to just mirror sources.b.o themselves outside of buildroot for a them to create primary site internally. Anyway that is my two-cents on your comments and I don't really have a preference whatever direction this goes. Thanks, -Ryan
On 12/04/15 18:37, Thomas Petazzoni wrote: > make source-check is here to check whether the remote sources for the > current selection of packages are still available. So it cannot be a > noconfig_targets, since it depends on a configuration being > available. The very fact that 'source-check' is basically the same as > 'source', and one is a noconfig_target and not the other is a clear > indication that the current implementation is wrong. Well, actually, source-check is a noconfig target because we don't need to read the .config file to be able to run it. Moving it out of noconfig almost doubles the runtime, because now both the first level and the second level make have to parse all the makefiles. So the real reason to do this is not because it was wrong (just a little weird and missing explanation), but because you need it for three patches later. That said, the change itself looks OK, so Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Regards, Arnout > > So this commit moves source-check to no longer be a noconfig_target. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > Makefile | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/Makefile b/Makefile > index 40ee2e2..6937dd3 100644 > --- a/Makefile > +++ b/Makefile > @@ -75,7 +75,7 @@ export BR2_VERSION_FULL := $(BR2_VERSION)$(shell $(TOPDIR)/support/scripts/setlo > noconfig_targets := menuconfig nconfig gconfig xconfig config oldconfig randconfig \ > defconfig %_defconfig allyesconfig allnoconfig silentoldconfig release \ > randpackageconfig allyespackageconfig allnopackageconfig \ > - source-check print-version olddefconfig > + print-version olddefconfig > > # Strip quotes and then whitespaces > qstrip = $(strip $(subst ",,$(1))) > @@ -422,7 +422,7 @@ world: target-post-image > > .PHONY: all world toolchain dirs clean distclean source outputmakefile \ > legal-info legal-info-prepare legal-info-clean printvars help \ > - target-finalize target-post-image > + target-finalize target-post-image source-check > > ################################################################################ > # > @@ -616,6 +616,10 @@ _external-deps: $(foreach p,$(PACKAGES),$(p)-all-external-deps) > external-deps: > @$(MAKE1) -Bs $(EXTRAMAKEARGS) _external-deps | sort -u > > +# check if download URLs are outdated > +source-check: > + $(MAKE1) DL_MODE=SOURCE_CHECK $(EXTRAMAKEARGS) source > + > legal-info-clean: > @rm -fr $(LEGAL_INFO_DIR) > > @@ -794,10 +798,6 @@ savedefconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile > --savedefconfig=$(if $(DEFCONFIG),$(DEFCONFIG),$(CONFIG_DIR)/defconfig) \ > $(CONFIG_CONFIG_IN) > > -# check if download URLs are outdated > -source-check: > - $(MAKE1) DL_MODE=SOURCE_CHECK $(EXTRAMAKEARGS) source > - > .PHONY: defconfig savedefconfig > > ################################################################################ >
Arnout, All, On 2015-04-14 21:42 +0200, Arnout Vandecappelle spake thusly: > On 12/04/15 18:37, Thomas Petazzoni wrote: > > make source-check is here to check whether the remote sources for the > > current selection of packages are still available. So it cannot be a > > noconfig_targets, since it depends on a configuration being > > available. The very fact that 'source-check' is basically the same as > > 'source', and one is a noconfig_target and not the other is a clear > > indication that the current implementation is wrong. > > Well, actually, source-check is a noconfig target because we don't need to read > the .config file to be able to run it. Maybe I missed something, but don't we need .config to get the list of enabled packages? Hmm.. Let me think.. Ah, I see what you mean. The first-level 'make' does not need it, because its only rule for now is to call itself with the 'source' goal and a variable set. I get it now. > Moving it out of noconfig almost doubles > the runtime, because now both the first level and the second level make have to > parse all the makefiles. > > So the real reason to do this is not because it was wrong (just a little weird > and missing explanation), but because you need it for three patches later. Yes, this change all by itself is meaningless, but we need it later, when source-check is eventually part of the package infra. It seemed obvious during my review, given the goal of the series. For clarification, that could be added to the commit log, indeed. Regards, Yann E. MORIN.
Dear Arnout Vandecappelle, On Tue, 14 Apr 2015 21:42:19 +0200, Arnout Vandecappelle wrote: > Well, actually, source-check is a noconfig target because we don't need to read > the .config file to be able to run it. Moving it out of noconfig almost doubles > the runtime, because now both the first level and the second level make have to > parse all the makefiles. > > So the real reason to do this is not because it was wrong (just a little weird > and missing explanation), but because you need it for three patches later. > > That said, the change itself looks OK, so > > Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Aaah, yes, good catch. make source-check recursively calls into make by calling make source. So the make invocation executing 'source-check' does not need the config file, so a noconfig target is OK. However, the sub-make invocation that executes the 'source' target does need the config file. I think the most correct option here is to squash this change with the change actually modifying the source-check implementation to use the package infrastructure, because as you say it's really why we need source-check to be outside of the noconfig target thing. Which makes me think that 'external-deps' could also be a noconfig target, while _external-deps would need to remain a normal target. But I'm not sure it's really worth the effort and additional "weirdness" of the code. Best regards, Thomas
diff --git a/Makefile b/Makefile index 40ee2e2..6937dd3 100644 --- a/Makefile +++ b/Makefile @@ -75,7 +75,7 @@ export BR2_VERSION_FULL := $(BR2_VERSION)$(shell $(TOPDIR)/support/scripts/setlo noconfig_targets := menuconfig nconfig gconfig xconfig config oldconfig randconfig \ defconfig %_defconfig allyesconfig allnoconfig silentoldconfig release \ randpackageconfig allyespackageconfig allnopackageconfig \ - source-check print-version olddefconfig + print-version olddefconfig # Strip quotes and then whitespaces qstrip = $(strip $(subst ",,$(1))) @@ -422,7 +422,7 @@ world: target-post-image .PHONY: all world toolchain dirs clean distclean source outputmakefile \ legal-info legal-info-prepare legal-info-clean printvars help \ - target-finalize target-post-image + target-finalize target-post-image source-check ################################################################################ # @@ -616,6 +616,10 @@ _external-deps: $(foreach p,$(PACKAGES),$(p)-all-external-deps) external-deps: @$(MAKE1) -Bs $(EXTRAMAKEARGS) _external-deps | sort -u +# check if download URLs are outdated +source-check: + $(MAKE1) DL_MODE=SOURCE_CHECK $(EXTRAMAKEARGS) source + legal-info-clean: @rm -fr $(LEGAL_INFO_DIR) @@ -794,10 +798,6 @@ savedefconfig: $(BUILD_DIR)/buildroot-config/conf outputmakefile --savedefconfig=$(if $(DEFCONFIG),$(DEFCONFIG),$(CONFIG_DIR)/defconfig) \ $(CONFIG_CONFIG_IN) -# check if download URLs are outdated -source-check: - $(MAKE1) DL_MODE=SOURCE_CHECK $(EXTRAMAKEARGS) source - .PHONY: defconfig savedefconfig ################################################################################
make source-check is here to check whether the remote sources for the current selection of packages are still available. So it cannot be a noconfig_targets, since it depends on a configuration being available. The very fact that 'source-check' is basically the same as 'source', and one is a noconfig_target and not the other is a clear indication that the current implementation is wrong. So this commit moves source-check to no longer be a noconfig_target. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- Makefile | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)