[Buildroot] [PATCHv3 19/20] package: package-based implementation of source, external-deps and legal-info
Thomas Petazzoni
thomas.petazzoni at free-electrons.com
Thu Oct 10 21:33:03 UTC 2013
Arnout,
Thanks a lot for your review!
On Thu, 10 Oct 2013 19:24:12 +0200, Arnout Vandecappelle wrote:
> > @@ -310,9 +308,6 @@ endif
> > include package/Makefile.in
> > include support/dependencies/dependencies.mk
> >
>
> You could remove this empty line as well.
Right.
> > -# all targets depend on the crosscompiler and it's prerequisites
> > -$(TARGETS_ALL): __real_tgt_%: $(BASE_TARGETS) %
> > +PACKAGES_CLEAN:=$(patsubst %,%-clean,$(PACKAGES))
>
> Can you make these match the coding style? PACKAGES_CLEAN = ...
Yes, certainly.
> > +PACKAGES_SOURCE:=$(patsubst %,%-all-source,$(PACKAGES) $(BASE_TARGETS))
>
> BASE_TARGETS is just "toolchain", right? And that is already part of
> the dependencies of PACKAGES, right? So why is that still needed here?
Yes, BASE_TARGETS is just toolchain now. However no, "toolchain" is not
yet part of the dependencies of all packages. This is something the
patch series from Fabio Porcedda is doing, and I don't want to solve
all problems in this patch set :)
The main reason I've kept BASE_TARGETS for now is because I knew Fabio
would be cleaning up that further with his patch set.
> > +PACKAGES_EXTERNAL_DEPS:=$(patsubst %,%-all-external-deps,$(PACKAGES) $(BASE_TARGETS))
> > +PACKAGES_DIRCLEAN:=$(patsubst %,%-dirclean,$(PACKAGES))
>
> The PACKAGES_DIRCLEAN is really redundant, it is only used to define
> them as .PHONY but that is not done for e.g. -build.
True, will remove.
> > +PACKAGES_LEGAL_INFO:=$(patsubst %,%-all-legal-info,$(PACKAGES) $(BASE_TARGETS))
> >
> > dirs: $(BUILD_DIR) $(STAGING_DIR) $(TARGET_DIR) \
> > $(HOST_DIR) $(BINARIES_DIR) $(STAMP_DIR)
> > @@ -386,12 +358,13 @@ $(BUILD_DIR)/buildroot-config/auto.conf: $(BUILDROOT_CONFIG)
> >
> > prepare: $(BUILD_DIR)/buildroot-config/auto.conf
> >
> > -world: $(BASE_TARGETS) $(TARGETS_ALL)
> > +world: $(BASE_TARGETS) $(PACKAGES) $(TARGETS)
> >
> > .PHONY: all world toolchain dirs clean distclean source outputmakefile \
> > legal-info legal-info-prepare legal-info-clean printvars \
> > - $(BASE_TARGETS) $(TARGETS) $(TARGETS_ALL) \
> > - $(TARGETS_CLEAN) $(TARGETS_DIRCLEAN) $(TARGETS_SOURCE) $(TARGETS_LEGAL_INFO) \
> > + $(BASE_TARGETS) $(PACKAGES) $(TARGETS) \
> > + $(PACKAGES_CLEAN) $(PACKAGES_DIRCLEAN) $(PACKAGES_SOURCE) $(PACKAGES_LEGAL_INFO) \
> > + $(PACKAGES_EXTERNAL_DEPS) \
> > $(BUILD_DIR) $(STAGING_DIR) $(TARGET_DIR) \
> > $(HOST_DIR) $(BINARIES_DIR) $(STAMP_DIR)
> >
> > @@ -562,10 +535,10 @@ target-post-image:
> > toolchain-eclipse-register:
> > ./support/scripts/eclipse-register-toolchain `readlink -f $(O)` $(notdir $(TARGET_CROSS)) $(BR2_ARCH)
> >
> > -source: dirs $(TARGETS_SOURCE) $(HOST_SOURCE)
> > +source: dirs $(PACKAGES_SOURCE)
> >
> > external-deps:
> > - @$(MAKE) -Bs DL_MODE=SHOW_EXTERNAL_DEPS $(EXTRAMAKEARGS) source | sort -u
> > + @$(MAKE) -s $(EXTRAMAKEARGS) $(PACKAGES_EXTERNAL_DEPS) | sort -u
> >
> > legal-info-clean:
> > @rm -fr $(LEGAL_INFO_DIR)
> > @@ -580,7 +553,7 @@ legal-info-prepare: $(LEGAL_INFO_DIR)
> > @cp $(BUILDROOT_CONFIG) $(LEGAL_INFO_DIR)/buildroot.config
> >
> > legal-info: dirs legal-info-clean legal-info-prepare $(REDIST_SOURCES_DIR) \
> > - $(TARGETS_LEGAL_INFO)
> > + $(PACKAGES_LEGAL_INFO)
> > @cat support/legal-info/README.header >>$(LEGAL_REPORT)
> > @if [ -r $(LEGAL_WARNINGS) ]; then \
> > cat support/legal-info/README.warnings-header \
> > @@ -590,7 +563,7 @@ legal-info: dirs legal-info-clean legal-info-prepare $(REDIST_SOURCES_DIR) \
> > @rm -f $(LEGAL_WARNINGS)
> >
> > show-targets:
> > - @echo $(TARGETS)
> > + @echo $(BASE_TARGETS) $(PACKAGES) $(TARGETS)
> >
> > else # ifeq ($(BR2_HAVE_DOT_CONFIG),y)
> >
> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index a46457c..bd6169c 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -424,6 +424,20 @@ endif
> > $(1)-show-depends:
> > @echo $$($(2)_DEPENDENCIES)
> >
> > +$(1)-all-source: $(foreach p,$($(2)_DEPENDENCIES),$(p)-all-source) $(1)-source
> > +
> > +$(1)-external-deps:
> > +ifneq ($$($(2)_SOURCE),)
> > + @echo $$($(2)_SOURCE)
> > +endif
> > +ifneq ($$($(2)_EXTRA_DOWNLOADS),y)
> > + @echo $$($(2)_EXTRA_DOWNLOADS)
> > +endif
>
> While you're at it, you could add $(2)_PATCH.
Ahh, yes, true. I thought about it at some point, and then forgot.
> Are the double dollars really needed? They're not used in -all-source
> so why would you use them here...
I'll check that, maybe not.
> > -TARGETS += $(1)
> > +PACKAGES += $(1)
> > PACKAGES_PERMISSIONS_TABLE += $$($(2)_PERMISSIONS)$$(sep)
> > PACKAGES_DEVICES_TABLE += $$($(2)_DEVICES)$$(sep)
> > PACKAGES_USERS += $$($(2)_USERS)$$(sep)
>
> Shouldn't you do something similar in fs/ ? Otherwise 'make source'
> will not download e.g. mtd.
That's a weakness of this patch series. I believe I might need to
convert the fs/ stuff to packages (after all they are packages that
have dependencies, and install something in the images/ directory), so
that the legal-info/source/external-deps logic works for them as well,
without doing hacks. What do you think about this?
Best regards,
Thomas
--
Thomas Petazzoni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
More information about the buildroot
mailing list