Skip to content

Conversation

@jcarrano
Copy link
Contributor

@jcarrano jcarrano commented Nov 7, 2018

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:

  1. Generating a file in the source tree is bad practice.
  2. 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).

@jcarrano 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 jcarrano requested a review from cladmi November 7, 2018 14:50
Makefile.include Outdated
$(RIOTPKG)/%/Makefile.include::
$(Q)"$(MAKE)" -C $(RIOTPKG)/$* Makefile.include

$(USEPKG:%=$(RIOTPKG)/%/Makefile.include): FORCE
Copy link
Contributor

Choose a reason for hiding this comment

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

This next line should go away too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@cladmi 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
Copy link
Contributor

cladmi commented Nov 13, 2018

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
Copy link
Contributor

cladmi commented Nov 13, 2018

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
cladmi previously requested changes Nov 22, 2018
Copy link
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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

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

cladmi commented Nov 23, 2018

Please rebase as #10456 is merged.

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

Rebased.

@cladmi
Copy link
Contributor

cladmi commented Nov 23, 2018

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 cladmi 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 State: waiting for other PR State: The PR requires another PR to be merged first labels Nov 23, 2018
@cladmi
Copy link
Contributor

cladmi commented Nov 23, 2018

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
Copy link
Contributor

cladmi commented Nov 23, 2018

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

@cladmi
Copy link
Contributor

cladmi commented Nov 23, 2018

I will do a PR.

@cladmi
Copy link
Contributor

cladmi commented Dec 6, 2018

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

@cladmi
Copy link
Contributor

cladmi commented Dec 7, 2018

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 jcarrano force-pushed the makefile.include-cannot-be-made branch from 59f2e48 to 8945a95 Compare December 7, 2018 16:48
@jcarrano
Copy link
Contributor Author

jcarrano commented Dec 7, 2018

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.
Copy link
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

jcarrano commented Dec 7, 2018

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
Copy link
Contributor

cladmi commented Dec 7, 2018

You can directly merge when murdock is green.

@cladmi cladmi merged commit cd35edd into RIOT-OS:master Dec 7, 2018
@aabadie aabadie added this to the Release 2019.01 milestone Dec 19, 2018
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: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants