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

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

Open
cladmi opened this issue Jan 23, 2019 · 1 comment
Open

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

cladmi opened this issue Jan 23, 2019 · 1 comment

Comments

@cladmi
Copy link
Contributor

@cladmi cladmi commented Jan 23, 2019

Description

Many variables are exported using export VARIABLE without even knowing why, the impact, or even be needed. In the same time, many variables are evaluated using := instead of using deferred variables.

Guess why calling make clean is slow ? All these variables/shell call must be evaluated for each target called even if the targett is not using it at all.

I tried preventing changing these without reasons, but keeping them in this wrong state by default, is causing me issues in several places. I want to start getting rid of them on a methodical way instead of currently working it around.

Listing them all

git grep -a -n 'export ' '*' ':!doc/**' ':!**/*.c' ':!**/*.h' ':!**/*.py' ':!makefiles/utils/**' ':!*.murdock' ':!dist/tools/buildsystem_sanity_check/**' ':!*release-notes.txt' ':!dist/tools/**' | tee output
wc -l output; date
1105
Fri May 17 13:37:10 CEST 2019
wc -l output ; date
938 output
Tue May 28 18:23:42 CEST 2019
wc -l output ; date  # Pre rule update
940 output
Mon Jun  3 10:01:49 CEST 2019
wc -l output ; date  # Grep rule updated
892 output
Mon Jun  3 10:04:58 CEST 2019
wc -l output; date
854 output
Mon Jun  3 10:46:56 CEST 2019
wc -l output; date
830 output
Tue Jun  4 16:04:14 CEST 2019
wc -l output; date
822 output
Tue Jun 11 13:31:51 CEST 2019
wc -l output; date
619 output
Wed Aug 21 20:17:48 CEST 2019
wc -l output; date  # rule update to see in patches and match 'export '
618 output
Thu Aug 29 11:51:28 CEST 2019
wc -l output; date
498 output
Thu Aug 29 14:39:32 CEST 2019

wc -l output ; date
503 output
Thu Sep 12 14:47:10 CEST 2019

I am personally tracking them in https://ci-ilab.imp.fu-berlin.de/job/RIOT%20cleanup%20warnings/

Steps

  • List variables where export or direct evaluation can directly be removed here == only used inside Makefile.include and remove them

    • LINKFLAGS
      • global export vars.inc.mk #10853
      • atmega_common #10854
      • Other platforms:
        git grep 'export LINKFLAGS' | wc -l
        37
        
    • LINK
      • need to export it for generate-xcompile-toolchain.sh so for building packages in general, it is also using LFLAGS which is not mapped to LINKFLAGS.
    • CFLAGS/INCLUDES/variables used by Makefile.base #12098
      • Static instead of dynamic checking for supported variables #12123 (not set for --std=c99 yet)
    • PORT/FFLAGS/TERMPROG/TERMFLAGS/FLASHER/...
      • TERMPROG TERMFLAGS:
        • unexport TERM* variables #11064
        • boards/nz32-sc151/Makefile.include:export TERMFLAGS = -p $(PORT) #11065
      • FLASHER/FFLAGS/DEBUGGER/DEBUGGER_FLAGS/DEBUGSERVER/DEBUGSERVER_FLAGS/RESET/RESET_FLAGS #11554
      • PORT #10440
        • dist/tools/buildsystem_sanity_check: check no PORT exports #12209
      • PORT_LINUX|PORT_DARWIN #11619
    • DEBUG_* and STLINK variables #11614
    • RIOT_VERSION Running the git command on my machine takes currently 0.1 seconds. Everytime make is run.
      • Remove exporting and immediate evaluation of CFLAGS_WITH_MACROS and only evaluate git describe command when needeed #11881
        • Follow-up: Makefile.include: remove RIOT_VERSION_OVERRIDE #11771
    • Unexporting CPU/CPU_MODEL from the board files is done together in #11477 (still exported in vars.inc.mk)
  • Only export in makefiles/vars.inc.mk for variables that currently really need to be exported

  • Variables that need to be exported to sub-builds need a mechanism to say who gets it

  • makefiles/utils/variables: add functions to help managing variables #11664

  • Variables that are transparently given to scripts even if only one needs it

    • GIT_CACHE_DIR
    • flashing tools specific variables which are exported in board instead of in the flasher

Cleaning up

This has different consequences:

Evaluation when not needed
  • Using an export on a variable means the value will be evaluated even for rules that do not use the variable.
    This causes issues for example with listing tty devices, evaluating INCLUDES and getting GCC_INCLUDES_DIRS

  • It also causes evaluation on the host machine when only the docker container should be doing it. It happened with avr-ld for example, some checks being performed on the host machine, saying objcopy not found.

Sub builds

When trying to call make inside of make many variables from the first build are sent to the second one.

Variables that can change a build

As everything is exported, it is hard to know what can have an impact on a build result.
Having it in control would simplify documenting what should and what should not be available.

The default behavior has become harmful

The default case has been to always use := or export even when it does not make sense, and it gets merged without even reviewing why there is an export there.

export PRIVATE_VAR_FOR_SCRIPT = foo_bar
OTHER_VAR_ONLY_USE_OF_PRIVATE_VAR_FOR_SCRIPT = $(PRIVATE_VAR_FOR_SCRIPT)

Examples

  • GLOBAL_GOALS has it should not evaluate the rest of the build system because it is slow or causes isssues
  • #10840
  • #7695 (comment)

Related issues

I am starting to build on a machine that has nothing but docker and the build system complains about these things.

@cladmi

This comment has been minimized.

Copy link
Contributor Author

@cladmi cladmi commented Feb 25, 2019

Exporting TERMPROG and TERMFLAGS for nothing breaks setting RIOT_TERMINAL from within testrunner as tried in #10952. And such for no reason !

@cladmi cladmi changed the title Tracking: remove harmfull use of `export` in make and immediate evaluation Tracking: remove harmful use of `export` in make and immediate evaluation Jun 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.