| Message ID | 20211125124844.75d8dd2c@camb691.localdomain |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [RFC,v2] Handle _LICENSE_FILES with full path | expand |
On 25/11/2021 13:48, Cyril Bur wrote: > legal-info currently assumes that a packages _LICENSE_FILES will be a > path or a filename relative to the source of the package. > > There is nothing preventing _LICENSE_FILES from containing a full path. > When this happens, the legal-info process adds the build directory on > the front of the _LICENSE_FILES value resulting in a failed cp of the > file. > > This patch adds a check to see if the _LICENSE_FILES path with the build > directory prepended exists (as is the case for all packages currently) > and uses that. Otherwise it will assume that the _LICENSE_FILES file is > a full path. > > An 'offending' package .mk file could look something like this: > LIBFOO_VERSION = 1.0 > LIBFOO_SOURCE = libfoo-$(LIBFOO_VERSION).tar.gz > LIBFOO_SITE = http://www.foosoftware.org/download > LIBFOO_LICENSE = GPL-3.0+ > LIBFOO_LICENSE_FILES = $(LIBFOO_PKGDIR)/COPYING Note that this happens to work for an external, because then PKGDIR expands to an absolute path, but for internal packages it's a relative path. So your patch is not going to work there. The proper way of doing this is define LIBFOO_COPY_LICENSE_FILE cp $(LIBFOO_PKGDIR)/COPYING $(LIBFOO_SRCDIR)/COPYING endef LIBFOO_POST_EXTRACT_HOOKS += LIBFOO_COPY_LICENSE_FILE Yes, it's a bit involved to write all that, but this kind of situation should be pretty exceptional. If it would start to happen in a lot of cases (i.e. if we start providing license files for packages that don't have any of their own), then we could add infra for it, e.g. LIBFOO_EXTERNAL_LICENSE_FILES (which looks for the license files in PKGDIR). However, I'm not so sure if it's such a good idea for buildroot itself to carry license files for projects that don't have any. If the project doesn't have any license file, not even a header file or something that has to be extracted explicitly (cfr. e.g. gcnano-binaries), then I don't think we should fake one either. It would give that file a veneer of authority which is inappropriate IMHO. Better make it clear to the user that that particular package is not to be trusted, license-wise. Therefore, I've marked this patch as Rejected in patchwork. Regards, Arnout > > [...] > > $(eval $(generic-package)) > > Signed-off-by: Cyril Bur <cyrilbur@gmail.com> > --- > Adding a licence file to buildroot is probably odd and a strange thing > to do, we're doing this internally (and I do hope I can push the package > soon) with https://github.com/BrianGladman/modes > > V1->V2 Reworked commit message and added example as requested by Thomas > Petazzoni > package/pkg-generic.mk | 2 +- > package/pkg-utils.mk | 4 ++++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index ded5176428..e6d9f457e4 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -1139,7 +1139,7 @@ ifneq ($$(call qstrip,$$($(2)_SOURCE)),) > ifeq ($$(call qstrip,$$($(2)_LICENSE_FILES)),) > $(Q)$$(call legal-warning-pkg,$$($(2)_BASENAME_RAW),cannot save license ($(2)_LICENSE_FILES not defined)) > else > - $(Q)$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAWNAME),$$($(2)_BASENAME_RAW),$$($(2)_HASH_FILE),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep)) > + $(Q)$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAWNAME),$$($(2)_BASENAME_RAW),$$($(2)_HASH_FILE),$$(F),$$(call prefix-if-needed,$$($(2)_DIR),$$(F)),$$(call UPPERCASE,$(4)))$$(sep)) > endif # license files > > ifeq ($$($(2)_SITE_METHOD),local) > diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk > index ae3c7f9da9..0c287a0f50 100644 > --- a/package/pkg-utils.mk > +++ b/package/pkg-utils.mk > @@ -207,6 +207,10 @@ endif > # > LEGAL_INFO_SEPARATOR = "::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::" > > +define prefix-if-needed # builddir, filename > + $(if $(wildcard $(1)/$(2)),$(1)/$(2),$(2)) > +endef > + > define legal-warning # text > echo "WARNING: $(1)" >>$(LEGAL_WARNINGS) > endef
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index ded5176428..e6d9f457e4 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -1139,7 +1139,7 @@ ifneq ($$(call qstrip,$$($(2)_SOURCE)),) ifeq ($$(call qstrip,$$($(2)_LICENSE_FILES)),) $(Q)$$(call legal-warning-pkg,$$($(2)_BASENAME_RAW),cannot save license ($(2)_LICENSE_FILES not defined)) else - $(Q)$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAWNAME),$$($(2)_BASENAME_RAW),$$($(2)_HASH_FILE),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep)) + $(Q)$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAWNAME),$$($(2)_BASENAME_RAW),$$($(2)_HASH_FILE),$$(F),$$(call prefix-if-needed,$$($(2)_DIR),$$(F)),$$(call UPPERCASE,$(4)))$$(sep)) endif # license files ifeq ($$($(2)_SITE_METHOD),local) diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk index ae3c7f9da9..0c287a0f50 100644 --- a/package/pkg-utils.mk +++ b/package/pkg-utils.mk @@ -207,6 +207,10 @@ endif # LEGAL_INFO_SEPARATOR = "::::::::::::::::::::::::::::::::::::::::::::::::::::::::::::" +define prefix-if-needed # builddir, filename + $(if $(wildcard $(1)/$(2)),$(1)/$(2),$(2)) +endef + define legal-warning # text echo "WARNING: $(1)" >>$(LEGAL_WARNINGS) endef
legal-info currently assumes that a packages _LICENSE_FILES will be a path or a filename relative to the source of the package. There is nothing preventing _LICENSE_FILES from containing a full path. When this happens, the legal-info process adds the build directory on the front of the _LICENSE_FILES value resulting in a failed cp of the file. This patch adds a check to see if the _LICENSE_FILES path with the build directory prepended exists (as is the case for all packages currently) and uses that. Otherwise it will assume that the _LICENSE_FILES file is a full path. An 'offending' package .mk file could look something like this: LIBFOO_VERSION = 1.0 LIBFOO_SOURCE = libfoo-$(LIBFOO_VERSION).tar.gz LIBFOO_SITE = http://www.foosoftware.org/download LIBFOO_LICENSE = GPL-3.0+ LIBFOO_LICENSE_FILES = $(LIBFOO_PKGDIR)/COPYING [...] $(eval $(generic-package)) Signed-off-by: Cyril Bur <cyrilbur@gmail.com> --- Adding a licence file to buildroot is probably odd and a strange thing to do, we're doing this internally (and I do hope I can push the package soon) with https://github.com/BrianGladman/modes V1->V2 Reworked commit message and added example as requested by Thomas Petazzoni package/pkg-generic.mk | 2 +- package/pkg-utils.mk | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-)