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

sys/shell/commands/ping: fix dependency [backport 2022.07] #18354

Merged
merged 1 commit into from
Aug 1, 2022
Merged

sys/shell/commands/ping: fix dependency [backport 2022.07] #18354

merged 1 commit into from
Aug 1, 2022

Conversation

maribu
Copy link
Member

@maribu maribu commented Jul 21, 2022

Backport of #18349

Contribution description

Add missing dependency to xtimer so that the shell command ping is
again provided when requested.

Testing procedure

Run make BOARD=nucleo-f767zi -C examples/gnrc_networking flash term and then help. If the ping command is listed again, the bug is fixed. (Did so, worked for me.)

Issues/PRs references

None

@maribu maribu requested a review from miri64 as a code owner July 21, 2022 15:59
@maribu maribu added Area: examples Area: Example Applications Area: sys Area: System Area: tests Area: tests and testing framework 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 Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jul 21, 2022
@maribu maribu mentioned this pull request Jul 21, 2022
Merged
@maribu maribu force-pushed the backport/2022.07/sys/shell/commands/sc_gnrc_icmpv6_echo branch from da3301b to dcf3f37 Compare July 21, 2022 15:59
miri64
miri64 reviewed Jul 21, 2022
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.

Mh... this somehow only contains the changes to the applications' Makefile.ci. Sorry missed the Makefile.dep -.-

@miri64
Copy link
Member

miri64 commented Jul 21, 2022

Ah this left out the ztimer conversion... So this needs dedicated testing for which I don't own the hardware :(

@maribu maribu changed the title sys/shell/commands/ping: fix dependency & convert to ztimer [backport 2022.07] sys/shell/commands/ping: fix dependency [backport 2022.07] Jul 21, 2022
@maribu
Copy link
Member Author

maribu commented Jul 21, 2022

Just checked, ping and ping6 still listed on the nucleo-f767zi when running help.

@miri64
Copy link
Member

miri64 commented Jul 21, 2022

Still, I leave the final decision to @chrysn

chrysn
chrysn approved these changes Aug 1, 2022
View reviewed changes
Copy link
Member

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

I don't have the hardware to test it, but this test:

$ strings examples/gnrc_networking/bin/nucleo-f767zi/gnrc_networking.elf G 'Ping via ICMPv6'

before and after this PR (before returning empty, after giving one occurrence) gives me enough confidence that this does the right thing.

@chrysn chrysn enabled auto-merge August 1, 2022 15:07
@miri64
Copy link
Member

miri64 commented Aug 1, 2022

@maribu needs rebase before this can be merged.

@chrysn chrysn disabled auto-merge August 1, 2022 15:47
@chrysn
Copy link
Member

chrysn commented Aug 1, 2022

GitHub has an "Update with rebase" option, wouldn't that do just the right thing?

@miri64
Copy link
Member

miri64 commented Aug 1, 2022

GitHub has an "Update with rebase" option, wouldn't that do just the right thing?

Ah, they updated it, so it is possible with a rebase. Yes in that case, it does!

@maribu @miri64
sys/shell/commands/sc_gnrc_icmpv6_echo: fix dependency
29e35ec 
Add missing dependency to xtimer so that the shell command `ping` is
again provided when requested.

(cherry picked from commit 3a5e363)
@miri64 miri64 force-pushed the backport/2022.07/sys/shell/commands/sc_gnrc_icmpv6_echo branch from dcf3f37 to 29e35ec Compare August 1, 2022 15:49
@miri64 miri64 enabled auto-merge August 1, 2022 15:49
@miri64
Copy link
Member

miri64 commented Aug 1, 2022

Should we fast-track this PR in the CI, so we can RC2 soonish?

@chrysn
Copy link
Member

chrysn commented Aug 1, 2022 •
edited
Loading

You can do that? In that case yes please, this is the last one I'd like to have in before pushing the button again.

([edit] I mean, without manually unticking "ready for CI" for a bunch of PRs and then re-enabling it in the right sequence)

@miri64
Copy link
Member

miri64 commented Aug 1, 2022

You can do that? In that case yes please, this is the last one I'd like to have in before pushing the button again.

Well yes, by toggling the label in the PRs that come before 😀

This was referenced Aug 1, 2022
Merged
Merged
Merged
@miri64
Copy link
Member

miri64 commented Aug 1, 2022

([edit] I mean, without manually unticking "ready for CI" for a bunch of PRs and then re-enabling it in the right sequence)

That's exactly, what I just did.

@miri64 miri64 merged commit 93a08b9 into RIOT-OS:2022.07-branch Aug 1, 2022
@maribu maribu deleted the backport/2022.07/sys/shell/commands/sc_gnrc_icmpv6_echo branch August 9, 2022 13:34
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 left review comments

@chrysn chrysn chrysn approved these changes

Assignees
No one assigned
Labels
Area: examples Area: Example Applications Area: sys Area: System Area: tests Area: tests and testing framework 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 Process: release backport Integration Process: The PR is a release backport of a change previously provided to master Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2022.07
Development

Successfully merging this pull request may close these issues.
3 participants
@maribu @miri64 @chrysn