makefiles: Introduce GIT_VERSION and use it for RIOT_VERSION by cladmi · Pull Request #11881 · RIOT-OS/RIOT

cladmi · 2019-07-22T12:50:36Z

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. #11771 (comment)

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

There are three steps

Define GIT_VERSION that only calls 'git' when used. It will also only call each commands only once thanks to memoized
Use it for RIOT_VERSION.
At that point, when RIOT_VERSION_OVERRIDE is set, git will not be called for this.
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

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

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

In master

The value was always called

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

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:

Speed update from 14.5 to 7.5 seconds

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

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

The `RIOT_VERSION` given to the program as the same value as before

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
-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"

Issues/PRs references

A real fix compared to only the workaround done in #11771
Tracking: remove harmful use of export in make and immediate evaluation #10850

Move git version evaluation to a separate file. The definition has been updated it to a deferred only definition. This way git is not called when GIT_VERSION is not used.

Use 'GIT_VERSION' for 'RIOT_VERSION'. This prevents evaluating 'git describe' and 'git rev-parse' when their value is not used.

This changes CFLAGS_WITH_MACROS to be: * a deferred variable * not exported so only evaluated when actually used The value is only used by `Makefile.include` and `makefiles/eclipse.inc.mk` so not required to export it.

cladmi · 2019-07-22T13:13:52Z

This also speeds up running make distclean in the base repository.

After running disclean already to be in a clean state. It makes the time go from 44s to 21s on my build machine:

time make --no-print-directory distclean
...

real	0m21.764s
user	0m5.596s
sys	0m0.760s

Where it was in master:

time make --no-print-directory distclean
...

real	0m44.287s
user	0m26.008s
sys	0m1.980s

@aabadie I think you are using distclean in your workflow.

miri64

ACK, I tested building and running examples/hello-world

on master (should give e.g. 2019.10-devel-128-g7d381, i.e. should not append the branch name and no extra - at the end)
on a detached HEAD (should give e.g. 2019.10-devel-128-g7d381-HEAD)
on a branch (should give e.g. 2019.10-devel-128-g7d381-test-pr-11881, i.e. with the branch name appended, prefixed with a -)
on a release tag (should just print the release name e.g. 2019.07)
the content of a release tar ball (should just print the release name e.g. 2019.07).

For all cherry-picked the commits from this PR on top (or just applied as patch in the tar ball case git -C ../RIOT diff HEAD~3 > patch.txt; patch -p1 < patch.txt).

All had the expected output.

I tried to verify the speed updates you claimed:

In this PR

$ 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	0m35,414s
user	0m27,379s
sys	0m11,662s
$ 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	0m47,847s
user	0m37,648s
sys	0m13,471s
$ time for i in {0..100}; do RIOT_CI_BUILD=1 make --no-print-directory -C examples/hello-world/ clean >/dev/null 2>/dev/null; done

real	0m36,378s
user	0m27,814s
sys	0m12,369s

and a significant speed boost is visible when not using RIOT_VERSION (done on git reset --hard HEAD~3 form this PR as HEAD):

$ 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	0m48,188s
user	0m38,251s
sys	0m13,398s
$ 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	0m47,909s
user	0m38,107s
sys	0m13,474s
$ time for i in {0..100}; do RIOT_CI_BUILD=1 make --no-print-directory -C examples/hello-world/ clean >/dev/null 2>/dev/null; done

real	0m48,806s
user	0m38,538s
sys	0m13,919s

=> ACK

cladmi · 2019-07-29T07:05:16Z

Thank you for the review.

on a detached HEAD (should give e.g. 2019.10-devel-128-g7d381-HEAD)

on a release tag (should just print the release name e.g. 2019.07)

I did not thought about these ones, and the release tag is one of the most important one. I will do another PR to document these in git_version.inc.mk.

the content of a release tar ball (should just print the release name e.g. 2019.07).

I indeed forgot to show this was not changed. Good catch.

miri64 · 2019-07-29T07:16:00Z

on a detached HEAD (should give e.g. 2019.10-devel-128-g7d381-HEAD)

on a release tag (should just print the release name e.g. 2019.07)

I did not thought about these ones, and the release tag is one of the most important one. I will do another PR to document these in git_version.inc.mk.

Now that I think about it... It makes no sense, that they have a different result in describ than the devel tag... Maybe they were also just read from the VERSION file?

cladmi · 2019-07-29T07:42:09Z

Indeed, if there is a VERSION file, the RIOT_VERSION is taken from it even in git. It was how I designed and implemented it in the first place.

Now that you say it, with the current and previous behavior, the git version indeed has -HEAD for a tag:

make --no-print-directory -C examples/hello-world/ info-debug-variable-RIOT_VERSION
2019.10-devel-HEAD

I could propose to change this. Which then would make being on master and on HEAD show the same thing.

We could also remove the branch completely

When I noticed the branch name was in the version, it felt for me a bit off somehow. It is a source on non reproducibility and local machine information.

cladmi · 2019-07-29T07:45:16Z

When I noticed the branch name was in the version, it felt for me a bit off somehow. It is a source on non reproducibility and local machine information.

Personally when I clone a PR, I have my own naming scheme, so when building would have a different binary than the one on the developer machine, even when building in docker.

miri64 · 2019-07-29T07:48:36Z

I'm not sure either, why the branch name / the word HEAD needs to be in the version string... I'd say open a PR to change it (maybe write a mail to devel about it as well) and maybe someone will scream the reason why it is necessary ;-)

cladmi added 3 commits July 22, 2019 12:00

makefiles/git_version.inc.mk: deferred GIT_VERSION definition

21649b2

Move git version evaluation to a separate file. The definition has been updated it to a deferred only definition. This way git is not called when GIT_VERSION is not used.

Makefile.include: use GIT_VERSION for RIOT_VERSION

cfd4677

Use 'GIT_VERSION' for 'RIOT_VERSION'. This prevents evaluating 'git describe' and 'git rev-parse' when their value is not used.

Makefile.include: Only evaluate RIOT_VERSION when needed

8859e1e

This changes CFLAGS_WITH_MACROS to be: * a deferred variable * not exported so only evaluated when actually used The value is only used by `Makefile.include` and `makefiles/eclipse.inc.mk` so not required to export it.

cladmi changed the title ~~Pr/git version~~ makefiles: GIT_VERSION Jul 22, 2019

This was referenced Jul 22, 2019

Makefile.include: remove RIOT_VERSION_OVERRIDE #11771

Merged

Tracking: remove harmful use of export in make and immediate evaluation #10850

Closed

miri64 added Area: build system Area: Build system Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 26, 2019

miri64 changed the title ~~makefiles: GIT_VERSION~~ makefiles: Introduce GIT_VERSION and use it for RIOT_VERSION Jul 26, 2019

miri64 approved these changes Jul 26, 2019

View reviewed changes

miri64 merged commit 2d1eda5 into RIOT-OS:master Jul 26, 2019

cladmi deleted the pr/git_version branch July 29, 2019 07:06

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

cladmi mentioned this pull request Oct 17, 2019

Release 2019.10 - RC1 RIOT-OS/Release-Specs#140

Closed

80 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

makefiles: Introduce GIT_VERSION and use it for RIOT_VERSION#11881

makefiles: Introduce GIT_VERSION and use it for RIOT_VERSION#11881
miri64 merged 3 commits intoRIOT-OS:masterfrom
cladmi:pr/git_version

cladmi commented Jul 22, 2019 •

edited

Loading

Uh oh!

cladmi commented Jul 22, 2019 •

edited

Loading

Uh oh!

miri64 left a comment •

edited

Loading

Uh oh!

cladmi commented Jul 29, 2019

Uh oh!

miri64 commented Jul 29, 2019

Uh oh!

cladmi commented Jul 29, 2019

Uh oh!

cladmi commented Jul 29, 2019

Uh oh!

miri64 commented Jul 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

cladmi commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

State at pull request

Step 2, when CFLAGS_WITH_MACROS was still evaluated

In master

Speed update

The RIOT_VERSION has still the same behavior as before.

Issues/PRs references

Uh oh!

cladmi commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Jul 29, 2019

Uh oh!

miri64 commented Jul 29, 2019

Uh oh!

cladmi commented Jul 29, 2019

Uh oh!

cladmi commented Jul 29, 2019

Uh oh!

miri64 commented Jul 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cladmi commented Jul 22, 2019 •

edited

Loading

Step 2, when `CFLAGS_WITH_MACROS` was still evaluated

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

cladmi commented Jul 22, 2019 •

edited

Loading

miri64 left a comment •

edited

Loading