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

gnrc_netif: replace GNRC_NETIF_SINGLE with gnrc_netif_single pseudo-module #14632

Merged
merged 1 commit into from
Jul 28, 2020
Merged

gnrc_netif: replace GNRC_NETIF_SINGLE with gnrc_netif_single pseudo-module #14632

merged 1 commit into from
Jul 28, 2020

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jul 28, 2020 •
edited
Loading

Contribution description

To being able to use this information during dependency resolution, make gnrc_netif_single a pseudo-module.

Testing procedure

File size of the affected examples should not change.

Issues/PRs references

#13746 (comment)

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Jul 28, 2020
miri64
miri64 requested changes Jul 28, 2020
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

NACK. This moves a GNRC-specific optimization to netif without providing any support for that in other network stacks and also is based on a misunderstanding in #13746 (comment)

@miri64
Copy link
Member

miri64 commented Jul 28, 2020

Alternatively you could move this to a pseudo-module to gnrc_netif_single, but that won't help you with the issue I pointed out in #13746.

@miri64
Copy link
Member

miri64 commented Jul 28, 2020

[…] but that won't help you with the issue I pointed out in #13746.

Ok, it could, since it makes it more obvious to the build system... ^^

@benpicco benpicco changed the title gnrc_netif: replace GNRC_NETIF_SINGLE with netif_single pseudo-module gnrc_netif: replace GNRC_NETIF_SINGLE with gnrc_netif_single pseudo-module Jul 28, 2020
@benpicco
Copy link
Contributor Author

Ok, I re-wrote the PR to use gnrc_netif_single instead.

@leandrolanzieri leandrolanzieri requested a review from jia200x July 28, 2020 11:15
miri64
miri64 previously requested changes Jul 28, 2020
View reviewed changes
tests/emcute/Makefile Show resolved Hide resolved
tests/gnrc_sock_dns/Makefile Show resolved Hide resolved
tests/gnrc_tcp/Makefile Show resolved Hide resolved
examples/emcute_mqttsn/Makefile Outdated Show resolved Hide resolved
examples/gnrc_minimal/Makefile Outdated Show resolved Hide resolved
@miri64
Copy link
Member

miri64 commented Jul 28, 2020

I'm fine with the current state, however the makefiles should not desolve into chaos ;-)

@miri64 miri64 added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jul 28, 2020
@miri64 miri64 dismissed their stale review July 28, 2020 11:38

I'm fine with the current state, but I still need to test this.

@miri64
Copy link
Member

miri64 commented Jul 28, 2020 •
edited
Loading

Tested so far

  • examples/emcute_mqttsn
  • examples/gnrc_minimal
  • tests/emcute
  • tests/gnrc_sock_dns
  • tests/gnrc_tcp

@miri64
Copy link
Member

miri64 commented Jul 28, 2020

tests/emcute fails, but in the same way it fails in master. IIRC it worked for the release tests, so need to investigate that 👀

@miri64 miri64 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jul 28, 2020
miri64
miri64 approved these changes Jul 28, 2020
View reviewed changes
Copy link
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. Please squash

@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 Jul 28, 2020
@miri64
Copy link
Member

miri64 commented Jul 28, 2020

tests/emcute fails, but in the same way it fails in master. IIRC it worked for the release tests, so need to investigate that 👀

Something in a274ea4 (#12428) broke that test according to my bisect :-/

@miri64
Copy link
Member

miri64 commented Jul 28, 2020 •
edited
Loading

Something in a274ea4 (#12428) broke that test according to my bisect :-/

as the failure happens on the large payload topic name test, I suspect something going wrong there.

@miri64
Copy link
Member

miri64 commented Jul 28, 2020

as the failure happens on the large payload topic name test, I suspect something going wrong there.

No, if I remove the previous two test cases the test case succeeds, but then the last one fails, so its always on the third test case :-/

@miri64
Copy link
Member

miri64 commented Jul 28, 2020

See #14636

@miri64 miri64 merged commit 0e3aa2f into RIOT-OS:master Jul 28, 2020
@benpicco benpicco deleted the netif_single branch July 28, 2020 16:00
@miri64 miri64 mentioned this pull request Jul 28, 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

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@smlng smlng Awaiting requested review from smlng

@jia200x jia200x Awaiting requested review from jia200x

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@benpicco @miri64