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

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

[James Byrne]

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(-)

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] [PATCH 1/1] Makefile: Correctly take source date from commit date

[Arnout Vandecappelle]

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 at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

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

[Yann E. MORIN]

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 at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

[James Byrne]

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.

[Buildroot] [PATCH v2] Makefile: Avoid executing 'git log' each time SOURCE_DATE_EPOCH is used

[James Byrne]

If SOURCE_DATE_EPOCH is not defined it was given a definition that
caused 'git log' to be executed each time the variable is referenced,
which is not very efficient given that the answer cannot change.

This commit moves the definition of BR2_VERSION_GIT_EPOCH after the
inclusion of Makefile.in (so that GIT is defined) and makes it a simply
expanded variable so that it is only evaluated once.

In addition, the git directory is no longer explicitly set to be TOPDIR
meaning that the default date will come from the commit date of whatever
repository the code is commited within instead of 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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 0724f28..3b846b9 100644
--- a/Makefile
+++ b/Makefile
@@ -247,8 +247,6 @@ 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))
 endif

 # To put more focus on warnings, be less verbose as default
@@ -504,6 +502,14 @@ include support/dependencies/dependencies.mk
 include $(sort $(wildcard toolchain/*.mk))
 include $(sort $(wildcard toolchain/*/*.mk))

+ifeq ($(BR2_REPRODUCIBLE),y)
+# If SOURCE_DATE_EPOCH has not been set then use the commit date, or the last
+# release date if the source tree is not within a Git repository.
+# See: https://reproducible-builds.org/specs/source-date-epoch/
+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))
+endif
+
 # Include the package override file if one has been provided in the
 # configuration.
 PACKAGE_OVERRIDE_FILE = $(call qstrip,$(BR2_PACKAGE_OVERRIDE_FILE))
--
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] [PATCH v2] Makefile: Avoid executing 'git log' each time SOURCE_DATE_EPOCH is used

[Yann E. MORIN]

James, All,

On 2018-04-06 11:26 +0100, James Byrne spake thusly:
> If SOURCE_DATE_EPOCH is not defined it was given a definition that
> caused 'git log' to be executed each time the variable is referenced,
> which is not very efficient given that the answer cannot change.
> 
> This commit moves the definition of BR2_VERSION_GIT_EPOCH after the
> inclusion of Makefile.in (so that GIT is defined) and makes it a simply
> expanded variable so that it is only evaluated once.
> 
> In addition, the git directory is no longer explicitly set to be TOPDIR
> meaning that the default date will come from the commit date of whatever
> repository the code is commited within instead of only doing so if the
> repository is at the same level as the Buildroot tree.

Since your are saying "in addition", it smells like this should have
been two seaparate commits:

  - one to change the variable to being simply expanded

  - a second one to not enforce the git directory.

I am totally OK with the first change, while I am still not convinced by
the second. As I already said, if Buildroot is used from a higher-level
buildsystem, it is the responsibility of that higher-level buildsystem
to appropriately set SOURCE_DATE_EPOCH in the environment.

Also, bear in mind that, even without a higher-level buildsystem, it is
possible to call Buildroot without actualy being in Buildroot's tree,
e.g.:

    $ make -C /path/to/buildroot O=/path/to/build-dir

Or even:

    $ cd /path/to/build-dir
    $ make -C /path/to/buildroot O=$(pwd) foo_defconfig
    $ make

And then you would lose the git info that is present in the Buildroot
tree in /path/to/buildroot. I actually use either forms quite a lot,
especially the second one. In fact, I even never build in the Buildroot
tree, except when people report it is broken and I need to investigate.

So, I am still opposed to the second change.

Regards,
Yann E. MORIN.

> Signed-off-by: James Byrne <james.byrne@origamienergy.com>
> ---
>  Makefile | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 0724f28..3b846b9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -247,8 +247,6 @@ 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))
>  endif
> 
>  # To put more focus on warnings, be less verbose as default
> @@ -504,6 +502,14 @@ include support/dependencies/dependencies.mk
>  include $(sort $(wildcard toolchain/*.mk))
>  include $(sort $(wildcard toolchain/*/*.mk))
> 
> +ifeq ($(BR2_REPRODUCIBLE),y)
> +# If SOURCE_DATE_EPOCH has not been set then use the commit date, or the last
> +# release date if the source tree is not within a Git repository.
> +# See: https://reproducible-builds.org/specs/source-date-epoch/
> +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))
> +endif
> +
>  # Include the package override file if one has been provided in the
>  # configuration.
>  PACKAGE_OVERRIDE_FILE = $(call qstrip,$(BR2_PACKAGE_OVERRIDE_FILE))
> --
> 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 at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v2] Makefile: Avoid executing 'git log' each time SOURCE_DATE_EPOCH is used

[James Byrne]

Hi Yann,

On 07/04/18 15:33, Yann E. MORIN wrote:
> On 2018-04-06 11:26 +0100, James Byrne spake thusly:
>> If SOURCE_DATE_EPOCH is not defined it was given a definition that
>> caused 'git log' to be executed each time the variable is referenced,
>> which is not very efficient given that the answer cannot change.
>>
>> This commit moves the definition of BR2_VERSION_GIT_EPOCH after the
>> inclusion of Makefile.in (so that GIT is defined) and makes it a simply
>> expanded variable so that it is only evaluated once.
>>
>> In addition, the git directory is no longer explicitly set to be TOPDIR
>> meaning that the default date will come from the commit date of whatever
>> repository the code is commited within instead of only doing so if the
>> repository is at the same level as the Buildroot tree.
>
> Since your are saying "in addition", it smells like this should have
> been two seaparate commits:
>
>    - one to change the variable to being simply expanded
>
>    - a second one to not enforce the git directory.
>
> I am totally OK with the first change, while I am still not convinced by
> the second. As I already said, if Buildroot is used from a higher-level
> buildsystem, it is the responsibility of that higher-level buildsystem
> to appropriately set SOURCE_DATE_EPOCH in the environment.

Understood, I will resubmit this patch without the second change.

> Also, bear in mind that, even without a higher-level buildsystem, it is
> possible to call Buildroot without actualy being in Buildroot's tree,
> e.g.:
>
>      $ make -C /path/to/buildroot O=/path/to/build-dir
>
> Or even:
>
>      $ cd /path/to/build-dir
>      $ make -C /path/to/buildroot O=$(pwd) foo_defconfig
>      $ make
>
> And then you would lose the git info that is present in the Buildroot
> tree in /path/to/buildroot. I actually use either forms quite a lot,
> especially the second one. In fact, I even never build in the Buildroot
> tree, except when people report it is broken and I need to investigate.
>
> So, I am still opposed to the second change.

In both of your examples it will still work as expected since the
buildroot makefile is always invoked with "-C /path/to/buildroot", which
means that when the 'git log' command is executed the current directory
is always '/path/to/buildroot' and so the commit date will be the date
of the tree that the Buildroot makefile is in.

If you think you might change your mind I will submit that change as a
new patch, as I still think it makes a better default, but if you don't
agree then I won't bother as it's a very minor thing, and as you say it
can easily be overridden by setting SOURCE_DATE_EPOCH in the environment.

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.

[Buildroot] [PATCH v3] Makefile: Avoid executing 'git log' each time SOURCE_DATE_EPOCH is used

[James Byrne]

If SOURCE_DATE_EPOCH is not defined it was given a definition that
caused 'git log' to be executed each time the variable is referenced,
which is not very efficient given that the answer cannot change.

This commit moves the definition of BR2_VERSION_GIT_EPOCH after the
inclusion of Makefile.in (so that GIT is defined) and makes it a simply
expanded variable so that it is only evaluated once.

Signed-off-by: James Byrne <james.byrne@origamienergy.com>
---
 Makefile | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 0724f28..cd53362 100644
--- a/Makefile
+++ b/Makefile
@@ -247,8 +247,6 @@ 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))
 endif

 # To put more focus on warnings, be less verbose as default
@@ -504,6 +502,14 @@ include support/dependencies/dependencies.mk
 include $(sort $(wildcard toolchain/*.mk))
 include $(sort $(wildcard toolchain/*/*.mk))

+ifeq ($(BR2_REPRODUCIBLE),y)
+# If SOURCE_DATE_EPOCH has not been set then use the commit date, or the last
+# release date if the source tree is not within a Git repository.
+# See: https://reproducible-builds.org/specs/source-date-epoch/
+BR2_VERSION_GIT_EPOCH := $(shell $(GIT) --git-dir=$(TOPDIR)/.git log -1 --format=%at 2> /dev/null)
+export SOURCE_DATE_EPOCH ?= $(or $(BR2_VERSION_GIT_EPOCH),$(BR2_VERSION_EPOCH))
+endif
+
 # Include the package override file if one has been provided in the
 # configuration.
 PACKAGE_OVERRIDE_FILE = $(call qstrip,$(BR2_PACKAGE_OVERRIDE_FILE))
--
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] [PATCH v3] Makefile: Avoid executing 'git log' each time SOURCE_DATE_EPOCH is used

[Yann E. MORIN]

James, All,

On 2018-04-09 17:08 +0100, James Byrne spake thusly:
> If SOURCE_DATE_EPOCH is not defined it was given a definition that
> caused 'git log' to be executed each time the variable is referenced,
> which is not very efficient given that the answer cannot change.
> 
> This commit moves the definition of BR2_VERSION_GIT_EPOCH after the
> inclusion of Makefile.in (so that GIT is defined) and makes it a simply
> expanded variable so that it is only evaluated once.
> 
> Signed-off-by: James Byrne <james.byrne@origamienergy.com>

Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Thanks for the respin! :-)

Regards,
Yann E. MORIN.

> ---
>  Makefile | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 0724f28..cd53362 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -247,8 +247,6 @@ 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))
>  endif
> 
>  # To put more focus on warnings, be less verbose as default
> @@ -504,6 +502,14 @@ include support/dependencies/dependencies.mk
>  include $(sort $(wildcard toolchain/*.mk))
>  include $(sort $(wildcard toolchain/*/*.mk))
> 
> +ifeq ($(BR2_REPRODUCIBLE),y)
> +# If SOURCE_DATE_EPOCH has not been set then use the commit date, or the last
> +# release date if the source tree is not within a Git repository.
> +# See: https://reproducible-builds.org/specs/source-date-epoch/
> +BR2_VERSION_GIT_EPOCH := $(shell $(GIT) --git-dir=$(TOPDIR)/.git log -1 --format=%at 2> /dev/null)
> +export SOURCE_DATE_EPOCH ?= $(or $(BR2_VERSION_GIT_EPOCH),$(BR2_VERSION_EPOCH))
> +endif
> +
>  # Include the package override file if one has been provided in the
>  # configuration.
>  PACKAGE_OVERRIDE_FILE = $(call qstrip,$(BR2_PACKAGE_OVERRIDE_FILE))
> --
> 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.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v2] Makefile: Avoid executing 'git log' each time SOURCE_DATE_EPOCH is used

[Yann E. MORIN]

James, All,

On 2018-04-09 16:50 +0100, James Byrne spake thusly:
> On 07/04/18 15:33, Yann E. MORIN wrote:
[--SNIP--]
> >So, I am still opposed to the second change.
> In both of your examples it will still work as expected since the
> buildroot makefile is always invoked with "-C /path/to/buildroot", which
> means that when the 'git log' command is executed the current directory
> is always '/path/to/buildroot' and so the commit date will be the date
> of the tree that the Buildroot makefile is in.

Indeed, you are absolutely right. I was  totally oblivious to that
fact, even though I moderately participated in that...

> If you think you might change your mind I will submit that change as a
> new patch, as I still think it makes a better default, but if you don't
> agree then I won't bother as it's a very minor thing, and as you say it
> can easily be overridden by setting SOURCE_DATE_EPOCH in the environment.

So, with your explanations above, I am no longer opposed to that second
change.

Be sure to provide adequate explanations in the commit log, though.
Maybe something that list the two cases:

  - if Buildroot is a git tree on its own, then the 'git log' will act
    on that tree because we always cd into Buildroot's tree;

  - if Buildroot is just a sub-directory in a upper-layer git tree, then
    the git log would act on that directory.

Thank you!

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v3] Makefile: Avoid executing 'git log' each time SOURCE_DATE_EPOCH is used

[Thomas Petazzoni]

Hello,

On Mon,  9 Apr 2018 17:08:01 +0100, James Byrne wrote:
> If SOURCE_DATE_EPOCH is not defined it was given a definition that
> caused 'git log' to be executed each time the variable is referenced,
> which is not very efficient given that the answer cannot change.
> 
> This commit moves the definition of BR2_VERSION_GIT_EPOCH after the
> inclusion of Makefile.in (so that GIT is defined) and makes it a simply
> expanded variable so that it is only evaluated once.
> 
> Signed-off-by: James Byrne <james.byrne@origamienergy.com>
> ---
>  Makefile | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Applied to master, thanks.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com