Makefile.include: remove rule to remake PKGs' Makefile.include by jcarrano · Pull Request #10344 · RIOT-OS/RIOT

jcarrano · 2018-11-07T14:50:25Z

Contribution description

This rule is not being used, it complicates the makefile and causes make clean to permform unnecessary actions.

All packages have a Makefile.include, so the rule is not needed anyways:

Generating a file in the source tree is bad practice.
The contents of Makefile.include should be fixed.

Also, it is defined with a double colon for no reason.

Testing procedure

Everything should continue to work as always (this can be tested by the CI.)

Run make clean from the RIOT base with and without this commit. Without this commit you will see that there are several Makefile.include that are processed for nothing.

Issues/PRs references

Undoes part of #576.
Depends on #10456 (fixes weird make concurrency issues).

cladmi · 2018-11-13T14:52:33Z

Makefile.include

-$(RIOTPKG)/%/Makefile.include::
-	$(Q)"$(MAKE)" -C $(RIOTPKG)/$* Makefile.include
-
 $(USEPKG:%=$(RIOTPKG)/%/Makefile.include): FORCE


This next line should go away too.

cladmi · 2018-11-13T15:00:21Z

This is in theory an API change as a package could rely on this rule to define a dynamic Makefile.include but I agree it should not be done this way. A dynamic behavior depending on the current board can still be done Makefile.include directly.

Also currently, no packages are using this at all as they all define a Makefile.include:

for pkg in  pkg/*/; do test -f ${pkg}/Makefile.include || echo NO MAKEFILE.INCLUDE for ${pkg}; done
# nothing

even if a package could be without a Makefile.include without problem, if it only provides an implementation for an API that is in RIOT so does not need any additional header path.

cladmi · 2018-11-13T15:07:03Z

This also removes unneeded output

make --no-print-directory -C tests/unittests/ clean
make: Nothing to be done for 'clean'.

In master.

make --no-print-directory -C tests/unittests/ clean
make[1]: Nothing to be done for 'Makefile.include'.
make[1]: Nothing to be done for 'Makefile.include'.
make[1]: Nothing to be done for 'Makefile.include'.
make[1]: Nothing to be done for 'Makefile.include'.
make[1]: Nothing to be done for 'Makefile.include'.
make: Nothing to be done for 'clean'.

I even needed to remove them for the Release-Specs test script:

https://github.com/RIOT-OS/Release-Specs/blob/a89920e1d9feb078f54aca56deebd9c04d10266c/02-tests/compile_and_test_for_board.py#L267-L276

cladmi

By checking for other usage of RIOTPKG/*/Makefile.include I found another line that should be cleaned: (git grep -n Makefile.include | grep PKG)

 all $(BASELIBS) $(USEPKG:%=$(RIOTPKG)/%/Makefile.include) $(BUILDDEPS): clean

https://github.com/RIOT-OS/RIOT/pull/10344/files#diff-ebbb3165b3276a049759e5cb4e04d6a4R482

You can squash directly with this change. I would do a last check after but I should be good to ACK.

jcarrano · 2018-11-22T15:58:01Z

I rebased on top of #10456 . I should have just cherry-picked those three commits, but it's done already.

cladmi · 2018-11-23T14:47:42Z

Please rebase as #10456 is merged.

jcarrano · 2018-11-23T14:56:26Z

Rebased.

cladmi · 2018-11-23T15:03:58Z

To notify next developers ending up here by bisecting, this PR could introduce other issues where running make clean all -j breaks the build. The issue is, I think, just revealed by this PR but was already there before like in #10456.
But it worth pinging here for help.

cladmi · 2018-11-23T16:05:49Z

I will try running the compilation all applications for native and samr21-xpro with a sleep 5 as first command in clean to find any other possible issues.

cladmi · 2018-11-23T16:22:02Z

I currently cannot build in docker with this as the clean trick is not done for docker.

cladmi · 2018-11-23T16:22:08Z

I will do a PR.

cladmi · 2018-12-06T14:33:25Z

I am re-running the local tests rebased on top of #10461 .

cladmi · 2018-12-07T16:41:44Z

I could re-run all compilation in parallel for native and samr21-xpro

BUILD_IN_DOCKER=1 DOCKER="sudo ~/.bin/sudo_docker" ./compile_and_test_for_board.py --jobs 0 --no-test  ~/work/git/RIOT/ samr21-xpro

And also without docker.

Except one error that is unrelated, and was there before that I will fix in a dedicated PR.

Please rebase as Makefile.include conflicts.

jcarrano · 2018-12-07T16:48:47Z

Rebased.

This rule is not being used, it complicates the makefile and causes make clean to permform unnecessary actions. All packages have a Makefile.include, so the rule is not needed anyways. Also, it is defined with a double colon for no reason.

This was addressed.

cladmi

ACK I agree with removing these Makefile.include targets.

Also the parallel building was tested.

If there are a need for something like this, it should be handled in the pkg/Makefile.include itself.

jcarrano · 2018-12-07T16:54:52Z

it should be handled in the pkg/Makefile.include itself.

But if it does, it should not create or modify files in the source tree.

cladmi · 2018-12-07T16:58:18Z

You can directly merge when murdock is green.

jcarrano added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Nov 7, 2018

jcarrano requested a review from cladmi November 7, 2018 14:50

cladmi reviewed Nov 13, 2018

View reviewed changes

cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 13, 2018

cladmi previously requested changes Nov 22, 2018

View reviewed changes

jcarrano force-pushed the makefile.include-cannot-be-made branch from 41dad19 to 71747bf Compare November 22, 2018 13:40

jcarrano mentioned this pull request Nov 22, 2018

examples/{javascript/lua}: correctly declare rules to embed scripts. #10456

Merged

jcarrano force-pushed the makefile.include-cannot-be-made branch from 71747bf to 1a08387 Compare November 22, 2018 15:56

jcarrano added the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 22, 2018

jcarrano mentioned this pull request Nov 23, 2018

make: make clean all does not make sense and should be removed #10459

Closed

4 tasks

jcarrano force-pushed the makefile.include-cannot-be-made branch from 1a08387 to 59f2e48 Compare November 23, 2018 14:56

cladmi mentioned this pull request Nov 23, 2018

Makefile.include: building in docker should be done after clean #10461

Merged

jcarrano force-pushed the makefile.include-cannot-be-made branch from 59f2e48 to 8945a95 Compare December 7, 2018 16:48

cladmi mentioned this pull request Dec 7, 2018

tests: compile-tests should only be executed in building machine #10573

Merged

cladmi approved these changes Dec 7, 2018

View reviewed changes

cladmi merged commit cd35edd into RIOT-OS:master Dec 7, 2018

aabadie added this to the Release 2019.01 milestone Dec 19, 2018

jcarrano mentioned this pull request Dec 19, 2018

makefiles/docker.inc.mk: Use directories in RIOT when possible #9646

Merged

Conversation

jcarrano commented Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

cladmi Nov 13, 2018

Choose a reason for hiding this comment

Uh oh!

jcarrano Nov 15, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi commented Nov 13, 2018

Uh oh!

cladmi commented Nov 13, 2018

Uh oh!

cladmi 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!

jcarrano commented Nov 22, 2018

Uh oh!

cladmi commented Nov 23, 2018

Uh oh!

jcarrano commented Nov 23, 2018

Uh oh!

cladmi commented Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cladmi commented Nov 23, 2018

Uh oh!

cladmi commented Nov 23, 2018

Uh oh!

cladmi commented Nov 23, 2018

Uh oh!

cladmi commented Dec 6, 2018

Uh oh!

cladmi commented Dec 7, 2018

Uh oh!

jcarrano commented Dec 7, 2018

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

jcarrano commented Dec 7, 2018

Uh oh!

cladmi commented Dec 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jcarrano commented Nov 7, 2018 •

edited

Loading

cladmi left a comment •

edited

Loading

cladmi commented Nov 23, 2018 •

edited

Loading