Skip to content

Makefile.include: check FEATURES_CONFLICT against FEATURES_USED#8925

Merged
kaspar030 merged 1 commit intoRIOT-OS:masterfrom
cladmi:pr/makefileinclude/features_conflict
Apr 12, 2018
Merged

Makefile.include: check FEATURES_CONFLICT against FEATURES_USED#8925
kaspar030 merged 1 commit intoRIOT-OS:masterfrom
cladmi:pr/makefileinclude/features_conflict

Conversation

@cladmi
Copy link
Copy Markdown
Contributor

@cladmi cladmi commented Apr 11, 2018

The FEATURES_CONFLICT check should be done on used features to also
test for included optional features.

Contribution description

This make the FEATURES_CONFLICT test detect conflict with optional features included.

It can be tested with: FEATURES_OPTIONAL="periph_dac periph_spi" make -C examples/hello-world/

$ FEATURES_OPTIONAL="periph_dac periph_spi" make -C examples/hello-world/ 
BOARD=stm32f4discovery
make: Entering directory '/home/cladmi/git/work/RIOT/examples/hello-world'
The following features may conflict: periph_dac periph_spi
Rationale: On stm32f4discovery boards there are the same pins for the DAC and/or SPI_0.

EXPECT undesired behaviour!
Building application "hello-world" for "stm32f4discovery" with MCU "stm32f4".
...

Issues/PRs references

It will allow using FEATURES_CONFLICT for #8890

@cladmi
Makefile.include: check FEATURES_CONFLICT against FEATURES_USED
45dad9a 
The FEATURES_CONFLICT check should be done on used features to also
test for included optional features.
@cladmi cladmi added 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 Apr 11, 2018
@cladmi cladmi requested review from basilfx and kaspar030 April 11, 2018 15:51
@cladmi cladmi mentioned this pull request Apr 11, 2018
Merged
@cladmi cladmi added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Apr 11, 2018
@basilfx
Copy link
Copy Markdown
Member

basilfx commented Apr 11, 2018

Works as expected and I can repeat the same for the EFM32.

ACK

@basilfx
Copy link
Copy Markdown
Member

basilfx commented Apr 11, 2018

(since this affects the Makefiles, do we need another ACK, @cladmi, @kaspar030?)

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Apr 11, 2018

I wonder if there's some logic we can implement so that having two optional but conflicting features would select one, or that requiring one conflicting with an optional would just not select the optional.

@cladmi
Copy link
Copy Markdown
Contributor Author

cladmi commented Apr 12, 2018

@kaspar030 I think the second case could be quite easily done but it would be a new feature where here it's just fixing the check.

With the way it's done now, to handle the one required and one optional case, I would do it with a state in in the Makefile.dep iterations. In the first iterations, optional features are not taken in to account, then when USEMODULE is stable, keep iterating and integrate the optional features that do not conflict until USEMODULE is stable again.

@cladmi cladmi added this to the Release 2018.04 milestone Apr 12, 2018
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Apr 12, 2018

ok!

@kaspar030 kaspar030 merged commit 8ba55c4 into RIOT-OS:master Apr 12, 2018
@cladmi cladmi deleted the pr/makefileinclude/features_conflict branch April 19, 2018 11:09
maxvankessel pushed a commit to maxvankessel/RIOT that referenced this pull request Apr 20, 2018
@kaspar030 @maxvankessel
Merge pull request RIOT-OS#8925 from cladmi/pr/makefileinclude/featur…
ded8d34 
…es_conflict

Makefile.include: check FEATURES_CONFLICT against FEATURES_USED
@cladmi cladmi mentioned this pull request May 8, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@basilfx basilfx basilfx approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

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: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2018.04

Development

Successfully merging this pull request may close these issues.

3 participants

@cladmi @basilfx @kaspar030