-
Notifications
You must be signed in to change notification settings - Fork 75
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
[DNM] samples: Removal of Zephyr smp_svr sample #60
Conversation
Zaphyr RTOS is keeping its own copy of sample and this sample has not been updated far too long to work with Zephyr. Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>
@carlescufi I think sample in Zephyr is enough. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cc @de-nordic and @nvlsianpu @utzig can you please add @de-nordic as a collaborator to this repo so we can add him as a reviewer? |
I cannot myself, but I opened an issue here: https://issues.apache.org/jira/browse/INFRA-19757 |
Thank! |
@de-nordic So, what is the plan here, only to maintain a sample in the zephyr repo or the zephyr fork of mcumgr ? Can we add Zephyr's sample here. That way we can actually make sure it works with zephyr going down the road and each time we update the zephyr smp_svr app or we make big changes to mcumgr we should test it with zephyr. |
Basically I want to avoid duplicate of the same sample, that gets out of sync. I think that this has been your sample, so If you prefer it to be here I do not have really strong arguments to oppose this. To test this with Zephyr you need to use Zephyr anyway. I need to think it over. I just want to avoid having out of sync versions where someone would have to pick patches by hand and apply them both ways. |
@de-nordic @carlescufi So, the answer I got in the ticket was: "This is not possible currently. Write access to ASF repos requires a filed ICLA. This is part of the ASF's legal provenance. If someone is so important to your review process, why would you not invite them as a committer?". So much for doing a review! We can do the process if you don't mind but it takes a few days and you would do sign an ICLA and so on and so forth...
I am wondering about this as well. Ideally there should be a single copy, but worse case scenario we could at least automate this a bit, I mean something like: "if someone touches this code under |
I think it could be done. Probably people involved in reviewing changes to smp_svr would also others on the need to update. |
I am with @utzig on this one. |
So, the problem with having one single copy is, the upstream and the forked version of |
Lets abandon the idea about removing the sample and lets update it instead: |
zephyr-rtos uses exact SHA for reference mcumgr - doesn't this help? |
I don't understand. This sample is maintained inside the zephyr tree. What is exactly the point of maintaining this copy here @de-nordic? |
As I understand @vrahane , they want to keep their copy, so I am at least updating it for them. |
@carlescufi In general, with all the sub repos, we maintain the samples in the repo, in this case it is important because there is some zephyr specific code in there which needs to be tested each time the library changes for example this time when we were updating it, it broke a few things on the zephyr side which needed testing. Moreover, the changes in mcumgr can vary in the fork and the upstream. I think both should maintain their own version, now if the fork was not there, it would have made sense. We do a similar thing for mcuboot where the zephyr sample gets maintained along with the mynewt sample. I would like this to be in sync. |
But the thing is that you are not going to be able to test this sample unless it's tied to a particular revision of Zephyr. Instead, in the case of Zephyr, we keep a reference to a stable version of mcumgr, which in my opinion is the correct dependency order. |
Right, I don't think that's gonna work reliably. There are API deprecations and changes in zephyr periodically, so I do believe that the better approach is the opposite. I will not block the PR, but I am not convinced that it provides much value. |
What do you mean by "opposite"? |
Exactly keeping smp_svr exclusively in the zephyr code-base, which force us, so zephyr community to maintenance it. so #60 approach |
No longer valid after #62 has been merged. |
Zaphyr RTOS is keeping its own copy of sample and this sample has not
been updated far too long to work with Zephyr.
Signed-off-by: Dominik Ermel dominik.ermel@nordicsemi.no