Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makefile.include: remove RIOT_VERSION_OVERRIDE #11771

Merged

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Jul 1, 2019

Contribution description

RIOT_VERSION is not used for building a specific RIOT version anymore
since #10543. The variable can now be used directly.

This now depends on #11881 which prevents calling git describe

When building with RIOT_CI_BUILD=1 this also now prevents calling
git describe and git rev-parse which saves around 0.1 seconds
per execution on my machine.

Testing procedure

No more usage in RIOT reported by git grep RIOT_VERSION_OVERRIDE

The buildtest version is correctly used for CI builds

RIOT_CI_BUILD=1 make --no-print-directory -C examples/hello-world/ info-debug-variable-RIOT_VERSION info-debug-variable-CFLAGS_WITH_MACROS
buildtest
-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"

The version is used when given in the environment

RIOT_VERSION=beer make --no-print-directory -C examples/hello-world/ info-debug-variable-RIOT_VERSION info-debug-variable-CFLAGS_WITH_MACROS 
beer
-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="beer"
Speed update given by #11881 directly

Doing nothing is way faster for me, let's see the difference in CI, as in the RIOT_CI_BUILD=1 context, the value from git describe is completely unnecessary.

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

real    0m19.081s
user    0m17.045s
sys     0m2.721s
time for i in {0..100}; do RIOT_CI_BUILD=1 make --no-print-directory -C examples/hello-world/ info-debug-variable-ABC >/dev/null 2>/dev/null; done

real    0m9.405s
user    0m7.987s
sys     0m2.092s

Issues/PRs references

Tracking: remove harmful use of export in make and immediate evaluation #10850
Depends on #11881
Makefile.include: remove functionality to build with another version. #10543

@cladmi cladmi added Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 1, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Jul 1, 2019

Not sure if the value is accurate, but compared to #11478, that also did not change any source code:

The build time went down 4 hours

@cladmi cladmi requested review from kaspar030 and smlng July 1, 2019 17:30
@cladmi
Copy link
Contributor Author

cladmi commented Jul 1, 2019

EDIT: The whole case is now optimized by being based on #11881

This currently optimize the CI build and not cases where RIOT_VERSION is unused by a target. It is a different task that I plan to do now that #11664 is merged, tracked by #10850

Only evaluating when needed would not help murdock as it sets RIOT_CI_BUILD anyway.

@kaspar030 kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 1, 2019
@kaspar030
Copy link
Contributor

I'm about to ACK. Should we add some kind of deprecation warning? I know my finger muscles will type RIOT_VERSION_OVERRIDE for a long time...

@miri64
Copy link
Member

miri64 commented Jul 2, 2019

👍 for warning when RIOT_VERSION_OVERRIDE is set.

@cladmi
Copy link
Contributor Author

cladmi commented Jul 8, 2019

I thought it was not needed for configuration changes, I will add it.

@cladmi
Copy link
Contributor Author

cladmi commented Jul 8, 2019

I handled it with a warning and kept the old behavior when set.
Should I put a removal date ?

It will still evaluate git describe as I wanted to put the handling near original usage.

Testing procedure for the deprecation handling:

RIOT_VERSION_OVERRIDE=manually_set make --no-print-directory -C examples/hello-world/ info-debug-variable-RIOT_VERSION info-debug-variable-CFLAGS_WITH_MACROS 
/home/harter/work/git/worktree/riot_test/examples/hello-world/../../Makefile.include:723: 'RIOT_VERSION_OVERRIDE' is deprecated, it can now be set with 'RIOT_VERSION' directly.
manually_set
-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="manually_set"

@cladmi
Copy link
Contributor Author

cladmi commented Jul 22, 2019

Rebased on #11881 to provide the speed boost also when not using RIOT_VERSION even without RIOT_CI_BUILD

@miri64 miri64 added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jul 26, 2019
@miri64
Copy link
Member

miri64 commented Jul 26, 2019

I just merged #11881. Please rebase again (or close...? #11881 said it was an alternative to this so why is it included here? ❓ ).

@cladmi
Copy link
Contributor Author

cladmi commented Jul 29, 2019

Indeed, I thought about rebasing this one after writing the other PR description so ended up being not consistent with the introduction sentence.

The speed improvement is now not provided by this PR anymore and it just replaces RIOT_VERSION_OVERRIDE with RIOT_VERSION.

@cladmi cladmi force-pushed the pr/make/remove_riot_version_override branch from 3b7cef7 to e4d34ee Compare July 29, 2019 08:00
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. please squash

RIOT_VERSION is not used for building a specific RIOT version anymore
since RIOT-OS#10543. The variable can now be used directly.
Add deprecation warning.
@cladmi cladmi force-pushed the pr/make/remove_riot_version_override branch from 2fe4b83 to e04850b Compare August 12, 2019 14:29
@cladmi
Copy link
Contributor Author

cladmi commented Aug 12, 2019

Squashed.

Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@jcarrano jcarrano merged commit a4a1dc4 into RIOT-OS:master Aug 20, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Aug 20, 2019

Thank you for the review.

@cladmi cladmi deleted the pr/make/remove_riot_version_override branch August 20, 2019 14:58
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants