search

ARMmbed / mbed-os

Arm Mbed OS is a platform operating system designed for the internet of things
https://mbed.com
Other
4.67k stars 2.98k forks source link

Prematurely introduced feature in PATCH release of Mbed OS (5.11.2) #9519

Closed screamerbg closed 5 years ago

screamerbg commented 5 years ago

Description

In Mbed OS 5.11.2 (PATCH release), a feature was introduced to blockdevice, which breaks blockdevice drivers that do not live inside Mbed OS - https://github.com/ARMmbed/mbed-os/pull/9135

This is diverging from the Mbed OS features release policy and also from principles of how code is rolled out in Mbed OS so far (Mbed OS 5.0 and onwards). CC @sg- @SenRamakri @mskedia

Additionally, this prematurely introduced feature breaks all Nuvoton based platforms that connect to Pelion, which use external SD card DMA driver (specific to Nuvoton) to store credentials and firmware images. For clarity, the impacted boards are:

Due to the patch, any Pelion DM or storage example which makes use of the Nuvoton SD DMA driver (or any external blockdevice driver for Mbed OS), fails with the following error:

Error:  #322: object of abstract class type "NuSDBlockDevice" is not allowed:
            pure virtual function "mbed::BlockDevice::get_type"  has no overrider

Lastly, we don't expect that every single blockdevice driver will live inside Mbed OS, just as we don't anticipate that other drivers (IP connectivity, BLE, etc) will always live inside Mbed OS. Therefore, the patch should be reverted and removed from the Mbed OS codebase immediately, and reintroduced in Mbed OS 5.12.

I'll mark the issue as bug, although it is an issue with the release process of Mbed OS.

CC @0xc0170 @samchuarm @cyliangtw

Issue request type

[ ] Question
[ ] Enhancement
[X] Bug
samchuarm commented 5 years ago

@ARMmbed/team-nuvoton

ciarmcom commented 5 years ago

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-806

0xc0170 commented 5 years ago

We will review the messaging for this change (release notes, commit messages - they should have answered questions asked) and do retrospective. I'll create a ticket for us (=maintainers).

screamerbg commented 5 years ago

@0xc0170 What's the outcome of this? I haven't seen any output. Shall I reopen this ticket for us to track it?

0xc0170 commented 5 years ago

@dannybenor Please provide an update for closur on Github when closing an issue in Jira.

The reason for providing this in the patch is in the referenced pull request. The process update was done and can be found: https://os.mbed.com/docs/mbed-os/latest/contributing/workflow.html - Functional or Breaking changes require "Release notes" section with details about why are we doing it and how to migrate current code. 5.12 PRs including functionality changes should have this new section present. They will be used in 5.12 release notes.

screamerbg commented 5 years ago

@0xc0170 Thanks for the explanation. Unfortunately I don't think that this was handled in a way the process indicates.

Namely PR https://github.com/ARMmbed/mbed-os/pull/9135 is marked as "Functionality change" and according to the workflow:

Functionality change A functionality change can be any change in the functionality, including adding a new feature, a new method or a function. Software language does not matter. A feature contribution contains a new API, capability or behavior. It does not break backward compatibility with existing APIs, capabilities or behaviors.

As explained above, this functionality contains a breaking change. Here is the definition for "Breaking change"

Breaking change A breaking change is any change that results in breaking user space. It should have strong justification for us to consider it. Often, such changes can be backward compatible, for example, deprecating the old functionality and introducing the new replacement. A contribution containing a breaking change is the most difficult PR to get merged. Any breaking changes in a codebase can have a large negative impact on any users of the codebase. Breaking changes are always limited to a major version release.

But the PR is not marked as a breaking change (see the empty checkbox).

I'm looking forward to learning more about the outcome / conclusion on how to improve the handling of these cases in the future.

0xc0170 commented 5 years ago

PR type was set incorrectly. It shall be edited to have "Breaking change" - I've fixed it now. As this was for 5.11, the release notes section is not present.

I'm looking forward to learning more about the outcome / conclusion on how to improve the handling of these cases in the future.

Who introduces changes like this must think about impact, test and write release notes with as much details as they can. Providing the release notes with good level of details should minimize the errors. This should help also reviews/users to to spot changes that are breaking but not introduced as such.

By default, these type of changes (functional changes including breaking ones) are not allowed to the patch releases. It can be only overruled by the product team (this was the case). If this is the case, we will make sure it is communicated (in the PR, in the release notes).

Any breaking change need an approval by the tech lead and the project lead.

cc @bulislaw

dannybenor commented 5 years ago

What is the required fix for this "bug"?

screamerbg commented 5 years ago

@0xc0170 Thanks for the comprehensive answer.