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_LWIP #7287

Merged
merged 4 commits into from Jun 25, 2018

Conversation

Projects
None yet
7 participants
@SeppoTakalo
Contributor

SeppoTakalo commented Jun 21, 2018

Description

Follow up on removal of FEATURE_NANOSTACK
Now do the same for LWIP so that all networking functions are enabled on all builds so that adding networking capabilities to targets that did not originally have those, do not require enabling of any feature.

Does not affect RAM/ROM usage.

Pull request type

[ ] Fix
[X] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

SeppoTakalo added some commits Jun 21, 2018

Remove FEATURE_LWIP. The LwIP stack is enabled on all builds now
Leave the FEATURE_LWIP enabled in build scripts so that it does
not break any builds.

Removed 'feature_add: ["LWIP"]' on all targets.
Add deprecation warning for those who enable LWIP or NANOSTACK
This is dropped by linker so does not cause any RAM/ROM cost.
@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Jun 21, 2018

@kjbracey-arm please review.

@0xc0170 0xc0170 requested a review from kjbracey-arm Jun 21, 2018

@0xc0170 0xc0170 requested a review from bulislaw Jun 21, 2018

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jun 21, 2018

Hi
Maybe a cleanup in tools/test_configs json files is also needed ?

@@ -845,7 +845,7 @@ EXCLUDE_PATTERNS = */tools/* \
*/features/mbedtls/* \
*/features/storage/* \
*/features/unsupported/* \
*/features/FEATURE_LWIP/* \
*/features/lwipstack/* \

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jun 21, 2018

Contributor

It's probably more straightforward just to be /features/lwip, I think. Does the "stack" really add anything in this context?

LWIPStack versus LWIPInterface objects kind of happened because they were in a single namespace, and reflected the underlying NetworkStack and NetworkInterface. But in the filing system like this, I think just lwip is fine. Maybe at some point lwip might end up in a networkstacks folder, if there were that many.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jun 21, 2018

Contributor

It's possible there should be a general tidy-up to add a top-level features/networking or something too. But I think that's out of scope for this. Anyway, for now don't try to express something in your leafname that would be better expressed in a directory structure.

This comment has been minimized.

@SeppoTakalo

SeppoTakalo Jun 21, 2018

Contributor

Within the lwipstack there is already folder lwip which is the ... stack.. :D

Would lwip/lwip look terrible? Currently the stack is in lwipstack/lwip

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 21, 2018

Travis failure in events is similar to the one we had for nanostack move? Please update

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Jun 21, 2018

@jeromecoutant

Maybe a cleanup in tools/test_configs json files is also needed ?

Why?
I don't see anything relevant to LWIP there.

Maybe the title is a bit wrong, but I'm not removing the LwIP stack.
I'm enabling it for all builds.

So the FEATURE_LWIP that is used to enable the stack, is now gone. Same we did for Nanostack earlier.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 25, 2018

I'll run first CI round

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 25, 2018

Build : SUCCESS

Build number : 2439
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7287/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 25, 2018

Waiting on @bulislaw for a final review.

@cmonr cmonr merged commit f3424da into ARMmbed:master Jun 25, 2018

14 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 926 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10163 cycles (+1228 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 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment