Skip to content

make: Add 'BUILDDEPS' variable#9002

Merged
cladmi merged 4 commits intoRIOT-OS:masterfrom
cladmi:pr/makefiles/builddeps
Jul 10, 2018
Merged

make: Add 'BUILDDEPS' variable#9002
cladmi merged 4 commits intoRIOT-OS:masterfrom
cladmi:pr/makefiles/builddeps

Conversation

@cladmi
Copy link
Contributor

@cladmi cladmi commented Apr 23, 2018

Extend/fix APPDEPS as a new variable to declare general build dependencies.

This PR proposes to add a new BUILDDEPS variable instead of fixing APPDEPS.
It is only a proposal and is here to show how it could be used.
Fixing APPDEPS and making it evolve is also a possibility, with other sub-prs, I just want to start talking about it and justify the pull requests that would change APPDEPS.

Contribution description

BUILDDEPS are files / make targets that should be build before compiling.
It can include packages source download, generating headers, modules.

It is the equivalent of APPDEPS but not limited to the application (as documented).
It cannot be done right now with APPDEPS as it is used in BASELIBS and fixing it requires changing mips using it for source files.

Other solution

Another solution is to fix APPDEPS now:

Issues/PRs references

This would be necessary to do #7654 properly.
With #8987 it would allow defining external packages by setting a dependency that downloads the source files.

@cladmi cladmi added 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 labels Apr 23, 2018
@cladmi cladmi requested review from jnohlgard and kaspar030 April 23, 2018 13:14
@cladmi
Copy link
Contributor Author

cladmi commented Apr 23, 2018

@neiljay I may also need some of your input. Do you remember why you put the assembly files as APPDEPS and not just using the module ?

export APPDEPS += $(RIOTCPU)/$(CPU)/$(CPU_MODEL)/$(CPU_MODEL).S

@cladmi
Copy link
Contributor Author

cladmi commented Apr 24, 2018

I found another line that should be using APPDEPS:

$(RIOTBUILD_CONFIG_HEADER_C): $(JS_H)

@cladmi cladmi force-pushed the pr/makefiles/builddeps branch from 81560ae to 8004035 Compare July 4, 2018 07:27
@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 Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Jul 4, 2018
@cladmi
Copy link
Contributor Author

cladmi commented Jul 4, 2018

Rebased and inline fixed a whitespace error.

@cladmi cladmi force-pushed the pr/makefiles/builddeps branch from 8004035 to d1bd7d5 Compare July 6, 2018 14:26
Makefile.include Outdated

DIRS += $(EXTERNAL_MODULE_DIRS)

# Define dependencies required for building (headers, dowloading source files,)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: downloading

export USEPKG # Pkg dependencies (third party modules) of the application. Set in the application's Makefile.
export DISABLE_MODULE # Used in the application's Makefile to suppress DEFAULT_MODULEs.
export APPDEPS # Files / Makefile targets that need to be created before the application can be build. Set in the application's Makefile.
# BUILDDEPS # Files / Makefile targets that need to be created before starting to build. Set by modules / build system included files.
Copy link
Contributor

Choose a reason for hiding this comment

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

took me a while to parse "Set by modules / build system included files.". What does "Set by build system included files." mean? Maybe just drop the whole "Set by" phrase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was writing in the same way as the line before but it does not help, I removed it.

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK. minor issues, feel free to fix.

cladmi added 4 commits July 6, 2018 18:56
BUILDDEPS are files / make targets that should be build before compiling.
It can include packages source download, generating headers, modules.

It is the equivalent of `APPDEPS` but not limited to the application.
It cannot be done right now with `APPDEPS` as it is used in `BASELIBS` and
fixing it requires changing mips using it for source files.
Use BUILDDEPS to define the RIOTBUILD_CONFIG_HEADER_C dependency
Use BUILDDEPS to define the APPDEPS dependency
Use BUILDDEPS to define pkg-prepare dependency
@cladmi cladmi force-pushed the pr/makefiles/builddeps branch from d1bd7d5 to 61f13ba Compare July 6, 2018 16:58
@cladmi
Copy link
Contributor Author

cladmi commented Jul 6, 2018

I inline fixed the download and removing the last part of the line as mentioned.

@cladmi cladmi added this to the Release 2018.10 milestone Jul 10, 2018
@cladmi cladmi merged commit 17545e4 into RIOT-OS:master Jul 10, 2018
@cladmi cladmi deleted the pr/makefiles/builddeps branch July 10, 2018 16:24
@cladmi cladmi mentioned this pull request Aug 21, 2018
18 tasks
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants