runners: restore obsolete synonyms for -i/--dev-id #53951
Conversation
This is a partial revert of commit 2cee5ff ("scripts: west_commands: runners: remove deprecated options"). I remarked at the time that the removal of the older way of writing things from the runners themselves seemed gratuitous since they are easy to continue to support indefinitely and people may have been used to the old way of doing things. It didn't seem worth the fight to push for a revert at the time, though. Since then I've run into real problems that their removal has caused in the wild and I am convinced that this part of that patch was wrong. Restore the original, undeprecated forms of these options, but make it clear in the command line help that they're just obsolete alternative spellings at this point. Signed-off-by: Marti Bolivar <[email protected]>
mbolivar-nordic
requested review from
gmarull,
trond-snekvik,
nordicjm and
carlescufi
There was a problem hiding this comment.
snr option was deprecated long time ago, what is the point of adding obsolete features?
Edit: I think that if bringing the option one release more helps, it makes sense reverting, but not keep forever. I think we've been fairly consistent with deprecations, and I see no reason for an exception here. There are many other trivial deprecations that have been removed in time. Also, let's be transparent on this case: deprecation support was implemented relying on deprecated options being present. So, the problem would have popped up anyway, unless kept forever. However, since problem has been already fixed, and no SDK released with this problem, I see no value on bringing this back.
|
My view on deprecation: this doesn't fall into the zephyr defined deprecation system really, that's written about and for code, e.g. let's use tinycbor - it has been replaced with a new module, the in-tree code that used it has been replaced with the new module, so why keep a whole module still around that is unmaintained? Hence why it was deprecated and removed. However in the case of a command line argument for a flash runner, it's 1 line in a python file, it's not something that has to ever be maintained since it's already working and if something needs to be updated, it would be for the underlying common code itself used by both old and new commands, and it's not a piece of code that people pull in to use in their applications, it's supporting code in a zephyr script. So for me (at least), keeping an old and new command in a script is not anywhere near the level of removing deprecated code that can be used in an application that has the potential to cause big problems if people carry on using it where it has issues or security vulnerabilities - because those would be going into built applications. |
|
Note: opened this as an alternative nrfconnect/sdk-zephyr#1032 |
|
Second note: fix has been deployed https://nrfconnect.github.io/vscode-nrf-connect/release_notes/connect/2022.11.140.html#fixes-in-2023144, so I've closed nrfconnect/sdk-zephyr#1032. |
|
@gmarull we are not going to come to an agreement about this and I am using my right as maintainer of the runners to set the direction of the area. I am choosing to keep old options around for compatibility as long as they are not a big maintenance burden, which this clearly is not. Please remove your -1. I don't need you to approve, but don't stand in the way. |
I won't block as soon as patch is reverted properly, ie, mark
I'd like to remember that maintainers do not have the final word in Zephyr. I've personally been blocked in stuff I maintain (had to escalate it), or blocked by a minority. I won't contribute to this nonsense though. |
It's not marked deprecated on purpose; the point is to make it clear that it's obsolete, but not deprecated.
You can see in the diff itself that the patch is not just about what happened to our SDK. Just because we found out about this issue from our SDK downstream users doesn't mean other people are not affected by this. I am restoring the old functionality so that the people we didn't hear from won't be affected either. |
So then my -1 stands, please escalate if you think I'm wrong. Adding obsolete options without any deprecation notice is a bad practice IMO. More considering this option was already deprecated. Imagine people copy a command from an outdated guide. They'll never notice about that option being obsolete until one day it is removed and breaks their workflows. With a deprecation message they'll at least notice.
I'm pretty sure impact has been low/zero. And the SDK issue was in the end not our fault, they did their job on using |
I really wish you would get out of the way |
There was a problem hiding this comment.
I see no issues in allowing a synonym for --dev-id if it makes life easier for lot of users.
One comment though, as I didn't read all existing code.
What happens if a user specifies both --dev-id and --snr on same invocation ?
Clearly a user shouldn't do so, but I'm curious how code will behave, as I don't see the two arguments being mutual exclusive when just looking at this PR.
gmarull
dismissed
their stale review
Giving up, can't be part of reviews where PR author asks me "to get out of the way". For other reviewers, this is what I proposed nrfconnect/sdk-zephyr#1032 (ie, restore what we had before)
Note that |
|
On Tue, Jan 31 2023, Torsten Tejlmand Rasmussen wrote:
@tejlmand approved this pull request.
I see no issues in allowing a synonym for `--dev-id` if it makes life easier for lot of users.
One comment though, as I didn't read all existing code.
What happens if a user specifies both `--dev-id` and `--snr` on same invocation ?
Clearly a user shouldn't do so, but I'm curious how code will behave,
as I don't see the two arguments being mutual exclusive when just
looking at this PR.
Last one specified on the command line wins. This is consistent with
many other other UNIX programs' behavior.
|
There was a problem hiding this comment.
I think it is sensible to keep supporting the old option as long as the maintenance burden of doing so is minimal.
A deprecation/obsolescence warning would still help users move on to using the new option in case we ever decide to drop the support for it at some point in the future (which, otherwise, we will likely have to do yet another "deprecation" with a warning before finally dropping it for good).
|
A deprecation/obsolescence warning would still help users move on to
using the new option in case we ever decide to drop the support for it
at some point in the future
The policy I have in mind right now as the runners package maintainer is
that, in the future, whenever we consolidate multiple runner-specific
options with a common runner option (so e.g. '--snr' in nrfjprog.py
became generalized as '--dev-id'), that we keep the old runner-specific
options for backwards compatibility in all cases.
It's really zero maintenance cost to keep a single line of code in the
argument parsing code that says the old option is just an alias for the
new one, and it lets people's existing scripts and muscle memory keep
working. I see no downsides.
|
This is a partial revert of commit 2cee5ff
("scripts: west_commands: runners: remove deprecated options").
I remarked at the time that the removal of the older way of writing things from the runners themselves seemed gratuitous since they are easy to continue to support indefinitely and people may have been used to the old way of doing things. It didn't seem worth the fight to push for a revert at the time, though.
Since then I've run into real problems that their removal has caused in the wild and I am convinced that this part of that patch was wrong.
Restore the original, undeprecated forms of these options, but make it clear in the command line help that they're just obsolete alternative spellings at this point.
Fixes: #54384