Hi KiBi,

Many thanks for reviewing this.

Cyril Brulebois wrote:
> Please make sure not to depend on features which are not found in stable
> (I'm not entirely sure about oldstable at this point), which might hinder
> our ability to cherry-pick bits and pieces from master to jessie.

I think I could make that easier by splitting into smaller commits:

  * parts only needed for kfreebsd
  * parts more appropriate for stable, oldstable, etc.
  * parts only appropriate for sid (or just not use new GNU tar
    features yet)

> > --- a/build/Makefile
> > +++ b/build/Makefile
> > @@ -56,7 +56,7 @@
> >  # Add to PATH so dpkg will always work, and so local programs will be 
> > found.
> >  PATH := util:$(PATH):/usr/sbin:/sbin
> >  EATMYDATA = $(shell which eatmydata 2>/dev/null)
> > -GZIP = $(shell which pigz gzip | head -1)
> > +GZIP = $(shell which pigz gzip | head -1) -n
> 
> I think I already added -n to a bunch of calls. Not sure whether adding
> it here once and for all would be better than adding it where it's
> missing though. Anyway, not my biggest question/comment/concern here.

Seems reasonable to factor out and put it here.  If we don't, someone
may add a new $GZIP call later, forget -n and make it unreproducible
again.

Although it is a macro here, GZIP is also the name of an environment
variable used by gzip (but not pigz?), which is likely confusing.  And
the later tar invocations call gzip (not pigz) in quite a few places
regardless of the GZIP macro contents;  those do look at the GZIP
environment variable though.

> I think those 3(.5) occurrences really should be factorized, especially
> given the logic is the same: replacing "cd foo && tar bar ." with more
> code. Somewhere under build/util would probably be suitable.

I agree.  It is a very common pattern in other Debian packages too, and
it often needs patching for reproducibility.

> > --- a/build/config/x86.cfg
> > +++ b/build/config/x86.cfg
> > @@ -332,6 +332,11 @@ arch_miniiso: x86_syslinux x86_grub_efi
> >                     | todos > $(TEMP_CD_TREE)/win32-loader.ini; \
> >     fi
> >  
> > +   # Clamp timestamps to be no newer than last changelog entry, see
> > +   # https://wiki.debian.org/ReproducibleBuilds/TimestampsInTarball
> > +   find $(TEMP_CD_TREE) -newermt "$(SOURCE_DATE)" -print0 \
> > +    | xargs -0r touch --no-dereference --date="$(SOURCE_DATE)"
> > +

> [...] above is using SOURCE_DATE, which is undefined as
> far as I can tell since SOURCE_DATE_EPOCH is what's getting defined.
> Maybe it should call the same new util (with different parameters since
> we only need touch here, and no tar call)?

That's a typo, and was buggy - it would touch all timestamps, not just
the ones necessary.  I may drop this part as I don't think it's needed
any more with the newer makefs, which will clamp the timestamps itself.

> Finally, not everything is built using debian/rules targets (with or
> without dpkg-buildpackage). One should still be able to just run e.g.:
> “make -C build build_netboot-gtk USE_UDEBS_FROM=sid”
> 
> See BUILD_DATE handling, for example. We end up with a default setting
> through:
> |    build/config/common:BUILD_DATE ?= $(shell date -u '+%Y%m%d-%H:%M')

That would not be reproducible, then!  (it is embedded in the tarballs)

> [ Please note that calling $(shell dpkg-parsechangelog -SDate) to set
> SOURCE_DATE_EPOCH there would only work when building from the toplevel
> directory, and not from the build/ subdirectory for example. ]

If it's anyway not going to be reproducible, we could similarly fall
back to a SOURCE_DATE_EPOCH ?= now;  or the caller could choose to
provide them.

Regards,
-- 
Steven Chamberlain
ste...@pyro.eu.org

Attachment: signature.asc
Description: Digital signature

_______________________________________________
Reproducible-builds mailing list
Reproducible-builds@lists.alioth.debian.org
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/reproducible-builds

Reply via email to