Skip to content

pseudomodules: allow defining them in pkg/PKG/Makefile.include#9003

Merged
cladmi merged 1 commit intoRIOT-OS:masterfrom
cladmi:pr/proposal/pkg/pseudomodules
Jul 5, 2018
Merged

pseudomodules: allow defining them in pkg/PKG/Makefile.include#9003
cladmi merged 1 commit intoRIOT-OS:masterfrom
cladmi:pr/proposal/pkg/pseudomodules

Conversation

@cladmi
Copy link
Copy Markdown
Contributor

@cladmi cladmi commented Apr 23, 2018 •
edited
Loading

Contribution description

Officially allow packages to define pseudomodules in their Makefile.include as
done in the libcose package.

RIOT/pkg/libcose/Makefile.include

Lines 11 to 12 in 6c39d2d

# Declare pseudomodules here to be selfcontained
PSEUDOMODULES += libcose_crypt_%

Issues/PRs references

Proposed implementation of this RFC #8984

@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 mentioned this pull request Apr 23, 2018
Closed
@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Apr 23, 2018

I think Makefile.dep is a better place, despite it's misleading name.

@cladmi
Copy link
Copy Markdown
Contributor Author

cladmi commented Apr 24, 2018

I think Makefile.dep is a better place, despite it's misleading name.

Can you elaborate on this ? for me it is more .include that describes a module/package where .dep handles only setting dependency.

One of the "problem" with Makefile.dep is that it needs to be parsed several times during USEMODULE resolution. I imagine it is even the reason to have these dependencies in a separate file from Makefile.include.

@kaspar030
Copy link
Copy Markdown
Contributor

kaspar030 commented Apr 25, 2018

Can you elaborate on this ? for me it is more .include that describes a module/package where .dep handles only setting dependency.

Sorry, yes. Makefile.dep is considered for info-boards-supported, but that actually doesn't matter for pseudomodules. Thus I think I changed my mind and now proclaim the opposite.

@cladmi cladmi mentioned this pull request Jun 7, 2018
Closed
4 tasks
@cladmi cladmi mentioned this pull request Jun 28, 2018
Merged
@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 Jul 4, 2018
@cladmi cladmi force-pushed the pr/proposal/pkg/pseudomodules branch from 788f323 to 459713d Compare July 4, 2018 06:54
@cladmi
Copy link
Copy Markdown
Contributor Author

cladmi commented Jul 4, 2018

The 'PSEUDOMODULES' is still there in libcose, so I think it now should be officially documented that it can be done.

@cladmi cladmi added this to the Release 2018.07 milestone Jul 4, 2018
@cladmi cladmi requested a review from smlng July 4, 2018 07:31
bergzand
bergzand reviewed Jul 5, 2018
View reviewed changes
makefiles/pseudomodules.inc.mk Outdated
PSEUDOMODULES += skald_ibeacon
PSEUDOMODULES += skald_eddystone


Copy link
Copy Markdown
Member

@bergzand bergzand Jul 5, 2018

Choose a reason for hiding this comment

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

Any reason for the duplicate newline here?

Copy link
Copy Markdown
Contributor Author

@cladmi cladmi Jul 5, 2018

Choose a reason for hiding this comment

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

I did it on purpose to separate the block where pseudomodules are defined and with the second block with the documentation.
I often only keep one newline between related things and use two to distinguish between blocks.

It is only personal style leaking here and I can change it.

Copy link
Copy Markdown
Member

@bergzand bergzand Jul 5, 2018

Choose a reason for hiding this comment

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

I think I slightly prefer a single line, but I don't mind having two newlines here if you think it makes things more readable.

Copy link
Copy Markdown
Contributor Author

@cladmi cladmi Jul 5, 2018

Choose a reason for hiding this comment

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

I update it then.

bergzand
bergzand approved these changes Jul 5, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

ACK

@cladmi
pseudomodules: allow defining them in pkg/PKG/Makefile.include
a62c8a8 
Officially allow packages to define pseudomodules in their Makefile.include as
done in the libcose package.
@cladmi cladmi force-pushed the pr/proposal/pkg/pseudomodules branch from 459713d to a62c8a8 Compare July 5, 2018 14:03
@cladmi
Copy link
Copy Markdown
Contributor Author

cladmi commented Jul 5, 2018

I rebased and changed the whitespace inline.

@cladmi
Copy link
Copy Markdown
Contributor Author

cladmi commented Jul 5, 2018

Murdock happy.

@cladmi cladmi merged commit 8bf468e into RIOT-OS:master Jul 5, 2018
@cladmi cladmi deleted the pr/proposal/pkg/pseudomodules branch July 5, 2018 14:30
@cladmi cladmi mentioned this pull request Aug 21, 2018
Closed
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergzand bergzand bergzand approved these changes

@smlng smlng Awaiting requested review from smlng

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 Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR

Projects

None yet

Milestone

Release 2018.07

Development

Successfully merging this pull request may close these issues.

3 participants

@cladmi @kaspar030 @bergzand