[riot-notifications] [RIOT-OS/RIOT] Pr/git version (#11881)

Gaëtan Harter notifications at github.com
Mon Jul 22 14:50:45 CEST 2019


### Contribution description

This is an alternative to #11771 and fixes the root of the problem for all normal uses outside of `RIOT_CI_BUILD=1`. https://github.com/RIOT-OS/RIOT/pull/11771#issuecomment-507357262


It only call 'git describe' and 'git rev-parse' through `GIT_VERSION` when it is used.

There are three steps

1. Define `GIT_VERSION` that only calls 'git' when used. It will also only call each commands only once thanks to `memoized`
2. Use it for `RIOT_VERSION`.
   At that point, when `RIOT_VERSION_OVERRIDE` is set, `git` will not be called for this.
3. Have a deferred not exported `CFLAGS_WITH_MACROS`.
   This prevents evaluating the value when unused, so not evaluated when doing `make clean` for example as it would have been before.

### Testing procedure

I will compare different uses and the evolution starting from step 3, step 2 and what was in master.

> I used RIOTPROJECT=${PWD} to hide the first git call to get the current repository.
> It removes this call from the `strace` output as it is not helping the tests



#### State at pull request

When `RIOT_VERSION` is not used, it is not called:

```
RIOTPROJECT=${PWD} strace -e trace=execve -ff make --no-print-directory -C examples/hello-world/ clean 2>&1 | grep '"git"'
```

Each command is still called only once (two git calls) when the value is used

```
RIOTPROJECT=${PWD} strace -e trace=execve -ff make --no-print-directory -C examples/hello-world/ all 2>&1 | grep '"git"'
[pid  9986] execve("/usr/bin/git", ["git", "--git-dir=/home/harter/work/git/"..., "describe", "--always", "--abbrev=4"], 0x564734141288 /* 64 vars */) = 0
[pid  9988] execve("/usr/bin/git", ["git", "--git-dir=/home/harter/work/git/"..., "rev-parse", "--abbrev-ref", "HEAD"], 0x55ee4a169248 /* 64 vars */) = 0
```

It is not called when `RIOT_VERSION_OVERRIDE` is set, here through `RIOT_CI_BUILD=1`

```
RIOT_CI_BUILD=1 RIOTPROJECT=${PWD} strace -e trace=execve -ff make --no-print-directory -C examples/hello-world/ all 2>&1 | grep '"git"'
```

#### Step 2, when `CFLAGS_WITH_MACROS` was still evaluated

<details><summary>Without the third commit, when the value was not used, like for `clean` for example, `git` would still be called.</summary>

```
RIOTPROJECT=${PWD} strace -e trace=execve -ff make --no-print-directory -C examples/hello-world/ clean 2>&1 | grep '"git"'
[pid 11517] execve("/usr/bin/git", ["git", "--git-dir=/home/harter/work/git/"..., "describe", "--always", "--abbrev=4"], 0x559c5ed68288 /* 64 vars */) = 0
[pid 11519] execve("/usr/bin/git", ["git", "--git-dir=/home/harter/work/git/"..., "rev-parse", "--abbrev-ref", "HEAD"], 0x556039163248 /* 64 vars */) = 0
```
</details>

#### In master

<details><summary>The value was always called</summary>

```
RIOTPROJECT=${PWD} strace -e trace=execve -ff make --no-print-directory -C examples/hello-world/ clean 2>&1 | grep '"git"'
[pid 11682] execve("/usr/bin/git", ["git", "--git-dir=/home/harter/work/git/"..., "describe", "--always", "--abbrev=4"], 0x5618db343288 /* 64 vars */) = 0
[pid 11684] execve("/usr/bin/git", ["git", "--git-dir=/home/harter/work/git/"..., "rev-parse", "--abbrev-ref", "HEAD"], 0x55d4af04c248 /* 64 vars */) = 0
```

even when `RIOT_VERSION_OVERRIDE` was set

```
RIOT_CI_BUILD=1 RIOTPROJECT=${PWD} strace -e trace=execve -ff make --no-print-directory -C examples/hello-world/ all 2>&1 | grep '"git"'
[pid 11784] execve("/usr/bin/git", ["git", "--git-dir=/home/harter/work/git/"..., "describe", "--always", "--abbrev=4"], 0x557bd39f72b8 /* 65 vars */) = 0
[pid 11786] execve("/usr/bin/git", ["git", "--git-dir=/home/harter/work/git/"..., "rev-parse", "--abbrev-ref", "HEAD"], 0x564bd2c78278 /* 65 vars */) = 0
```
</details>


#### Speed update

On my machine I do not manage to have reliable timing anymore… Like they are completely inconsistent. I use the one from my build machine that are consistent:

<details><summary>Speed update from 14.5 to 7.5 seconds</summary>

Here `GIT_VERSION` is not evaluated as it is using `RIOT_VERSION_OVERRIDE` instead:

```
time for i in {0..100}; do RIOT_CI_BUILD=1 make --no-print-directory -C examples/hello-world/ info-debug-variable-CFLAGS_WITH_MACROS >/dev/null 2>/dev/null; done

real	0m7.611s
user	0m1.840s
sys	0m0.236s

```

The normal build with RIOT_VERSION being used:
```
time for i in {0..100}; do  make --no-print-directory -C examples/hello-world/ info-debug-variable-CFLAGS_WITH_MACROS >/dev/null 2>/dev/null; done

real	0m14.817s
user	0m8.312s
sys	0m0.640s
```

Commands not using `RIOT_VERSION` have speedup without overwriting:

```
time for i in {0..100}; do make --no-print-directory -C examples/hello-world/ clean  >/dev/null 2>/dev/null; done

real	0m7.702s
user	0m1.804s
sys	0m0.256s
```

Where in master it gives

```
time for i in {0..100}; do make --no-print-directory -C examples/hello-world/ clean  >/dev/null 2>/dev/null; done

real	0m14.826s
user	0m8.424s
sys	0m0.504s
```
</details>


#### The `RIOT_VERSION` has still the same behavior as before.

<details><summary>The `RIOT_VERSION` given to the program as the same value as before</summary>

Version includes branch name:

```
make --no-print-directory -C examples/hello-world/ info-debug-variable-RIOT_VERSION
2019.10-devel-100-g8859e-pr/git_version
```

In master it removes the branch name

```
$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'riot/master'.

$ git merge pr/git_version
Merge made by the 'recursive' strategy.
 Makefile.include             | 24 +++++++-----------------
 makefiles/git_version.inc.mk | 11 +++++++++++
 2 files changed, 18 insertions(+), 17 deletions(-)
 create mode 100644 makefiles/git_version.inc.mk

$ make --no-print-directory -C examples/hello-world/ info-debug-variable-RIOT_VERSION
2019.10-devel-108-g1ddf2
```

Outside of a git repository, the version is said to be unknown

```
$ mv .git .git_saved
$ make --no-print-directory -C examples/hello-world/ info-debug-variable-RIOT_VERSION
UNKNOWN (builddir: /home/harter/work/git/RIOT)
$ mv .git_saved/ .git
```

The value is given to the compiler in `CFLAGS_WITH_MACROS`

```
make --no-print-directory -C examples/hello-world/ info-debug-variable-CFLAGS_WITH_MACROS
ake --no-print-directory -C examples/hello-world/ info-debug-variable-CFLAGS_WITH_MACROS
-DDEVELHELP -Werror -Wall -Wextra -pedantic -std=gnu99 -m32 -fstack-protector-all -ffunction-sections -fdata-sections -DDEBUG_ASSERT_VERBOSE -DRIOT_APPLICATION="hello-world" -DBOARD_NATIVE="native" -DRIOT_BOARD=BOARD_NATIVE -DCPU_NATIVE="native" -DRIOT_CPU=CPU_NATIVE -DMCU_NATIVE="native" -DRIOT_MCU=MCU_NATIVE -fno-delete-null-pointer-checks -fdiagnostics-color -Wstrict-prototypes -Wold-style-definition -fno-common -Wall -Wextra -Wformat=2 -Wformat-overflow -Wformat-truncation -Wmissing-include-dirs -DMODULE_AUTO_INIT -DMODULE_BOARD -DMODULE_CORE -DMODULE_CORE_MSG -DMODULE_CPU -DMODULE_NATIVE_DRIVERS -DMODULE_PERIPH -DMODULE_PERIPH_COMMON -DMODULE_PERIPH_GPIO -DMODULE_PERIPH_PM -DMODULE_PERIPH_UART -DMODULE_SYS -DRIOT_VERSION="2019.10-devel-100-g8859e-pr/git_version"
```

And overwritten when using `RIOT_CI_BUILD=1`

```
RIOT_CI_BUILD=1 make --no-print-directory -C examples/hello-world/ info-debug-variable-CFLAGS_WITH_MACROS
-DDEVELHELP -Werror -Wall -Wextra -pedantic -std=gnu99 -m32 -fstack-protector-all -ffunction-sections -fdata-sections -DDEBUG_ASSERT_VERBOSE -DRIOT_APPLICATION="hello-world" -DBOARD_NATIVE="native" -DRIOT_BOARD=BOARD_NATIVE -DCPU_NATIVE="native" -DRIOT_CPU=CPU_NATIVE -DMCU_NATIVE="native" -DRIOT_MCU=MCU_NATIVE -fno-delete-null-pointer-checks -fdiagnostics-color -Wstrict-prototypes -Wold-style-definition -fno-common -Wall -Wextra -Wformat=2 -Wformat-overflow -Wformat-truncation -Wmissing-include-dirs -DMODULE_AUTO_INIT -DMODULE_BOARD -DMODULE_CORE -DMODULE_CORE_MSG -DMODULE_CPU -DMODULE_NATIVE_DRIVERS -DMODULE_PERIPH -DMODULE_PERIPH_COMMON -DMODULE_PERIPH_GPIO -DMODULE_PERIPH_PM -DMODULE_PERIPH_UART -DMODULE_SYS -DRIOT_VERSION="buildtest"
```
</details>

### Issues/PRs references

A real fix compared to only the workaround done in https://github.com/RIOT-OS/RIOT/pull/11771
Tracking: remove harmful use of `export` in make and immediate evaluation #10850
You can view, comment on, or merge this pull request online at:

  https://github.com/RIOT-OS/RIOT/pull/11881

-- Commit Summary --

  * makefiles/git_version.inc.mk: deferred GIT_VERSION definition
  * Makefile.include: use GIT_VERSION for RIOT_VERSION
  * Makefile.include: Only evaluate RIOT_VERSION when needed

-- File Changes --

    M Makefile.include (24)
    A makefiles/git_version.inc.mk (11)

-- Patch Links --

https://github.com/RIOT-OS/RIOT/pull/11881.patch
https://github.com/RIOT-OS/RIOT/pull/11881.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11881
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190722/d041fe85/attachment-0001.htm>


More information about the notifications mailing list