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

Removing feature names from the "allowed feature" config list. #7799

Merged
merged 2 commits into from Dec 19, 2018

Conversation

Projects
None yet
@bridadan
Copy link
Contributor

commented Aug 15, 2018

Description

I was going through the OS source and noticed this list still references a bunch of FEATURE_* folders that don't exist anymore. Currently, these are the only FEATURE_* folders that I see in the Mbed OS tree:

$ cd mbed-os
$ find features -regextype sed -regex "^.*/FEATURE_[^/]*$"
features/cryptocell/FEATURE_CRYPTOCELL310
features/deprecated_warnings/FEATURE_NANOSTACK
features/deprecated_warnings/FEATURE_LWIP
features/FEATURE_UVISOR
features/FEATURE_BLE
features/storage/FEATURE_STORAGE

This change only leaves the above features enabled. The following features have been removed from the whitelist:

"CLIENT",
"IPV4",
"COMMON_PAL",
"LOWPAN_BORDER_ROUTER",
"LOWPAN_HOST",
"LOWPAN_ROUTER",
"NANOSTACK_FULL",
"THREAD_BORDER_ROUTER",
"THREAD_END_DEVICE",
"THREAD_ROUTER",
"ETHERNET_HOST"

I don't believe this is a breaking change, but it'd be good to hear from @theotherjimmy.

Pull request type

[ ] Fix
[x] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change

@bridadan bridadan requested a review from theotherjimmy Aug 15, 2018

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2018

Ah crud, looks like both mbed-cloud-client and easy-connect add COMMON_PAL in their mbed_lib.json. If I remove those lines it compiles ok.

Should we hold off on this kind of change then? More likely mbed-cloud-client and easy-connect should be updated to remove these lines (considering COMMON_PAL doesn't exist anymore)?

@cmonr cmonr added the needs: review label Aug 15, 2018

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2018

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2018

Side note, the config tests don't seem to run correctly under Python 3 when I tested locally.

@0xc0170 0xc0170 requested review from ARMmbed/mbed-os-tools Sep 21, 2018

@0xc0170

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

@teetak01 @JanneKiiskila Thoughts?

Any review for this PR?

@bridadan This needs a rebase now.

@teetak01

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

I think the FEATURE_PAL was removed in 5.9 version of Mbed OS?

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Actually, FEATURE_NANOSTACK, FEATURE_LWIP and FEATURE_COMMON_PAL has all been deprecated in same time.

And I don't think it was published in 5.9, so actually those are removed in 5.10 release. So if you remove these now, it can only be published in 5.11

NANOSTACK or LWIP features are causing the deprecation warning by defining dummy functions like this:
https://github.com/ARMmbed/mbed-os/blob/master/features/deprecated_warnings/FEATURE_NANOSTACK/feature_nanostack_is_deprecated.c

So if you remove those from allowed features, remove the whole deprecated_warnings folder.

@teetak01

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Based on @SeppoTakalo I would keep this PR in hold until 5.11.0-RC1 (thus no merging to master) until we can check that no components are anymore depending on this.

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Well, I would merge it into master.

That would crash all tests that use deprecated features, which is fine. Gives at least clear indicator that they are not up to date.

Waiting for release candidate means that these are going to be fixed when we are in most busiest time. So I would not wait with the merging, at least not longer than 5.10 is out.

@TeroJaasko

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

It might be nicer for application developer if he was given time to prepare for COMMON_PAL removal, just as was done with deprecated_warnings/FEATURE_NANOSTACK/. So why not have the COMMON_PAL give deprecation warning for one release (5.11) and then remove it completely. Just killing application build suddenly overnight is not that nice. Sometimes it is really useful to be able to swap between master and release versions of Mbed OS.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Sep 21, 2018

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2018

Thanks for the feedback everyone.

@teetak01 Thanks for the changes in ARMmbed/mbed-cloud-client-internal#1146 and ARMmbed/update-client-hub#371

@0xc0170 Since I've implemented this, LWIP is now a deprecated feature. I'm happy to rebase this, but I'd like to update the tests to use a feature that won't be removed immediately. Browsing the features folder, FEATURE_BOOTLOADER looks like a good candidate. Do you think this one makes sense to use?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

@0xc0170 Since I've implemented this, LWIP is now a deprecated feature. I'm happy to rebase this, but I'd like to update the tests to use a feature that won't be removed immediately. Browsing the features folder, FEATURE_BOOTLOADER looks like a good candidate. Do you think this one makes sense to use?

👍

@bridadan bridadan force-pushed the bridadan:remove_allowed_features branch to b23c868 Oct 1, 2018

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2018

Rebased and moved all FEATURE_LWIP references in the config testing FEATURE_BOOTLOADER

@cmonr cmonr added needs: review and removed needs: work labels Oct 8, 2018

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2018

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

@bridadan It looks like this isn't passing the cloud_client_smoke_test. I've restarted the job in case it was intermittent, but the failuire was across three compilers, so not counting on it.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

Found the error:

[Build K64F ARM] Building project mbed-cloud-client-example (K64F, ARM)
[Build K64F ARM] Scan: mbed-cloud-client-example
[Build K64F ARM] Scan: env
[Build K64F ARM] [ERROR] Feature 'COMMON_PAL' is not a supported features
[Build K64F ARM] [mbed] ERROR: "/usr/bin/python" returned error.
[Build K64F ARM]        Code: 1
[Build K64F ARM]        Path: "/builds/ws/mbed-os-ci-cloud-client-test-job/mbed-cloud-client-example"
[Build K64F ARM]        Command: "/usr/bin/python -u /builds/ws/mbed-os-ci-cloud-client-test-job/mbed-cloud-client-example/mbed-os/tools/make.py -t ARM -m K64F --source . --build ./BUILD/K64F/ARM"
[Build K64F ARM]        Tip: You could retry the last command with "-v" flag for verbose output
[Build K64F ARM] ---

@cmonr cmonr added needs: work and removed needs: review labels Oct 19, 2018

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2018

@0xc0170

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

@cmonr that's correct, it will fail until ARMmbed/mbed-cloud-client-internal#1146 and https://github.com/ARMmbed/update-client-hub/pull/371/files are merged

I don't think only these 2 are gating this. I might be the only one confused , so let me write down my understanding what is going on here:

Looking at the changes does not say much (lot of clean-up). The most important change that I did not find in the description above: https://github.com/ARMmbed/mbed-os/pull/7799/files#diff-0f1a973b12e465163c3fe8f2d1f933c9L66

These are being removed:

"CLIENT",
"IPV4",
"COMMON_PAL",
"LOWPAN_BORDER_ROUTER",
"LOWPAN_HOST",
"LOWPAN_ROUTER",
"NANOSTACK_FULL",
"THREAD_BORDER_ROUTER",
"THREAD_END_DEVICE",
"THREAD_ROUTER",
"ETHERNET_HOST"

Not "LWIP", neither "STORAGE" ? When these features were deprecated ?

These 2 have warnings: https://github.com/ARMmbed/mbed-os/tree/master/features/deprecated_warnings . Where is the rest ? Does LOWPAN_HOST trigger deprecated warning?
Is it ideal (having features supported in the tools, but deprecation only in the code), why there is no DEPRECATED_FEATURES - it would be in one place (config would take care of - allowed vs deprecated vs removed). I am confused about how we deprecate features after looking at this changeset. It looks like some were just removed, some added warnings to the code.

This PR could be split to two:

  1. clean up our code - remove deprecated features references (simple fix, no tools touched). We can easily scope this PR down to this step, and continue with 2nd that might require some more work around the code and docs
  2. deprecate features removal- make sure deprecated features issue warning (not certain we want removal before it is documented "when it can be removed"- it's been already discussed, the doc might be up there, please reference if it is, if not, we need to add it).
@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

Thanks for your comments @0xc0170.

You are correct, I should have listed in my description the features being removed. I have updated the description with the list you posted (thanks!).

Not "LWIP", neither "STORAGE" ? When these features were deprecated ?

I left LWIP and STORAGE features in place since there are currently FEATURE_LWIP and FEATURE_STORAGE directories in the OS.

Where is the rest ? Does LOWPAN_HOST trigger deprecated warning?
Is it ideal (having features supported in the tools, but deprecation only in the code), why there is no DEPRECATED_FEATURES - it would be in one place (config would take care of - allowed vs deprecated vs removed). I am confused about how we deprecate features after looking at this changeset. It looks like some were just removed, some added warnings to the code.

I don't really know to be honest, it does seem like the other features were just removed without the deprecation warnings that we have for LWIP and NANOSTACK. I wasn't involved in that effort so I can't say. Perhaps someone else from the OS team can comment on what the strategy is moving forward for deprecating the use of features?


Regarding splitting this into two changes, that's no problem! Just want to clarify what you mean:

  1. For this change, I would reduce it to everything except the changes in this list, correct? https://github.com/ARMmbed/mbed-os/pull/7799/files#diff-0f1a973b12e465163c3fe8f2d1f933c9L66

  2. For this one would I would limit it to ONLY the changes in this list, correct? https://github.com/ARMmbed/mbed-os/pull/7799/files#diff-0f1a973b12e465163c3fe8f2d1f933c9L66

@mbed-ci

This comment has been minimized.

Copy link

commented Dec 14, 2018

Test run: FAILED

Summary: 1 of 7 test jobs failed
Build number : 8
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR

@cmonr cmonr added needs: work and removed needs: CI labels Dec 14, 2018

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

@bridadan It would appear that TEST_APPS-DEVICE-SOCKET_APP still needs COMMON_PAL.

@SeppoTakalo Would you or your team know if COMMON_PAL can be removed from the test applicaton?

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2018

It should be able to be removed considering there is no code for COMMON_PAL anymore 😄. Good to double check with @SeppoTakalo anyway though.

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2018

I removed both LWIP and COMMON_PAL since both of those features are removed/deprecated.

@cmonr cmonr added needs: CI and removed needs: work labels Dec 14, 2018

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

Will rerun CI in the meanwhile and hold for merge for the final OK.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Dec 15, 2018

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
  • jenkins-ci/mbed-os-ci_exporter
@0xc0170

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

ConfigException: Feature 'COMMON_PAL' is not a supported features from cloud client test - it needs an update?

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Dec 17, 2018

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

COMMON_PAL can be removed.

See https://github.com/ARMmbed/mbed-os/releases/tag/mbed-os-5.10.0

Connectivity stack always builtin
Applications are no longer required to specify FEATURE_NANOSTACK, FEATURE_LWIP or FEATURE_COMMON_PAL to use any of the connectivity stacks that Mbed OS supplies. Those are always included in the build.

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

@SeppoTakalo that's what I'm trying to do 😄, there are no more mentions of COMMON_PAL within the mbed-os tree in this branch. But it looks like the CI pulls in a pinned version of the cloud client example, specifically version 2.0.0. Unfortunately, I don't think I can fix this within mbed-os. I think this reference lives in a magic CI repo somewhere... Perhaps @OPpuolitaival can help?

@OPpuolitaival

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

@bridadan cloud client test pass now

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

Think I found where it's still being referenced: https://github.com/ARMmbed/mbed-cloud-client/blob/f84aeea5047569b5833caf84056e4b3aa429e12c/CHANGELOG.md#release-210-11122018

Note that the reference to COMMON_PAL is removed in 2.1.0 for that repo, not 2.0.0. The 2.1.0 release of mbed-cloud-client was released mid-Nov.

@OPpuolitaival @JanneKiiskila How should we proceed since it seems that this needs a CI update? Should we open a PR against the repo to update the pinned version? Is there a process to test the updated version with master before deploying to CI?

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

Whoops. Looks like I had the old webpage cached....

Restarting CI job: jenkins-ci/exporter

License network issues.

@cmonr cmonr merged commit 442cbba into ARMmbed:master Dec 19, 2018

21 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 Internal error, will be fixed
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 Passed, 0 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10064 cycles (+856 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
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

@cmonr cmonr removed the ready for merge label Dec 20, 2018

@screamerbg

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2018

@bridadan As the mbed-os build system is at the heart of the Online Compiler build service, which has to support both Mbed OS 2 and Mbed OS 5.1-5.11, removing these features will inevitably break the Online Compiler build process with older versions of Mbed OS. Imagine using the current tools with an older codebase and an older version of targets.json (but still backwards compatible), which has features_add: ["LOWPAN_BORDER_ROUTER"] or one of the other tags removed with this PR. You could imagine how it would fail miserably because of this PR.

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 24, 2018

@screamerbg At the time I made this PR I didn't realize how that part of the system worked. I recently learned about it and I should have taken this PR down, but it slipped through the cracks. Thanks for the heads up 👍

@cmonr I hate to say this since we had this PR open for so long, but can we revert this so we don't publish this accidentally in a tools release? I think as a maintainer you have a big revert button. If not I'm happy to put up a PR with the revert commit

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2018

@screamerbg That's unfortunate. Do you see anytime in the near future where we'd be able to make a PR like this again, without it affecting the online compiler?

@bridadan My magic revert button opened a PR 😄 #9212

@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2019

@screamerbg This will not break the online complier, as it omits the whitelist check.

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.