Skip to content

make.dep: fix sock_udp deps for stnp#12931

Merged
miri64 merged 2 commits intoRIOT-OS:masterfrom
haukepetersen:fix_deps_sockudpusers
Feb 12, 2020
Merged

make.dep: fix sock_udp deps for stnp#12931
miri64 merged 2 commits intoRIOT-OS:masterfrom
haukepetersen:fix_deps_sockudpusers

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Dec 12, 2019 •
edited
Loading

Contribution description

IMHO, the dependecies for sntp and gcoap are a little off currently: both directly depend on gnrc_sock_udp, but both are not tied to GNRC (anymore). So the correct dependency should be to sock_udp, right?

This PR changes this dependency slightly by doing two things:

  • if GNRC is used, include gnrc_sock_udp in the case sock_udp is selected (-> sub dependency in the gnrc block in Makefile.dep)
  • make sntp and gcoap simply depend on sock_udp instead of gnrc_sock_udp

Context is once more, that I played around with using gcoap on top of a non-GNRC network stack... So does this work for everyone or can you think of a better solution?

Testing procedure

Build sntp and gcoap examples. The output of make info-modules should be unchanged.

Issues/PRs references

none

@haukepetersen
make.dep: add dep for gnrc_sock_udp in GNRC deps
17ecba0 
If some any module wants to use sock_udp and GNRC is configured as
network stack, this PR takes care of pulling in gnrc_sock_udp then.
@haukepetersen
make.dep/sntp: remove gnrc dependency
b06c01a 
This module should depend on sock_udp instead of gnrc_sock_udp.
@haukepetersen haukepetersen added Area: network Area: Networking Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Dec 12, 2019
@miri64
Copy link
Copy Markdown
Member

miri64 commented Dec 12, 2019

Well at the moment gcoap is hardly dependent on gnrc_sock_udp (see #8130)

* if GNRC is used, include `gnrc_sock_udp` in the case `sock_udp` is selected (-> sub dependency in the `gnrc` block in `Makefile.dep`)

Why single out GNRC?

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Dec 12, 2019

Well at the moment gcoap is hardly dependent on gnrc_sock_udp

Yep, I noticed that by now :-). On first sight it looked like it compiled fine in my environment, but there were some copmile issues that were augmented by local stupidity...

Still, I think for sntp this PR should hold?!

@miri64
Copy link
Copy Markdown
Member

miri64 commented Dec 12, 2019

Still, I think for sntp this PR should hold?!

Yapp

@kb2ma
Copy link
Copy Markdown
Member

kb2ma commented Dec 12, 2019

And with @miri64's infrastructure work, hopefully gcoap can also be liberated soon. ;-)

@kb2ma
Copy link
Copy Markdown
Member

kb2ma commented Dec 12, 2019

If only there were more RIOT hours in a day...

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Dec 13, 2019

Simply removed the commit effecting gcoap. So the changes should be valid now, right?

miri64
miri64 approved these changes Dec 13, 2019
View reviewed changes
Copy link
Copy Markdown
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.

ACK. Would be nice to also have this kind of dependencies for the other sock-types / stack types

@miri64 miri64 changed the title make.dep: fix sock_udp deps for stnp and gcoap make.dep: fix sock_udp deps for stnp Dec 13, 2019
@haukepetersen haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Dec 16, 2019
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Dec 16, 2019

some builds for the esp32 failed, could someone with some knowledge about this platform take a look?

@haukepetersen haukepetersen removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Dec 16, 2019
@miri64 miri64 mentioned this pull request Dec 16, 2019
Closed
@miri64
Copy link
Copy Markdown
Member

miri64 commented Dec 16, 2019

some builds for the esp32 failed, could someone with some knowledge about this platform take a look?

See #12964.

@gschorcht gschorcht mentioned this pull request Dec 17, 2019
Merged
3 tasks
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 11, 2020
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Feb 11, 2020

I guess this can be merged now?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Feb 12, 2020

Yepp

@miri64 miri64 merged commit ff0d228 into RIOT-OS:master Feb 12, 2020
@kb2ma kb2ma mentioned this pull request Feb 13, 2020
Closed
@leandrolanzieri leandrolanzieri added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Feb 20, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020
@haukepetersen haukepetersen deleted the fix_deps_sockudpusers branch March 2, 2020 10:17
@kb2ma kb2ma mentioned this pull request Mar 16, 2020
Closed
@miri64 miri64 mentioned this pull request Aug 31, 2020
Merged
miri64 added a commit to miri64/RIOT that referenced this pull request Aug 31, 2020
@miri64
sock: make sock implementation dependency of sock API + stack
8386ec9 
This is the logical continuation of [RIOT-OS#12931] for _all_ `sock`
implementations.

[RIOT-OS#12931]: RIOT-OS#12931
@miri64 miri64 mentioned this pull request Aug 31, 2020
Merged
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

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2020.04

Development

Successfully merging this pull request may close these issues.

5 participants

@haukepetersen @miri64 @kb2ma @benpicco @leandrolanzieri