Skip to content

Conversation

@fjmolinas
Copy link
Contributor

Contribution description

This PR moves BOARD and CPU Makefile includes after dependency resolution.

Testing procedure

  • green murdock

  • run time ./dist/tools/buildsystem_sanity_check/save_all_dependencies_resolution_variables.sh on this PR and master. There should be no diff. I ran it on a version of this branch rebased on sys/arduino: include arduino_sketches in Makefile.dep #14350. But I think that shouldn't matter since that module was included after dependency resolution already so result should not be affected by this PR.

I'm ticking it as major since it affects all BOARD CPU, but IMO the change if fine if no dependencies have changed :)

Issues/PRs references

Ticks an item in #9913

@fjmolinas fjmolinas added Area: build system Area: Build system Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 25, 2020
@fjmolinas fjmolinas requested a review from haukepetersen as a code owner June 25, 2020 07:37
@fjmolinas fjmolinas requested review from kaspar030 and maribu June 25, 2020 07:39
@leandrolanzieri leandrolanzieri added this to the Release 2020.07 milestone Jun 25, 2020
@aabadie
Copy link
Contributor

aabadie commented Jun 25, 2020

There are a couple of other build system tricks that could (or should ?) be removed as well:

EEPROM_FILE_FLAGS = $(if $(filter periph_eeprom,$(FEATURES_USED)),--eeprom $(EEPROM_FILE),)

CFLAGS_FPU ?= $(if $(filter cortexm_fpu,$(USEMODULE)),$(_CORTEX_HW_FPU_CFLAGS),-mfloat-abi=soft)

I don't know if they should be part of this PR or if there are others.

@leandrolanzieri
Copy link
Contributor

Some others:

# NOTE: These checks can be turned into normal conditionals when #9913 is fixed
# use 32 priority levels if any WiFi interface or the ETH interface is used
CFLAGS += $(if $(filter esp_wifi_any esp_eth,$(USEMODULE)),-DSCHED_PRIO_LEVELS=32)

# override default CFLAGS_OPT in case module esp_gdb is enabled
CFLAGS_OPT ?= $(if $(filter esp_gdb,$(USEMODULE)),-Og,-Os)

# NOTE: These checks can be turned into normal conditionals when #9913 is fixed
# increase the test timeout for file system tests that use the SPI flash drive
RIOT_TEST_TIMEOUT = $(if $(filter spiffs littlefs,$(USEMODULE)),120)

# NOTE: These can be turned into normal conditionals after #9913 is fixed
GDBSTUB_DIR ?= $(if $(filter esp_gdbstub,$(USEMODULE)),$(RIOTCPU)/$(CPU)/vendor/esp-gdbstub)
CFLAGS += $(if $(filter esp_gdbstub,$(USEMODULE)),-DGDBSTUB_BREAK_ON_INIT=1)
INCLUDES += $(if $(filter esp_gdbstub,$(USEMODULE)),-I$(GDBSTUB_DIR))
BASELIBS += $(if $(filter esp_now, $(USEMODULE)), -lespnow)

INCLUDES += $(if $(filter esp_eth,$(USEMODULE)),-I$(RIOTCPU)/$(CPU)/vendor/esp-idf/include/ethernet)
INCLUDES += $(if $(filter esp_eth,$(USEMODULE)),-I$(ESP32_SDK_DIR)/components/ethernet/include)

# NOTE: These checks can be turned into a normal conditional when #9913 is fixed
BASELIBS += $(if $(filter esp_wifi_any,$(USEMODULE)),$(_ESP_WIFI_ANY_BASELIBS))
BASELIBS += $(if $(filter esp_now,$(USEMODULE)),$(_ESP_NOW_BASELIBS))

@fjmolinas
Copy link
Contributor Author

@leandrolanzieri should I do those changes in this PR? IMO we could do it in a followup, and can come after the feature freeze.

@leandrolanzieri
Copy link
Contributor

@leandrolanzieri should I do those changes in this PR? IMO we could do it in a followup, and can come after the feature freeze.

I'm OK with having that on a followup

@fjmolinas
Copy link
Contributor Author

@leandrolanzieri should I do those changes in this PR? IMO we could do it in a followup, and can come after the feature freeze.

I'm OK with having that on a followup

I'll open right away.

@leandrolanzieri
Copy link
Contributor

I'm running the scripts now...

@maribu
Copy link
Member

maribu commented Jun 25, 2020

Nice one :-)

@fjmolinas
Copy link
Contributor Author

@leandrolanzieri found a diff where test_utils_interactive_sync was not included for a couple of applications.

@aabadie
Copy link
Contributor

aabadie commented Jun 25, 2020

@leandrolanzieri found a diff where test_utils_interactive_sync was not included for a couple of applications.

Is this problematic from your point of view ?

@fjmolinas
Copy link
Contributor Author

@leandrolanzieri I rand it again and I only get a diff in FEATURES_REQUIRED which comes from 9dea6a1 being removed and therefore there not being feature duplication. But FEATURES_REQUIRED_SORTED is all good.

@leandrolanzieri
Copy link
Contributor

@leandrolanzieri found a diff where test_utils_interactive_sync was not included for a couple of applications.

This is not really an issue. It was happening because of my local setup. Disregard.

@leandrolanzieri
Copy link
Contributor

@leandrolanzieri I rand it again and I only get a diff in FEATURES_REQUIRED which comes from 9dea6a1 being removed and therefore there not being feature duplication. But FEATURES_REQUIRED_SORTED is all good.

It would be good to post the diff for the record

@fjmolinas
Copy link
Contributor Author

Here is the diff I had: diff.txt

@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels Jun 26, 2020
@leandrolanzieri leandrolanzieri added Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jun 26, 2020
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

As explained in #9913 this change makes sense as it helps resolving the problem of setting certain configurations depending on the used modules.

@fjmolinas provided the diff that shows that there are no impacts in dependency resolution. From my side it's an ACK.

Maybe we can have a second one? @aabadie?

@miri64 miri64 added the Process: needs >1 ACK Integration Process: This PR requires more than one ACK label Jun 26, 2020
@aabadie
Copy link
Contributor

aabadie commented Jun 26, 2020

Maybe we can have a second one?

I'm all for this change and I trust your results ! ACK

@leandrolanzieri
Copy link
Contributor

And GO!

@leandrolanzieri leandrolanzieri merged commit 8c7b677 into RIOT-OS:master Jun 26, 2020
@fjmolinas fjmolinas deleted the pr_reorder_makefiles branch June 26, 2020 08:48
@fjmolinas
Copy link
Contributor Author

Awesome to get this finally done, finishing what @cladmi started! Thanks for all the help along the way @leandrolanzieri @aabadie!

@cladmi
Copy link
Contributor

cladmi commented Jun 27, 2020

Congrats

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 Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Process: needs >1 ACK Integration Process: This PR requires more than one ACK Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines 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.

6 participants