Skip to content

makefiles/modules.inc.mk: refactoring#11109

Merged
jcarrano merged 5 commits intoRIOT-OS:masterfrom
cladmi:pr/make/refactor/modules.inc.mk
Mar 11, 2019
Merged

makefiles/modules.inc.mk: refactoring#11109
jcarrano merged 5 commits intoRIOT-OS:masterfrom
cladmi:pr/make/refactor/modules.inc.mk

Conversation

@cladmi
Copy link
Contributor

@cladmi cladmi commented Mar 5, 2019

Contribution description

This pull request does some refactoring in makefiles/modules.inc.mk.

  • remove unnecessary exports
  • factorize $(sort $(USEMODULE) $(USEPKG))
  • re-organize by usage
  • Change the way REALMODULES is defined

Testing procedure

The value of EXTDEFINES and REALMODULES should be the same in master and this pull request for all applications and boards:

BOARD=samr21-xpro make --no-print-directory -C riot_master/examples/default info-debug-variable-EXTDEFINES
BOARD=samr21-xpro make --no-print-directory -C RIOT/examples/default info-debug-variable-EXTDEFINES
BOARD=samr21-xpro make --no-print-directory -C riot_master/examples/default info-debug-variable-REALMODULES
BOARD=samr21-xpro make --no-print-directory -C RIOT/examples/default info-debug-variable-REALMODULES

I provided an automated test script for this:

https://gist.github.com/cladmi/9818888da7412a9195ec0d4c3c2a8b8a

Both output should not report any difference

./compare_riot_repositories_variable_value.sh riot_master riot_pr EXTDEFINES
./compare_riot_repositories_variable_value.sh riot_master riot_pr REALMODULES

There are currently /bin/sh: 1: Syntax error: Missing '))' messages but they are unrelated.

Issues/PRs references

I did this cleanup locally a long time ago to better understand this file, and now I want to use this as a pre-requisite to introducing LIBS for #11111 #8711

cladmi added 5 commits March 5, 2019 15:45
USEMODULE is already exported by `makefiles/vars.inc.mk`.
BASELIBS is used only in the main Makefile.include or included files.
As it is not used in sub make executions or scripts it does not need to
be exported.
Factorize the reused value in a private variable.

I define it private as somehow 'USEPKG' is supposed to be removed so the
variables can be removed later.
Put all lines related to CFLAGS handling together.
Refactor to define REALMODULES incrementally without overwriting
'NO_PSEUDOMODULES'.

This also allows an external makefile to maybe add something there if it really
needs to.
@cladmi cladmi added Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Mar 5, 2019
@cladmi cladmi added this to the Release 2019.04 milestone Mar 5, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Mar 5, 2019

I am currently comparing both outputs and will report any issues found.

@cladmi
Copy link
Contributor Author

cladmi commented Mar 6, 2019

My test script finds no changed values:

cat extdefines realmodules  | grep -c differ
0

@jcarrano jcarrano self-requested a review March 6, 2019 13:03
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.

Tested. Works.

@jcarrano jcarrano added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 11, 2019
@jcarrano jcarrano merged commit 265a118 into RIOT-OS:master Mar 11, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Mar 11, 2019

Thanks for the review. I will update the pull requests that depend on it.

@cladmi cladmi deleted the pr/make/refactor/modules.inc.mk branch March 11, 2019 13:27
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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants