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.

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

treewide: remove deprecated shell_commands module #20485

Merged

Conversation

dylad
Copy link
Member

@dylad dylad commented Mar 19, 2024

Contribution description

This PR removes a deprecated module that should have been removed a few release ago.
I've replaced it by shell_cmds_default where needed.

Testing procedure

CI should be enough to catch up any miss.

Issues/PRs references

None.

dylad added 2 commits March 19, 2024 15:10
Replaces it by shell_cmds_default where needed

Signed-off-by: Dylan Laduranty <dylan.laduranty@mesotic.com>
Signed-off-by: Dylan Laduranty <dylan.laduranty@mesotic.com>
@dylad dylad added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 19, 2024
@github-actions github-actions bot added Area: network Area: Networking Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: build system Area: Build system Area: pkg Area: External package ports Area: BLE Area: Bluetooth Low Energy support Area: sys Area: System Area: examples Area: Example Applications labels Mar 19, 2024
@dylad dylad changed the title treewide: remove deprecated shell commands treewide: remove deprecated shell_commands module Mar 19, 2024
@dylad dylad added the Process: removal Integration Process: The PR is removing a deprecated feature or API label Mar 19, 2024
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

Comment on lines -262 to -266
ifneq (,$(filter shell_commands,$(USEMODULE)))
# shell_commands has been renamed to shell_cmds_default, but let's keep this
# for backward compatibility
USEMODULE += shell_cmds_default
endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really necessary?
This will break all external apps, keeping those few lines around wouldn't hurt and avoid the churn on users.

Copy link
Member Author

Choose a reason for hiding this comment

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

This module is deprecated for more than a year already. We were supposed to remove it after 2023.07 release according to makefiles/pseudomodules.inc.mk
I guess we give enough time to our users to migrate already.

This will break all external apps, keeping those few lines around wouldn't hurt and avoid the churn on users.

The question is do we want to keep backward compatibility forever ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm opting for removal. If your old app really needs it, you can still use old releases of RIOT.
(I also believe it is a good thing, to annoy your users to migrate to new APIs regularly, as long as it is done in a sensible manner. A bit like Apple does it. But that's clearly an personal opinion)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also in favor of removal, in the end that's what our yet-to-be-documented deprecation process is for.

@riot-ci
Copy link

riot-ci commented Mar 19, 2024

Murdock results

✔️ PASSED

f78a41f treewide: replace shell_commands module in documentation

Success Failures Total Runtime
10006 0 10008 10m:32s

Artifacts

@Teufelchen1
Copy link
Contributor

@benpicco @dylad Can we get this in today but no later than tomorrow?

@dylad
Copy link
Member Author

dylad commented Mar 25, 2024

I'm still in favor of removing long enough deprecated stuff.
@benpicco What are your thoughts ?

@maribu maribu added this pull request to the merge queue Mar 26, 2024
Merged via the queue into RIOT-OS:master with commit 1b64abf Mar 26, 2024
32 checks passed
@dylad
Copy link
Member Author

dylad commented Mar 26, 2024

Thanks !

@dylad dylad deleted the pr/treewide/remove_deprecated_shell_commands branch March 26, 2024 10:50
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BLE Area: Bluetooth Low Energy support Area: build system Area: Build system Area: doc Area: Documentation Area: examples Area: Example Applications Area: network Area: Networking Area: pkg Area: External package ports 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 Process: removal Integration Process: The PR is removing a deprecated feature or API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants