obgm
Changes from Iotivity for draft-ietf-core-coap-tcp-tls and cross-platform support
These changes were already in the Iotivity code base, and we're trying to unfork libcoap and have iotivity just use the regular libcoap library. These changes include support for draft-ietf-core-coap-tcp-tls and some cross-platform fixes (e.g., to also run on Windows).
Thanks, this contribution is highly appreciated. Unfortunately, Travis reports this PR to break the Linux port. It would be great if you could take a look into that issue.
My apologies -- I have pushed an old patch to src/block.c which now breaks this PR.
Is this task tracked on iotivity tracker ?
I see there is a conflict to be resolved
Yes https://jira.iotivity.org/browse/IOT-1072 tracks the iotivity task. IoTivity already uses this branch temporarily on Windows and other OS's will start snapping to it after merge. I will rebase to fix the conflict.
I believe all feedback so far has been addressed. Would like to see this PR merged so iotivity can start depending on the upstream branch. Let me know if there are any more blocking issues.
Hello Dave,
first thanks for contributing! Some comments below.
Beside the build issues I'd like to see better commit messages, please see and note https://github.com/obgm/libcoap/blob/develop/CONTRIBUTE#L161 Please describe why the commit is made in the way you have done and make explanation what the commit is about (for example c1bda6e0a4136bc3a3f981ffb4d38a984f2a4f3c 749514f4200cc0675dc134562b42a8ca06b831fe fd5c9ec1f584c06fc1608df61e32fe4694c1b0df). It's a bit tedious to go through every source change to understand what's changed and why. That's obviously not needed for typo fixes, but a good commit message helps to see what was changed.
Please don't mix extensions of specific changes to the header files or small adoptions within the header files with improving of the API documentation in other parts, split them of into more easier to understand changes. For example c11591d5237dfd8880971b319eabab8d81db7e9b is much to big. But other commits will need also be split of into smaller peaces.
Also try to decrease merge commit from upstream much as possible, normally there shouldn't be even one as you should use git rebase instead of git merge to pull upstream changes. I'm currently not able to make a review due the huge changes that I not really understand what and why the source is changed.
Please don't be disappointed, such big changes need time to get integrated. Maybe you can split the PR into more than one PR if it turns out there are some thematic changes. If I read your first post this PR is including two things, tcp-tls and cross-platform support for Windows. For the latter it would be good to have build support than.
Regards Carsten
Hi Dave, I had a quick look at the code and would like to process it quickly. Do you see a chance to address Carsten's comments by splitting this PR into multiple commits? (Edit: There are still several open issues, see, e. g., travis build.)
Building with -DWITH_TCP=1 (after fixing the build errors) yields even more warnings.
FYI, The changes in this PR have been done in IoTivity's fork of this repo across the course of a couple years by a bunch of people. I am merely pushing them here, I am not authoritative for all the original changes, just trying to get rid of a forked copy and make contributions in the future to a single place rather than IoTivity folks continuing to make changes to their own fork. I will do what I can, but I may not be able to answer all questions myself. Pushing back just means that tons of devices (like all the devices Samsung ships) will continue to use a separate fork not the master GitHub version.
Hi as Samsung OSG developer I believe it's better for iotivity to be the most aligned possible with upstream, thanks @dthaler for your efforts.
Dave, thanks for a little bit explanation about the details of this PR. I agree and encourage the best for libcoap and the other projects is obviously to land pull requests early in libcoap and the uses code is not differing much between projects. It's like in the kernel, commit early and do also a pull request early.
I think Olaf is of course willing to pull your changes in this PR. But we need to puzzle the various things out into parts that are logical things. That for one simple reason, not only we need to understand what the changes are about. So how to get further here?
I'd suggest to do the easy parts first, for example changes that are made by you or direct working people.
Olaf has released the recent state of libcoap as version 4.1.2 so we can start now mostly lazy with new changes.
It certainly is high on my priority list but sorting stuff out will take some time. Maybe it is feasible to cherry-pick some of the Windows-specific compatibility code. That part seems pretty straight-forward to me.
@dthaler I am having trouble understanding what these changes are about. For example, commit 5f2d2386 introduces a function coap_add_token_to_empty_message(). Neither the commit log nor the doxygen comment for the function prototype in commit bd60461d explain what this function is about and, more important, why it is needed in addition to coap_add_token().