Message ID | 1522239999-15582-1-git-send-email-james.byrne@origamienergy.com |
---|---|
State | Superseded |
Headers | show |
Series | [1/1] Makefile: Correctly take source date from commit date | expand |
On 28-03-18 14:26, James Byrne wrote: > For reproducible builds SOURCE_DATE_EPOCH is supposed to be set to the > date of the last Buildroot commit if running from a git repository, but > this doesn't actually work. This corrects this in the following way: > > 1) Set GIT from BR2_GIT, otherwise it may be undefined. > > 2) Use 'git rev-parse --is-inside-work-tree' to see if we are inside a If you say "1 and 2" in your commit log, that's a hint it should be two separate patches :-) > Git repository. There is no need to use TOPDIR, because the current > directory will always be TOPDIR, and in any case the Git repository may > be at a higher directory level if Buildroot has been imported into a > larger build system. Hm, but in that case, do you really want SOURCE_DATE_EPOCH to be set to the git epoch of the larger build system? I think that depends a lot on the exact circumstances. Anyway, a much more likely situation is that your "larger build system" includes one or more BR2_EXTERNAL directories - and those are typically a lot more volatile than the Buildroot directory itself. So maybe the source epoch should be set to the git epoch of one of those? I think there are just too many different situations here, and that we should just rely on the user explicitly setting SOURCE_DATE_EPOCH to whatever is appropriate for her/him. That said, using any surrounding git repo sounds like a sane default. So in the end I'm not really opposed to your patch. > > Signed-off-by: James Byrne <james.byrne@origamienergy.com> > --- > Makefile | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index 9e2402d..59af554 100644 > --- a/Makefile > +++ b/Makefile > @@ -253,8 +253,10 @@ export TZ = UTC > export LANG = C > export LC_ALL = C > export GZIP = -n > -BR2_VERSION_GIT_EPOCH = $(shell GIT_DIR=$(TOPDIR)/.git $(GIT) log -1 --format=%at) > -export SOURCE_DATE_EPOCH ?= $(if $(wildcard $(TOPDIR)/.git),$(BR2_VERSION_GIT_EPOCH),$(BR2_VERSION_EPOCH)) > +GIT := $(call qstrip,$(BR2_GIT)) This is in fact defined in package/pkg-download.mk. I guess the problem is that that that file doesn't get included on an unconfigured system. However, in that case, how does your BR2_REPRODUCIBLE get set? Can you specify under which circumstances this variable is not defined? > +BR2_IN_WORK_TREE = $(if $(subst false,,$(shell $(GIT) rev-parse --is-inside-work-tree 2> /dev/null || echo false)),Y,) This would be much simpler: BR2_IN_WORK_TREE = $(shell $(GIT) rev-parse --is-inside-work-tree 2> /dev/null) This will be empty in case it is not in a work tree, and 'true' if it is. > +BR2_VERSION_GIT_EPOCH = $(shell $(GIT) log -1 --format=%at) > +export SOURCE_DATE_EPOCH ?= $(if $(BR2_IN_WORK_TREE),$(BR2_VERSION_GIT_EPOCH),$(BR2_VERSION_EPOCH)) But actually, it could be made a whole lot simpler: BR2_VERSION_GIT_EPOCH = $(shell $(GIT) log -1 --format=%at 2> /dev/null) export SOURCE_DATE_EPOCH ?= $(or $(BR2_VERSION_GIT_EPOCH),$(BR2_VERSION_EPOCH)) BR2_VERSION_GIT_EPOCH will be empty if it's not in a worktree. Regards, Arnout > endif > > # To put more focus on warnings, be less verbose as default > -- > 2.7.4 > > The contents of this email and any attachment are confidential to the intended recipient(s). If you are not an intended recipient: (i) do not use, disclose, distribute, copy or publish this email or its contents; (ii) please contact the sender immediately; and (iii) delete this email. Origami Energy Limited (company number 8619644); Origami Storage Limited (company number 10436515) and OSSPV001 Limited (company number 10933403), each registered in England and each with a registered office at: Ashcombe Court, Woolsack Way, Godalming, GU7 1LQ. > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot >
James, All, On 2018-03-28 13:26 +0100, James Byrne spake thusly: > For reproducible builds SOURCE_DATE_EPOCH is supposed to be set to the > date of the last Buildroot commit if running from a git repository, but > this doesn't actually work. This corrects this in the following way: > > 1) Set GIT from BR2_GIT, otherwise it may be undefined. > > 2) Use 'git rev-parse --is-inside-work-tree' to see if we are inside a > Git repository. There is no need to use TOPDIR, because the current > directory will always be TOPDIR, and in any case the Git repository may > be at a higher directory level if Buildroot has been imported into a > larger build system. Besides what Arnout said, I'm not a big fan of this patch. Buildroot does abide by the rules: if SOURCE_DATE_EPOCH is seet in the environemnt, we do not set it. Otherwise we do set it to a date that is meaningful to Buildroot itself. In an upper-layer buildsystem wants to use its own date, it should set it in the environment before calling us. That's what the spec about SOURCE_DATE_EPOCH mandates: https://reproducible-builds.org/specs/source-date-epoch/ So, I would just drop that patch. Now, having SOURCE_DATE_EPOCH set correctly in an unconfigured tree could be fixed, in deed, but what would be the use case for that? Regards, Yann E. MORIN. > Signed-off-by: James Byrne <james.byrne@origamienergy.com> > --- > Makefile | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/Makefile b/Makefile > index 9e2402d..59af554 100644 > --- a/Makefile > +++ b/Makefile > @@ -253,8 +253,10 @@ export TZ = UTC > export LANG = C > export LC_ALL = C > export GZIP = -n > -BR2_VERSION_GIT_EPOCH = $(shell GIT_DIR=$(TOPDIR)/.git $(GIT) log -1 --format=%at) > -export SOURCE_DATE_EPOCH ?= $(if $(wildcard $(TOPDIR)/.git),$(BR2_VERSION_GIT_EPOCH),$(BR2_VERSION_EPOCH)) > +GIT := $(call qstrip,$(BR2_GIT)) > +BR2_IN_WORK_TREE = $(if $(subst false,,$(shell $(GIT) rev-parse --is-inside-work-tree 2> /dev/null || echo false)),Y,) > +BR2_VERSION_GIT_EPOCH = $(shell $(GIT) log -1 --format=%at) > +export SOURCE_DATE_EPOCH ?= $(if $(BR2_IN_WORK_TREE),$(BR2_VERSION_GIT_EPOCH),$(BR2_VERSION_EPOCH)) > endif > > # To put more focus on warnings, be less verbose as default > -- > 2.7.4 > > The contents of this email and any attachment are confidential to the intended recipient(s). If you are not an intended recipient: (i) do not use, disclose, distribute, copy or publish this email or its contents; (ii) please contact the sender immediately; and (iii) delete this email. Origami Energy Limited (company number 8619644); Origami Storage Limited (company number 10436515) and OSSPV001 Limited (company number 10933403), each registered in England and each with a registered office at: Ashcombe Court, Woolsack Way, Godalming, GU7 1LQ. > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hi Arnout, On 30/03/18 20:28, Arnout Vandecappelle wrote: > On 28-03-18 14:26, James Byrne wrote: >> Git repository. There is no need to use TOPDIR, because the current >> directory will always be TOPDIR, and in any case the Git repository may >> be at a higher directory level if Buildroot has been imported into a >> larger build system. > > Hm, but in that case, do you really want SOURCE_DATE_EPOCH to be set to the git > epoch of the larger build system? I think that depends a lot on the exact > circumstances. > > Anyway, a much more likely situation is that your "larger build system" > includes one or more BR2_EXTERNAL directories - and those are typically a lot > more volatile than the Buildroot directory itself. So maybe the source epoch > should be set to the git epoch of one of those? > > I think there are just too many different situations here, and that we should > just rely on the user explicitly setting SOURCE_DATE_EPOCH to whatever is > appropriate for her/him. That said, using any surrounding git repo sounds like a > sane default. So in the end I'm not really opposed to your patch. You are right. I had not understood the significance of SOURCE_DATE_EPOCH, but do now, thanks. I still think however that taking the commit date of whatever repository you are in is a better default than only doing so if the repository is at the same level as the Buildroot tree. >> Signed-off-by: James Byrne <james.byrne@origamienergy.com> >> --- >> Makefile | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/Makefile b/Makefile >> index 9e2402d..59af554 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -253,8 +253,10 @@ export TZ = UTC >> export LANG = C >> export LC_ALL = C >> export GZIP = -n >> -BR2_VERSION_GIT_EPOCH = $(shell GIT_DIR=$(TOPDIR)/.git $(GIT) log -1 --format=%at) >> -export SOURCE_DATE_EPOCH ?= $(if $(wildcard $(TOPDIR)/.git),$(BR2_VERSION_GIT_EPOCH),$(BR2_VERSION_EPOCH)) >> +GIT := $(call qstrip,$(BR2_GIT)) > > This is in fact defined in package/pkg-download.mk. I guess the problem is that > that that file doesn't get included on an unconfigured system. However, in that > case, how does your BR2_REPRODUCIBLE get set? Can you specify under which > circumstances this variable is not defined? I have to apologise here because I confused myself by adding debugging to the Makefile that caused SOURCE_DATE_EPOCH to get evaluated immediately (before Makefile.in was included), and hence I got an error instead of the correct date. In normal usage however this would never happen, so this change is unnecessary. It did make me realise though that the 'git log' command is executed every time SOURCE_DATE_EPOCH is referenced (over 400 times in the build I tested), which is not very efficient given that the answer cannot change, so I will submit a revised patch that addresses this. >> +BR2_IN_WORK_TREE = $(if $(subst false,,$(shell $(GIT) rev-parse --is-inside-work-tree 2> /dev/null || echo false)),Y,) > > This would be much simpler: > > BR2_IN_WORK_TREE = $(shell $(GIT) rev-parse --is-inside-work-tree 2> /dev/null) > > This will be empty in case it is not in a work tree, and 'true' if it is. > >> +BR2_VERSION_GIT_EPOCH = $(shell $(GIT) log -1 --format=%at) >> +export SOURCE_DATE_EPOCH ?= $(if $(BR2_IN_WORK_TREE),$(BR2_VERSION_GIT_EPOCH),$(BR2_VERSION_EPOCH)) > But actually, it could be made a whole lot simpler: > > BR2_VERSION_GIT_EPOCH = $(shell $(GIT) log -1 --format=%at 2> /dev/null) > export SOURCE_DATE_EPOCH ?= $(or $(BR2_VERSION_GIT_EPOCH),$(BR2_VERSION_EPOCH)) > > BR2_VERSION_GIT_EPOCH will be empty if it's not in a worktree. Good point, that's much neater. I will use this in my revised patch. Regards, James The contents of this email and any attachment are confidential to the intended recipient(s). If you are not an intended recipient: (i) do not use, disclose, distribute, copy or publish this email or its contents; (ii) please contact the sender immediately; and (iii) delete this email. Origami Energy Limited (company number 8619644); Origami Storage Limited (company number 10436515) and OSSPV001 Limited (company number 10933403), each registered in England and each with a registered office at: Ashcombe Court, Woolsack Way, Godalming, GU7 1LQ.
diff --git a/Makefile b/Makefile index 9e2402d..59af554 100644 --- a/Makefile +++ b/Makefile @@ -253,8 +253,10 @@ export TZ = UTC export LANG = C export LC_ALL = C export GZIP = -n -BR2_VERSION_GIT_EPOCH = $(shell GIT_DIR=$(TOPDIR)/.git $(GIT) log -1 --format=%at) -export SOURCE_DATE_EPOCH ?= $(if $(wildcard $(TOPDIR)/.git),$(BR2_VERSION_GIT_EPOCH),$(BR2_VERSION_EPOCH)) +GIT := $(call qstrip,$(BR2_GIT)) +BR2_IN_WORK_TREE = $(if $(subst false,,$(shell $(GIT) rev-parse --is-inside-work-tree 2> /dev/null || echo false)),Y,) +BR2_VERSION_GIT_EPOCH = $(shell $(GIT) log -1 --format=%at) +export SOURCE_DATE_EPOCH ?= $(if $(BR2_IN_WORK_TREE),$(BR2_VERSION_GIT_EPOCH),$(BR2_VERSION_EPOCH)) endif # To put more focus on warnings, be less verbose as default
For reproducible builds SOURCE_DATE_EPOCH is supposed to be set to the date of the last Buildroot commit if running from a git repository, but this doesn't actually work. This corrects this in the following way: 1) Set GIT from BR2_GIT, otherwise it may be undefined. 2) Use 'git rev-parse --is-inside-work-tree' to see if we are inside a Git repository. There is no need to use TOPDIR, because the current directory will always be TOPDIR, and in any case the Git repository may be at a higher directory level if Buildroot has been imported into a larger build system. Signed-off-by: James Byrne <james.byrne@origamienergy.com> --- Makefile | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) -- 2.7.4 The contents of this email and any attachment are confidential to the intended recipient(s). If you are not an intended recipient: (i) do not use, disclose, distribute, copy or publish this email or its contents; (ii) please contact the sender immediately; and (iii) delete this email. Origami Energy Limited (company number 8619644); Origami Storage Limited (company number 10436515) and OSSPV001 Limited (company number 10933403), each registered in England and each with a registered office at: Ashcombe Court, Woolsack Way, Godalming, GU7 1LQ.