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

Remove FEATURE_STORAGE and all underlying deprecated features #10258

Merged
merged 1 commit into from Apr 26, 2019

Conversation

Projects
None yet
9 participants
@davidsaada
Copy link
Contributor

commented Mar 28, 2019

Description

FEATURE_STORAGE and underlying features - cfstore, flash-journal and storage-volume-manager, have long been deprecated. This PR removes them all.
In addition, remove references to these in nanostack (used cfstore in uninvoked tests) and tools metadata such as astyle and doxygen.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SenRamakri

Release Notes

Remove FEATURE_STORAGE and all underlying deprecated features: cfstore, flash-journal and storage-volume-manager.

@ciarmcom ciarmcom requested review from SenRamakri and ARMmbed/mbed-os-maintainers Mar 28, 2019

@ciarmcom

This comment has been minimized.

@@ -1,6 +1,6 @@
{
"test_target": {
"expected_features": ["BOOTLOADER", "STORAGE"],
"included_source": ["FEATURE_BOOTLOADER/lib1/lib1.c", "FEATURE_STORAGE/lib2/lib2.c"]
"expected_features": ["BOOTLOADER"],

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 28, 2019

Contributor

Please don't remove code that tests how the tools work WRT features just because the feature that's used as test data is no longer part of Mbed OS

This comment has been minimized.

Copy link
@davidsaada

davidsaada Mar 28, 2019

Author Contributor

I don't mind reverting this file, but will this work even if STORAGE feature and FEATURE_STORAGE directory no longer exist?
Alternatively, I can replace these with other feature and directory, if this only requires any two features.

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 28, 2019

Contributor

Yes. it uses a local folder. You're welcome to rework it to not use the string "STORAGE" if you're so inclined, but I don't think it's worth the time.

This comment has been minimized.

Copy link
@davidsaada

davidsaada Mar 28, 2019

Author Contributor

Thanks. Indeed got better things to do with my time. Reverted.

@SenRamakri

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@cmonr and @ARMmbed/mbed-os-maintainers - Can we please tag this change for 5.12.1. Without this change we cannot compile with ARM Compiler 6.12 which is the ARM compiler version with latest Keil MDK. Im afraid there will be lot of developers trying to use latest Keil MDK with MbedOS and the builds will be broken. So we need this change in ASAP(5.12.1).

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@SenRamakri @davidsaada

I'm not sure I understand. Why does removing this code happen to make compilation work with ARMC6?

And where is the tracking issue for this removal? Was it discovered during OOB?

@davidsaada davidsaada force-pushed the davidsaada:david_remove_feature_storage branch from 26f7b06 to 2a771fc Mar 28, 2019

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@cmonr We had this change planned anyway. As for the ARMC6 problem, will let @SenRamakri answer that.

@davidsaada davidsaada force-pushed the davidsaada:david_remove_feature_storage branch from 2a771fc to 732dd36 Mar 28, 2019

@SenRamakri

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@cmonr - Its not discovered during OOB because OOB used ARMC 6.11 version. We have identified some header file conflicts with ARMC 6.12 with header files present under the directories which @davidsaada is trying to remove with this commit. I brought this up to him yesterday and he prioritized this change to fix ARMC 6.12 issue although it was planned for later.

@SenRamakri
Copy link
Contributor

left a comment

@davidsaada - Changes look good to me, Ill approve.
Btw, are there any docs which also needs removed or changed based on this change?

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

My concern with this PR is that we explicit state in our guidelines that we do not deprecate features in patch releases. If there is a specific fix that is required for ARMC6 then I would prefer to see that split out into a separate PR that we can take into a patch release and then move the deprecation to the next minor...

@bulislaw

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Mbed deprecation policy says that we can remove features that were marked as deprecated for at least 6 months. Can you confirm all of the removed items were explicitly marked as deprecated for at least 6 months?

Also that will need to go into 5.13 not any of the 5.12 patch releases.

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

@adbridge @SenRamakri @bulislaw All three features have been deprecated since mbed-os 5.5.
Neither have any reference in our documentation.

@SenRamakri

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

Sounds good - But we need "VERSION" file removed to fix the ARMC 6.12 build failures for 5.12.1.
@davidsaada - Is that ok to just remove the VERSION file for now with a separate PR and target this PR for 5.13?

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Sounds good - But we need "VERSION" file removed to fix the ARMC 6.12 build failures for 5.12.1.
@davidsaada - Is that ok to just remove the VERSION file for now with a separate PR and target this PR for 5.13?

Will do (on Sunday). In that case I believe a tracking issue would help.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@davidsaada What's the status here? The fix was merged, can this proceed?

@davidsaada

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@davidsaada What's the status here? The fix was merged, can this proceed?

From my POV it can proceed. Nothing more I have to do here. But thought this would wait for 5.13.

Review addressed

@0xc0170 0xc0170 removed the request for review from ARMmbed/mbed-os-ipcore Apr 11, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

CI started while waiting for reviews

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Apr 11, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 11, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

CI job restarted: jenkins-ci/exporter

Failures appear relevant, but making sure.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Restarted, latest failure was success but exporter reported failure

@0xc0170 0xc0170 added needs: review and removed needs: CI labels Apr 12, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@bulislaw Please review

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 17, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test
@bulislaw
Copy link
Member

left a comment

Could you like to PR with docs changes?

@adbridge adbridge added needs: CI and removed needs: review labels Apr 25, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

ci started

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 25, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

@adbridge adbridge added ready for merge and removed needs: CI labels Apr 26, 2019

@adbridge adbridge merged commit b1cd3da into ARMmbed:master Apr 26, 2019

26 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9945 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.