| Message ID | 1520326085-18337-2-git-send-email-angelo@amarulasolutions.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] Makefile: add tainting support | expand |
Hello, On Tue, 6 Mar 2018 09:48:04 +0100, Angelo Compagnucci wrote: > +# Check here if there are packages declaring they harm > +# the reproducibility of the build > +.PHONY: check-tainted > +check-tainted: > +ifneq ($(BR2_REPRODUCIBLE),) > +ifneq ($(BR2_TAINTED_BY),) I don't think BR2_REPRODUCIBLE has anything to do with this 'tainting' thing. BR2_REPRODUCIBLE is about getting binary identical results given a Buildroot configuration. But even with BR2_REPRODUCIBLE disabled, Buildroot aims at building the same source code, which the nodejs npm stuff does not allow to do. So the 'tainting' happens regardless of whether BR2_REPRODUCIBLE=y or not. If you remove the BR2_REPRODUCIBLE condition, then the Buildroot build would always fail as soon as one package taints the build, so I'm not sure a hard failure is a good approach here. Perhaps just a warning at the end of the build ? > .PHONY: target-post-image > -target-post-image: $(TARGETS_ROOTFS) target-finalize > +target-post-image: check-tainted $(TARGETS_ROOTFS) target-finalize > @$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_IMAGE_SCRIPT)), \ > $(call MESSAGE,"Executing post-image script $(s)"); \ > $(EXTRA_ENV) $(s) $(BINARIES_DIR) $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS))$(sep)) > @@ -1063,6 +1074,7 @@ help: > @echo ' source - download all sources needed for offline-build' > @echo ' external-deps - list external packages used' > @echo ' legal-info - generate info about license compliance' > + @echo ' check-tainted - check if any selected package harms build reproducibility' Not sure it is really useful to make this a first class target, documented in the help text. A warning at the end of the build is probably enough. Or maybe simply mention it as a Config.in comment ? comment "WARNING: nodejs modules will make your build non reproducible!" depends on BR2_PACKAGE_NODEJS_MODULES_ADDITIONAL != "" > @echo ' printvars - dump all the internal variables' > @echo > @echo ' make V=0|1 - 0 => quiet build (default), 1 => verbose build' > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index a2a12e7..e643ecf 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -556,6 +556,10 @@ ifndef $(2)_REDISTRIBUTE > endif > endif > > +ifdef $(2)_TAINTS > + BR2_TAINTED_BY+=$$($(2)_RAWNAME) > +endif I think "TAINTS" is very vague. Unfortunately, I don't have a good proposal. In addition, it's not only about tainting the build in terms of reproducibility, but also about the lack of integration with the download and legal infrastructures. I'm not sure what a good name for this would be. I've added Arnout in Cc, he often has interesting (and creative) ideas on such topics. > +# Save a legal warning if tainted > +ifneq ($$(call qstrip,$$($(2)_TAINTS)),) Implementation detail: you always use the construct: ifneq ($(var),) which is negative logic. Please use: ifeq ($(var),YES) instead. Of course, unless the variable really is empty vs. non-empty, but that isn't the case here. Also, your patch series was sent at a time where patchwork was missing some patches, so your PATCH 2/2 was not recorded. It would be useful if you could resend this patch series. Best regards, Thomas
Dear Thomas, 2018-04-28 18:53 GMT+02:00 Thomas Petazzoni <thomas.petazzoni@bootlin.com>: > Hello, > > On Tue, 6 Mar 2018 09:48:04 +0100, Angelo Compagnucci wrote: > >> +# Check here if there are packages declaring they harm >> +# the reproducibility of the build >> +.PHONY: check-tainted >> +check-tainted: >> +ifneq ($(BR2_REPRODUCIBLE),) >> +ifneq ($(BR2_TAINTED_BY),) > > I don't think BR2_REPRODUCIBLE has anything to do with this 'tainting' > thing. > > BR2_REPRODUCIBLE is about getting binary identical results given a > Buildroot configuration. But even with BR2_REPRODUCIBLE disabled, > Buildroot aims at building the same source code, which the nodejs npm > stuff does not allow to do. So the 'tainting' happens regardless of > whether BR2_REPRODUCIBLE=y or not. > > If you remove the BR2_REPRODUCIBLE condition, then the Buildroot build > would always fail as soon as one package taints the build, so I'm not > sure a hard failure is a good approach here. Perhaps just a warning at > the end of the build ? Fair enough, will do. > >> .PHONY: target-post-image >> -target-post-image: $(TARGETS_ROOTFS) target-finalize >> +target-post-image: check-tainted $(TARGETS_ROOTFS) target-finalize >> @$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_IMAGE_SCRIPT)), \ >> $(call MESSAGE,"Executing post-image script $(s)"); \ >> $(EXTRA_ENV) $(s) $(BINARIES_DIR) $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS))$(sep)) >> @@ -1063,6 +1074,7 @@ help: >> @echo ' source - download all sources needed for offline-build' >> @echo ' external-deps - list external packages used' >> @echo ' legal-info - generate info about license compliance' >> + @echo ' check-tainted - check if any selected package harms build reproducibility' > > Not sure it is really useful to make this a first class target, > documented in the help text. A warning at the end of the build is > probably enough. I think this should be kept especially for CI systems in which it's useful to know if a user introduced something that harms the build. > Or maybe simply mention it as a Config.in comment ? > > comment "WARNING: nodejs modules will make your build non reproducible!" > depends on BR2_PACKAGE_NODEJS_MODULES_ADDITIONAL != "" > >> @echo ' printvars - dump all the internal variables' >> @echo >> @echo ' make V=0|1 - 0 => quiet build (default), 1 => verbose build' >> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk >> index a2a12e7..e643ecf 100644 >> --- a/package/pkg-generic.mk >> +++ b/package/pkg-generic.mk >> @@ -556,6 +556,10 @@ ifndef $(2)_REDISTRIBUTE >> endif >> endif >> >> +ifdef $(2)_TAINTS >> + BR2_TAINTED_BY+=$$($(2)_RAWNAME) >> +endif > > I think "TAINTS" is very vague. Unfortunately, I don't have a good > proposal. In addition, it's not only about tainting the build in terms > of reproducibility, but also about the lack of integration with the > download and legal infrastructures. Yes, it's not the best name probably, but the concept is here. > > I'm not sure what a good name for this would be. I've added Arnout in > Cc, he often has interesting (and creative) ideas on such topics. > >> +# Save a legal warning if tainted >> +ifneq ($$(call qstrip,$$($(2)_TAINTS)),) > > Implementation detail: you always use the construct: > > ifneq ($(var),) > > which is negative logic. Please use: > > ifeq ($(var),YES) > > instead. Of course, unless the variable really is empty vs. non-empty, > but that isn't the case here. > > Also, your patch series was sent at a time where patchwork was missing > some patches, so your PATCH 2/2 was not recorded. It would be useful if > you could resend this patch series. > > Best regards, > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) > Embedded Linux and Kernel engineering > https://bootlin.com > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
On 03-05-18 23:34, Angelo Compagnucci wrote: > Dear Thomas, > > 2018-04-28 18:53 GMT+02:00 Thomas Petazzoni <thomas.petazzoni@bootlin.com>: >> Hello, >> >> On Tue, 6 Mar 2018 09:48:04 +0100, Angelo Compagnucci wrote: [snip] >>> @@ -1063,6 +1074,7 @@ help: >>> @echo ' source - download all sources needed for offline-build' >>> @echo ' external-deps - list external packages used' >>> @echo ' legal-info - generate info about license compliance' >>> + @echo ' check-tainted - check if any selected package harms build reproducibility' >> >> Not sure it is really useful to make this a first class target, >> documented in the help text. A warning at the end of the build is >> probably enough. > > I think this should be kept especially for CI systems in which it's > useful to know if a user introduced something that harms the build. Yes, makes sense. Especially the way you did it in v2: non-fatal in the target-finalize step, but fatal if check-tainted is called explicitly. >> Or maybe simply mention it as a Config.in comment ? >> >> comment "WARNING: nodejs modules will make your build non reproducible!" >> depends on BR2_PACKAGE_NODEJS_MODULES_ADDITIONAL != "" >> >>> @echo ' printvars - dump all the internal variables' >>> @echo >>> @echo ' make V=0|1 - 0 => quiet build (default), 1 => verbose build' >>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk >>> index a2a12e7..e643ecf 100644 >>> --- a/package/pkg-generic.mk >>> +++ b/package/pkg-generic.mk >>> @@ -556,6 +556,10 @@ ifndef $(2)_REDISTRIBUTE >>> endif >>> endif >>> >>> +ifdef $(2)_TAINTS >>> + BR2_TAINTED_BY+=$$($(2)_RAWNAME) >>> +endif >> >> I think "TAINTS" is very vague. Unfortunately, I don't have a good >> proposal. In addition, it's not only about tainting the build in terms >> of reproducibility, but also about the lack of integration with the >> download and legal infrastructures. > > Yes, it's not the best name probably, but the concept is here. In the kernel the 'taint' name is also used for things that are not really literally tainting, so why not. Regards, Arnout >> I'm not sure what a good name for this would be. I've added Arnout in >> Cc, he often has interesting (and creative) ideas on such topics. [snip]
diff --git a/Makefile b/Makefile index 1b34818..afa5f29 100644 --- a/Makefile +++ b/Makefile @@ -761,8 +761,19 @@ endif $(call MESSAGE,"Executing post-build script $(s)"); \ $(EXTRA_ENV) $(s) $(TARGET_DIR) $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS))$(sep)) +# Check here if there are packages declaring they harm +# the reproducibility of the build +.PHONY: check-tainted +check-tainted: +ifneq ($(BR2_REPRODUCIBLE),) +ifneq ($(BR2_TAINTED_BY),) + $(error Buildroot is tainted (by: $(BR2_TAINTED_BY)). Cannot do a reproducible build.) + @exit 1 +endif +endif + .PHONY: target-post-image -target-post-image: $(TARGETS_ROOTFS) target-finalize +target-post-image: check-tainted $(TARGETS_ROOTFS) target-finalize @$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_IMAGE_SCRIPT)), \ $(call MESSAGE,"Executing post-image script $(s)"); \ $(EXTRA_ENV) $(s) $(BINARIES_DIR) $(call qstrip,$(BR2_ROOTFS_POST_SCRIPT_ARGS))$(sep)) @@ -1063,6 +1074,7 @@ help: @echo ' source - download all sources needed for offline-build' @echo ' external-deps - list external packages used' @echo ' legal-info - generate info about license compliance' + @echo ' check-tainted - check if any selected package harms build reproducibility' @echo ' printvars - dump all the internal variables' @echo @echo ' make V=0|1 - 0 => quiet build (default), 1 => verbose build' diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index a2a12e7..e643ecf 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -556,6 +556,10 @@ ifndef $(2)_REDISTRIBUTE endif endif +ifdef $(2)_TAINTS + BR2_TAINTED_BY+=$$($(2)_RAWNAME) +endif + $(2)_REDISTRIBUTE ?= YES $(2)_REDIST_SOURCES_DIR = $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))/$$($(2)_RAW_BASE_NAME) @@ -869,6 +873,11 @@ else $(Q)$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAWNAME),$$($(2)_RAW_BASE_NAME),$$($(2)_PKGDIR),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep)) endif # license files +# Save a legal warning if tainted +ifneq ($$(call qstrip,$$($(2)_TAINTS)),) + $(Q)$$(call legal-warning-pkg,$$($(2)_RAWNAME),unknown license for additional modules or dependencies) +endif + ifeq ($$($(2)_SITE_METHOD),local) # Packages without a tarball: don't save and warn @$$(call legal-warning-nosource,$$($(2)_RAWNAME),local)