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

Makefile.include: fully define BASELIBS before using its value #9451

Merged
merged 3 commits into from
Jul 5, 2018
Merged

Makefile.include: fully define BASELIBS before using its value #9451

merged 3 commits into from
Jul 5, 2018

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Jun 28, 2018 •
edited
Loading

Contribution description

As described in #8913 some places are using the immediate value of BASELIBS before it is actually set (== use of non initialized variables…). The problem was mitigated because ELFFILE is always rebuild even if files in $(BASELIBS) did not change.

This PR addresses this by moving the definition of BASELIBS sooner in Makefile.include.

This required:

I also added a debug commit that asserts that BASELIBS was not changed during build. It can be removed before merging.

Issues/PRs references

Issue #8913

This was problematic when trying to reference $(BASELIBS) as a target dependency in the application Makefile: #9351

@cladmi cladmi added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) 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 labels Jun 28, 2018
This was referenced Jun 28, 2018
Closed
Merged
@cladmi cladmi force-pushed the pr/make/makefile_include/define_baselibs branch 2 times, most recently from a814ac8 to 0dc40ef Compare July 2, 2018 10:15
@cladmi
Copy link
Contributor Author

cladmi commented Jul 2, 2018 •
edited
Loading

I moved the 'test' commit earlier in the history to help demonstrate the fixes.

You can execute these tests on each commit and see the fixes applied:

make -C examples/hello-world
make -C tests/unittests
make -C examples/bindist

Each commit currently fixes one problem so it is easy to verify.

@cladmi cladmi added this to the Release 2018.07 milestone Jul 2, 2018
@cladmi cladmi requested a review from kYc0o July 2, 2018 10:17
smlng
smlng approved these changes Jul 4, 2018
View reviewed changes
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

tested ACK, first commit fails as expected with BASELIB change detected, following commit fix that as promised 😉

@cladmi
Copy link
Contributor Author

cladmi commented Jul 4, 2018

Is there a reason to keep the BASELIBS change detection or should I remove as I planned to ?

@cladmi
Makefile.include: process 'modules.inc.mk' before using BASELIBS
d8a2643 
Build targets were using the immediate value of '$(BASELIBS)' before it was
actually set. bindist.inc.mk should also be processed before as it is adding
`BIN_USEMODULE` to `USEMODULE`.

This fixes use before define problems for:

 * `make -C examples/hello-world`
 * `make -C examples/bindist`
@cladmi
Makefile.include: process pkg/*/Makefile.include before using BASELIBS
fb27a80 
They can define PSEUDOMODULES which is used to populate BASELIBS.
It means they must be processed before using BASELIBS immediate value.

This moved the processing of all `Makefile.include` before using `BASELIBS`.
It is also moved before setting `BASELIBS` in `modules.inc.mk to keep the
order logical (it would work anyway thanks to deferred variables evaluation).

This fixes the problems for `make -C tests/unittests`
@cladmi
Makefile.include: detect if BASELIBS changed after its first use
90fdd09
@smlng
Copy link
Member

smlng commented Jul 4, 2018

Is there a reason to keep the BASELIBS change detection or should I remove as I planned to ?

mhm, maybe its good to keep this (and maybe add other) checks in the build system - but could be moved into a distinct file like makefiles/checks.inc.mk and maybe add a switch to dis/enable these checks? I mean it would be nice to verify that other changes to the make/build stuff isn't introducing any problems related to BASELIBS and such.

@cladmi cladmi force-pushed the pr/make/makefile_include/define_baselibs branch from 0dc40ef to 90fdd09 Compare July 4, 2018 08:24
@cladmi
Copy link
Contributor Author

cladmi commented Jul 4, 2018

I moved the commit as last so there is no "broken" intermediate state.

Refactoring with a separate check file could be done in a separate PR and would need specific thoughts as this one needs to be init at the right place.

@cladmi cladmi merged commit 534b0da into RIOT-OS:master Jul 5, 2018
@cladmi cladmi deleted the pr/make/makefile_include/define_baselibs branch July 5, 2018 11:38
@cladmi cladmi modified the milestone: Release 2018.07 Jul 10, 2018
cladmi added a commit to cladmi/RIOT that referenced this pull request Jul 10, 2018
@cladmi
Makefile.include: remove debug '$(info)'
82c8d69 
Remove debug '$(info)' added by RIOT-OS#9451
@cladmi cladmi mentioned this pull request Jul 10, 2018
Merged
pekkanikander pushed a commit to AaltoNEPPI/RIOT that referenced this pull request Aug 28, 2018
@cladmi @pekkanikander
Makefile.include: remove debug '$(info)'
697bcbe 
Remove debug '$(info)' added by RIOT-OS#9451
danpetry pushed a commit to danpetry/RIOT that referenced this pull request Sep 5, 2018
@cladmi
Makefile.include: remove debug '$(info)'
92f094f 
Remove debug '$(info)' added by RIOT-OS#9451
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smlng smlng smlng approved these changes

@jnohlgard jnohlgard Awaiting requested review from jnohlgard

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@kYc0o kYc0o Awaiting requested review from kYc0o

Assignees
No one assigned
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: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2018.07
Development

Successfully merging this pull request may close these issues.
2 participants
@cladmi @smlng