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

IPCore String-based API removal #11942

Merged
merged 4 commits into from
Feb 21, 2020

Conversation

michalpasztamobica
Copy link
Contributor

@michalpasztamobica michalpasztamobica commented Nov 26, 2019

Summary of change

Remove the deprecated string-based APIs. This is a breaking change, targeted for mbed-os-6.0.
The APIs were deperecated in mbed-os-5.15, see #11914. Their internal usage was given up in #11941.

The following API functions are being removed:

TCPServer (whole class is deprecated already)
TCPSocket::connect(const char *host, uint16_t port);
TLSSocket::connect(const char *host, uint16_t port);
DTLSSocket::connect(const char *host, uint16_t port)
InternetDatagramSocket::sendto(const char *host, uint16_t port, data, size);
InternetSocket::bind(const char *address, uint16_t port);
L3IP:add_ipv4_multicast_group(const char *address);
L3IP:add_ipv6_multicast_group(const char *address)
L3IP:remove_ipv4_multicast_group(const char *address);
L3IP:remove_ipv6_multicast_group(const char *address)
NetworkInterface::get_ip_address()
NetworkInterface::get_netmask()
NetworkInterface::get_gateway()
NetworkInterface::set_network(const char *ip_address, const char *netmask, const char *gateway)
NetworkStack::get_ip_address()
NetworkStack::get_ip_address_if()

Impact of changes

Applications using the old APIs will not compile.

Migration actions required

Users are expected to use their SocketAddress-based counterparts and explicitly call gethostbyname() if DNS address resolution is needed.

Documentation

Documentation was added to the first PR: #11914.


Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[x] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@AnttiKauppila
@SeppoTakalo

@ciarmcom
Copy link
Member

@michalpasztamobica, thank you for your changes.
@AnttiKauppila @SeppoTakalo @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

@0xc0170 0xc0170 added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Nov 26, 2019
SeppoTakalo
SeppoTakalo previously approved these changes Nov 26, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 26, 2019

Please review travis failures, they look related

@michalpasztamobica
Copy link
Contributor Author

@0xc0170 , this PR removes the deprecated string-based APIs. It will only compile when the internal usage of these deprecated API functions is removed. So, effectively, this PR depends on #11941 .
Sorry if I submitted this prematurely, at least we have time for everyone to review...

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 27, 2019

@michalpasztamobica All fine here, just state the dependency (adding here label needs preceding PR)

@michalpasztamobica michalpasztamobica force-pushed the remove_deprecated_apis branch 2 times, most recently from 483d921 to 676c07b Compare December 2, 2019 07:05
@michalpasztamobica
Copy link
Contributor Author

This is now rebased and ready for CI, @0xc0170 .
Just a reminder: this is a breaking change... I think we need that big red label here?

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 2, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 2, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

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

@michalpasztamobica
Copy link
Contributor Author

CI has found a number of platforms, which I did not build locally, that may need adjustments as they are still using the deprecated API. Unfortunately I did not find them all in #11941, as I was not able to build every possible platform. I will replace their usage with the new API, but it will take some time.

@michalpasztamobica
Copy link
Contributor Author

@0xc0170 , please help me make sure I got things right.
It seems that CI is using the examples.py script to build and run official example projects (I found CI failing in this line).
To have this PR merged I therefore need to fix all the example projects first?
I already have some code prepared, for example here: ARMmbed/mbed-os-example-wifi#176. But this will only compile, when I update the mbed-os versions to a commit which contains the new SocketAddress-based API.
I can therefore either:

  1. wait for mbed-os-5.15 release, then update all the examples,
    OR
  2. update all the examples, with mbed-os.lib pointing to the commit which contains new API addition (but does not have any particular tag).

I guess option (2) is not real, I have only seen mbed-os.lib being updated to release tags.

Either way I can only resolve the failures of this PR once examples are all updated.

Is that right, or is there some smarter way of merging this sooner?

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the description (new template as we used here the old back in November).

@mbed-ci
Copy link

mbed-ci commented Feb 11, 2020

Test run: FAILED

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

Failed test jobs:

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

@mergify mergify bot added needs: work and removed needs: CI labels Feb 11, 2020
@michalpasztamobica
Copy link
Contributor Author

michalpasztamobica commented Feb 11, 2020

@0xc0170 , failures seem unrelated (K66F mesh-minimal-example):

[DEBUG] Output:   DigitalOut led_1(MBED_CONF_APP_LED, 1);
[DEBUG] Output:                    ^
[DEBUG] Output: "/builds/ws/mbed-os-ci_build-IAR@6/examples/mbed-os-example-mesh-minimal/mesh_led_control_example.cpp",39  Error[Pe020]: identifier "MBED_CONF_APP_LED" is undefined
[DEBUG] Output: 
[DEBUG] Output:   InterruptIn my_button(MBED_CONF_APP_BUTTON);
[DEBUG] Output:                         ^
[DEBUG] Output: "/builds/ws/mbed-os-ci_build-IAR@6/examples/mbed-os-example-mesh-minimal/mesh_led_control_example.cpp",40  Error[Pe020]: identifier "MBED_CONF_APP_BUTTON" is undefined
[DEBUG] Output: 
[DEBUG] Output:   DigitalOut output(MBED_CONF_APP_RELAY_CONTROL, 1);
[DEBUG] Output:                     ^
[DEBUG] Output: "/builds/ws/mbed-os-ci_build-IAR@6/examples/mbed-os-example-mesh-minimal/mesh_led_control_example.cpp",41  Error[Pe020]: identifier "MBED_CONF_APP_RELAY_CONTROL" is undefined
[DEBUG] Output: 
[DEBUG] Output:           my_button.mode(MBED_CONF_APP_BUTTON_MODE);
[DEBUG] Output:                          ^
[DEBUG] Output: "/builds/ws/mbed-os-ci_build-IAR@6/examples/mbed-os-example-mesh-minimal/mesh_led_control_example.cpp",210  Error[Pe020]: identifier "MBED_CONF_APP_BUTTON_MODE" is undefined

And then things like this for nanostack-border-router:

[DEBUG] Output: "/builds/ws/mbed-os-ci_build-IAR@6/examples/nanostack-border-router/source/static_6lowpan_config.h",40  Error[Pe020]: identifier "MBED_CONF_APP_BEACON_PROTOCOL_ID" is undefined

And the same happened for NUCLEO_F429ZI

EDIT: and they only occur for IAR 👀

@michalpasztamobica
Copy link
Contributor Author

I tried to rerun just the IAR build which failed, but it yielded the same result (could not build blinky-baremetal). The logs are not showing which version gets built, so I assume it might just be building the old code?
@0xc0170 , could it help if you triggered the CI to run from scratch?

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 13, 2020

CI restarted

@mbed-ci
Copy link

mbed-ci commented Feb 13, 2020

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 12
Build artifacts

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 13, 2020

Final approval from @bulislaw and this will go in soon.

@0xc0170 0xc0170 added release-version: 6.0.0-alpha-3 and removed release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 labels Feb 21, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 21, 2020

CI started (to make sure nothing has changed in the last days), should be quick

@mbed-ci
Copy link

mbed-ci commented Feb 21, 2020

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 13
Build artifacts

@kjbracey
Copy link
Contributor

I've been busy shifting virtual to override, and rebasing on top of this revealed some misalignments between base and derived classes. See my PRs.

@cmonr
Copy link
Contributor

cmonr commented Feb 22, 2020

Leaving a note here.
Looks like one of the examples in the docs needs to be updated.

Docs are now failing one of the code snippet tests: https://travis-ci.org/ARMmbed/mbed-os-5-docs/jobs/653627350#L807

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.