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.

Sign up for GitHub

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

Jump to bottom

make: improve module dependencies #5891

Merged
merged 5 commits into from
Oct 19, 2016
Merged

make: improve module dependencies #5891

merged 5 commits into from
Oct 19, 2016

Conversation

kaspar030
Copy link
Contributor

This PR tries to improve our module dependency system:

  • Makefile.dep is now recursively evaluated until all transitive dependencies are sorted out
  • Makefile.dep now implicitly includes boards/$board/Makefile.dep (if it exists)
  • Makefile.dep now implicitly includes boards/$pkg/Makefile.dep (if it exists) for each package, allowing packages to have dependencies without modifying the main Makefile.dep

I've adapted some packages, but would like to hear comments for the approach in general before investing more work.

@kaspar030 kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: build system Area: Build system Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Sep 29, 2016
@jnohlgard
Copy link
Member

Is it safe to include makefiles recursively?
Would it be possible to rewrite the iteration into a loop somehow?
Did you test what happens if you make an infinite recursion? Does gnu make error out or will it run until it grows out of stack space?

@kaspar030
Copy link
Contributor Author

Is it safe to include makefiles recursively?

I think yes...

Would it be possible to rewrite the iteration into a loop somehow?

I don't know how to, with make. No loop constructs...

Did you test what happens if you make an infinite recursion? Does gnu make error out or will it run until it grows out of stack space?

I did some simple tests, the number of open files seems to be the recursion limit:

[kaspar@booze tmp]$ cat x.mk
include x.mk
[kaspar@booze tmp]$ make -f x.mk
x.mk:1: *** Too many open files.  Stop.
[kaspar@booze tmp]$

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 7, 2016
@kaspar030
Copy link
Contributor Author

ping

@kaspar030 kaspar030 removed Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Oct 18, 2016
@kaspar030
Copy link
Contributor Author

Unmarked RFC and WIP as everything compiles well.

miri64
miri64 approved these changes Oct 19, 2016
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Confirmed (tested with with some more complex applications). ACK & go

@miri64 miri64 merged commit bef0231 into RIOT-OS:master Oct 19, 2016
@kaspar030 kaspar030 deleted the improve_module_deps branch October 19, 2016 10:52
miri64
miri64 reviewed Oct 19, 2016
View reviewed changes
Makefile.dep
@@ -555,3 +530,13 @@ ifneq (,$(filter random,$(USEMODULE)))
USEMODULE += tinymt32
endif
endif

# include package dependencies
-include $(USEPKG:%=$(RIOTPKG)/%/Makefile.dep)
Copy link
Member

Choose a reason for hiding this comment

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

Arghs... Should have looked at this more careful. Now one NEEDS to mention a packet right? (except lwIP which is still above for some weird reason).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. lwip has it's dependencies still in the main Makefile.dep, that's why no explicit "USEPKG" is needed.

Copy link
Contributor Author

@kaspar030 kaspar030 Oct 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I thought that is actually cleaner than before.
It would be possible, though, to USEPKG all modules edit from USEMODULE edit that have a directory in pkg/.

Copy link
Member

Choose a reason for hiding this comment

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

Mh… might be true that an explicit mentioning of a package in USEPKG might be cleaner, but it is very inconvenient 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VERY!! inconvenient!!! ;)

Seriously, this opens the way to "magic package names" where you'd just mention "USEPKG += foo" and some Makefile automagically gets the package metadata from "github.com/riot-packages/foo.git", ...

@kaspar030
Copy link
Contributor Author

thx for testing! 😄

@miri64 miri64 mentioned this pull request Oct 19, 2016
Closed
@smlng smlng mentioned this pull request Oct 19, 2016
Closed
@miri64 miri64 mentioned this pull request Oct 19, 2016
Closed
smlng added a commit to smlng/RIOT that referenced this pull request Oct 21, 2016
@smlng
boards/remote: add Makefile.dep
95ab1ae 
add missing Makefile.dep files to boards remote-pa, remote-reva, and
remote-revb, as introduced by RIOT-OS#5891. Otherwise certain features are
not working, such as radio.
@smlng smlng mentioned this pull request Oct 21, 2016
Merged
@aabadie aabadie mentioned this pull request Oct 25, 2016
Merged
@miri64 miri64 mentioned this pull request Oct 30, 2016
Merged
@miri64 miri64 mentioned this pull request Nov 3, 2016
Merged
neiljay pushed a commit to neiljay/RIOT that referenced this pull request Nov 4, 2016
@smlng
boards/remote: add Makefile.dep
4d8828d 
add missing Makefile.dep files to boards remote-pa, remote-reva, and
remote-revb, as introduced by RIOT-OS#5891. Otherwise certain features are
not working, such as radio.
samkumar pushed a commit to samkumar/RIOT-OS that referenced this pull request Nov 7, 2016
@smlng @samkumar
boards/remote: add Makefile.dep
03ec357 
add missing Makefile.dep files to boards remote-pa, remote-reva, and
remote-revb, as introduced by RIOT-OS#5891. Otherwise certain features are
not working, such as radio.
@miri64 miri64 added this to the Release 2016.10 milestone Nov 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

Assignees

@miri64 miri64

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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2016.10
Development

Successfully merging this pull request may close these issues.
3 participants
@kaspar030 @jnohlgard @miri64