-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
Ah crud, looks like both mbed-cloud-client and easy-connect add Should we hold off on this kind of change then? More likely |
@teetak01 @JanneKiiskila Thoughts? |
Side note, the config tests don't seem to run correctly under Python 3 when I tested locally. |
Any review for this PR? @bridadan This needs a rebase now. |
I think the FEATURE_PAL was removed in 5.9 version of Mbed OS? |
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: So if you remove those from allowed features, remove the whole |
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. |
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. |
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. |
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 |
👍 |
19c94ef
to
b23c868
Compare
Rebased and moved all FEATURE_LWIP references in the config testing FEATURE_BOOTLOADER |
@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-tools @theotherjimmy When y'all get a chance. |
@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. |
Found the error:
|
@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 |
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:
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 This PR could be split to two:
|
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!).
I left
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 Regarding splitting this into two changes, that's no problem! Just want to clarify what you mean:
|
Test run: FAILEDSummary: 1 of 7 test jobs failed Failed test jobs:
|
@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? |
It should be able to be removed considering there is no code for |
I removed both LWIP and COMMON_PAL since both of those features are removed/deprecated. |
Will rerun CI in the meanwhile and hold for merge for the final OK. |
CI started |
Test run: FAILEDSummary: 2 of 11 test jobs failed Failed test jobs:
|
|
See https://github.com/ARMmbed/mbed-os/releases/tag/mbed-os-5.10.0
|
@SeppoTakalo that's what I'm trying to do 😄, there are no more mentions of |
@bridadan cloud client test pass now |
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? |
Whoops. Looks like I had the old webpage cached.... Restarting CI job: License network issues. |
@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 |
@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 |
@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? |
@screamerbg This will not break the online complier, as it omits the whitelist check. |
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 onlyFEATURE_*
folders that I see in the Mbed OS tree:This change only leaves the above features enabled. The following features have been removed from the whitelist:
I don't believe this is a breaking change, but it'd be good to hear from @theotherjimmy.
Pull request type