Skip to content

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

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

Merged
merged 2 commits into from
Dec 19, 2018

Conversation

bridadan
Copy link
Contributor

@bridadan bridadan 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 August 15, 2018 15:35
@bridadan
Copy link
Contributor Author

bridadan 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
Copy link
Contributor

cmonr commented Aug 15, 2018

@teetak01 @JanneKiiskila Thoughts?

@bridadan
Copy link
Contributor Author

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

@0xc0170 0xc0170 requested review from a team September 21, 2018 09:05
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 21, 2018

@teetak01 @JanneKiiskila Thoughts?

Any review for this PR?

@bridadan This needs a rebase now.

@teetak01
Copy link
Contributor

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

@SeppoTakalo
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

@bridadan
Copy link
Contributor Author

Thanks for the feedback everyone.

@teetak01 Thanks for the changes in https://github.com/ARMmbed/mbed-cloud-client-internal/pull/1146 and https://github.com/ARMmbed/update-client-hub/pull/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
Copy link
Contributor

0xc0170 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 remove_allowed_features branch from 19c94ef to b23c868 Compare October 1, 2018 18:53
@bridadan
Copy link
Contributor Author

bridadan commented Oct 1, 2018

Rebased and moved all FEATURE_LWIP references in the config testing FEATURE_BOOTLOADER

@cmonr
Copy link
Contributor

cmonr commented Oct 8, 2018

@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-tools @theotherjimmy When y'all get a chance.

@cmonr
Copy link
Contributor

cmonr 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
Copy link
Contributor

cmonr 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] ---

@bridadan
Copy link
Contributor Author

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

@0xc0170
Copy link
Contributor

0xc0170 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
Copy link
Contributor Author

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
Copy link

mbed-ci 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
Copy link
Contributor

cmonr 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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@cmonr
Copy link
Contributor

cmonr commented Dec 14, 2018

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

@cmonr
Copy link
Contributor

cmonr commented Dec 14, 2018

CI started

@mbed-ci
Copy link

mbed-ci 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
Copy link
Contributor

0xc0170 commented Dec 17, 2018

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

@SeppoTakalo
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

@bridadan cloud client test pass now

@cmonr
Copy link
Contributor

cmonr 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
Copy link
Contributor

cmonr 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
@screamerbg
Copy link
Contributor

screamerbg 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
Copy link
Contributor Author

@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
Copy link
Contributor

cmonr 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
Copy link
Contributor

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.