diff mbox series

[1/1] Makefile: Correctly take source date from commit date

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

Commit Message

James Byrne March 28, 2018, 12:26 p.m. UTC
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.

Comments

Arnout Vandecappelle March 30, 2018, 7:28 p.m. UTC | #1
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
>
Yann E. MORIN April 1, 2018, 6:23 a.m. UTC | #2
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
James Byrne April 6, 2018, 10:22 a.m. UTC | #3
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 mbox series

Patch

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