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

Removed Boost and ported to C++11. Added support for Android and iOS #26

Merged
merged 7 commits into from
Mar 7, 2017
Merged

Removed Boost and ported to C++11. Added support for Android and iOS #26

merged 7 commits into from
Mar 7, 2017

Conversation

esteve
Copy link
Contributor

@esteve esteve commented Mar 7, 2016

This PR replaces Boost with C++11 counterparts. And where they are not available, I either reimplemented them or used an alternative, namely:

  • There aren't semaphores in C++11, and it seemed to me that the interprocess features are not used, so I implemented a simple semaphore with condition variables (which are lighter, especially on Windows).
  • Instead of Boost.Asio, I added the standalone Asio distribution as a thirdparty submodule.
  • Instead of Boost.PropertyTree, I've used Tinyxml2, which is lighter and very portable.

I also added support for Android, including:

  • Added Michal Srb (@michalsrb)'s fork of Ken MacKay (@kmackay)'s implementation of ifaddrs for Android.
  • Tinyxml2, which works on Android.

Notes:

  • I couldn't find an example of the EDP XML format, so I don't know if the code I added with Tinyxml2 works fine. Could you add an example file so I can test it?
  • I had to use std::strftime instead of std::put_time because the latter is not implemented in GCC 4.8, which is the one that Ubuntu Trusty ships.

This work is completely independent and not officially endorsed or supported by OSRF, I did it in my spare time.

@esteve
Copy link
Contributor Author

esteve commented Mar 7, 2016

Also, I put the semaphore under the LGPL3 license, as the rest of the project. I tried my best to keep the same code style (tabs, naming, etc.) in the changes I made, let me know if they need reworking.

@esteve
Copy link
Contributor Author

esteve commented Apr 25, 2016

I just rebased against master and fixed the conflicts, I haven't had time to test the latest changes thoroughly, but they should work fine on Android.

@esteve
Copy link
Contributor Author

esteve commented Jul 13, 2016

Rebased this PR again against master. Changes since the last rebase:

  • I removed Boost from the examples and replaced it with C++11
  • Reorganized the files for the new FlowController and ThroughputController classes a bit so that they no longer expose <asio.hpp>
  • Added support for Universal Windows Platform apps (e.g. Windows IoT, Windows Phone, Windows 10)

@JaimeMartin
Copy link
Member

Hi Esteve,

Impressive! Thanks a lot for all this effort (again). We will test everything.

After our change to Apache License 2.0,Any contribution that you make to this repository (including this pull request) will be under the Apache 2 License, as dictated by that
license:

5. Submission of Contributions. Unless You explicitly state otherwise,
   any Contribution intentionally submitted for inclusion in the Work
   by You to the Licensor shall be under the terms and conditions of
   this License, without any additional terms or conditions.
   Notwithstanding the above, nothing herein shall supersede or modify
   the terms of any separate license agreement you may have executed
   with Licensor regarding such Contributions.

I note you published the semaphore as LGPL, could you change that? I will contact you directly.

@esteve
Copy link
Contributor Author

esteve commented Jul 14, 2016

@JaimeMartin I had the license header removed from the semaphore in a previous rebase. What the clause 5 of the Apache license says is that, given a contributed piece of code that doesn't have an explicit license, it'll default to the Apache 2.0 license. In any case, I pushed an update to the branch where I added a header to the sempahore code with the Apache 2.0 license, just to avoid any confusion.

Also, I've split the UWP code into a separate commit, in case you'd rather cherry pick only the C++11 and Android changes.

@esteve
Copy link
Contributor Author

esteve commented Aug 23, 2016

I've rebased this PR once again against master, no major changes this time.

@scott-eddy
Copy link

@JaimeMartin is there any word on when/if this will be merged into master?

@esteve
Copy link
Contributor Author

esteve commented Oct 4, 2016

Here's yet another periodic rebase. I second @scott-eddy , is there any actual plan to merge or test this? It's been 7 months since I first submitted this for review and updating this PR every time there's a conflict with master consumes a fair amount of time.

It's ok if it takes a lot of time to test all the changes (though you already use certain parts of C++11, namely std::mutex, std::unique_lock, etc.), but it'd be great to get some feedback.

@JaimeMartin
Copy link
Member

Hi Esteve,

During the last months we have been working in several features to support properly ROS2 and our customers, so it is a matter of bandwidth.

We are very interested on this, and our plan is to prepare an experimental branch to test this PR properly. I think we will be able to start the first tests in less than one month as we are incorporating a new engineer to the project.

Thanks for your patience, and your contribution.

@esteve
Copy link
Contributor Author

esteve commented Oct 4, 2016

@JaimeMartin that's fantastic, thanks for the update, I just wanted a bit of feedback. Let me know if there's anything I can help with to get this merged.

@esteve esteve changed the title Removed Boost and ported to C++11. Added support for Android. Removed Boost and ported to C++11. Added support for Android and iOS Dec 8, 2016
@esteve
Copy link
Contributor Author

esteve commented Dec 8, 2016

I just updated the description to mention that I added support for iOS to this branch.

@dirk-thomas
Copy link
Contributor

I would like to suggest that it might be helpful to split up the unrelated parts of this PR. I would really like to see the boost to c++11 changes to get merged. @JaimeMartin Can you comment if the chances to integrate these changes individually would speed things up?

@esteve
Copy link
Contributor Author

esteve commented Dec 20, 2016

@dirk-thomas good idea. I've split the first commit and moved the Android-specific changes to 53fc231

@dirk-thomas
Copy link
Contributor

@JaimeMartin @richiprosima Can you we please get a comment from you on this?

@JaimeMartin
Copy link
Member

Hi @dirk-thomas, @esteve ,

We will be releasing resources to test & merge this very soon.

@JaimeMartin JaimeMartin added the in progress Issue or PR which is being reviewed label Jan 26, 2017
@esteve
Copy link
Contributor Author

esteve commented Jan 26, 2017

@JaimeMartin thanks for the update. I just rebased this PR against master, @JavierIH @VicenteEprosima feel free to ask me any questions here about the changes.

@dirk-thomas
Copy link
Contributor

@JaimeMartin When you review the changes I would suggest to consider not to introduce custom options like EPROSIMA_HONOR_BUILD_SHARED_LIBS. CMake already has an option for that purpose: BUILD_SHARED_LIBS. If the CMake default (FALSE) doesn't match your needs you can override as we do it for ROS (see https://github.com/ros2/ament_cmake_ros/blob/master/ament_cmake_ros/cmake/build_shared_libs.cmake).

@esteve
Copy link
Contributor Author

esteve commented Jan 26, 2017

@dirk-thomas Fast-CDR already uses EPROSIMA_HONOR_BUILD_SHARED_LIBS (eProsima/Fast-CDR#8) to choose whether to use BUILD_SHARED_LIBS to let CMake build Fast-CDR as a shared library. However, feel free to submit any patches to use your approach, I'm sure the folks at eProsima will appreciate your effort.

@dirk-thomas
Copy link
Contributor

I will leave it up to eProsima to pick their strategy. I only wanted to provide my 2 cent that adding a custom option for something CMake already provides seems counterproductive. Users familiar with CMake won't know about it and will certainly not expect it. Even worse if the developer uses the CMake option to override the behavior they can affect the add_library() calls which don't use SHARED but not the ones which do (so actually asymmetric behavior - one value works the other just doesn't because it conflict with the custom option unknown to the user).

@esteve
Copy link
Contributor Author

esteve commented Jan 26, 2017

@dirk-thomas I'm not arguing with you, you raise many valid points, so if you feel so strongly about this, it'd be great you could submit a patch that addresses it, that's the beauty of opensource. I don't have much time to work on this PR, so anyone who can contribute, the more the merrier.

@richiprosima
Copy link
Contributor

@esteve changes are good for me because Fast-CDR and Fast-RTPS repositories didn't offer the alternative to build static libraries on Linux. I wasn't aware of the existence of BUILD_SHARED_LIBS. If this option is available on CMake, better not duplicate as you said. Changes have been already done on eProsima/Fast-CDR@56778bd. In the process of merging this PR we will apply the change on Fast-RTPS. Thanks to both.

@esteve
Copy link
Contributor Author

esteve commented Feb 21, 2017

I've rebased this PR and applied the same commit that @richiprosima made for Fast-CDR so that both projects use the same approach to building static libraries. I only applied the patch on non-Windows platforms, since the libraries are already built as both static and shared on Windows.

esteve and others added 2 commits March 5, 2017 04:08
This code was ported from boost to std, but it won't be used anymore.
esteve and others added 5 commits March 6, 2017 18:02
Includes additional commits by @JavierIH's addressing the following
eProsima's internal refs:

- Refs #1933. Fix error codes on logs
- Refs #1933. Modified include for asio in flowcontroller
- Refs #1933. Updated tests
- Refs #1938 Fixed XML parser
- Refs #1942. Solved some Windows related compilation errors
- Refs #1949. changed virtual for override keyword in some virtual functions
- Refs #1942. migrated tests from boost to std
- Refs #1933. Updated examples
- Refs #1933. solved some warnings related with latest changes
- Refs #1934. changed asio error type
- Refs #1933. fix deadlineqos example
- Refs #1933. fix warnings on windows
Includes additional commits by @JavierIH's addressing the following
eProsima's internal refs:

- Refs #1945. Reviewed StringMatching and added unit tests.
@esteve
Copy link
Contributor Author

esteve commented Mar 6, 2017

I've merged @JavierIH 's PR and reordered and squashed some of the commits, but both two branches are equivalent.

I just realized that this PR is one day away from turning a year old since I first submitted it for review, whoa!

@spiderkeys
Copy link
Contributor

We are very interested in using FastRTPS on Android and would love to see this land. Curious to know if this is a near-term target for eProsima? Would it be safe to start working from this PR's branch?

@esteve Thanks in general for your efforts both here and in your efforts towards ROS2 support on Android!

@spiderkeys
Copy link
Contributor

@richiprosima @esteve

Also, out of curiosity, do you have available the methods you use to build FastRTPS for Android? Using @esteve's ros2_java README as a baseline, I was able to successfully build using the following commands, but they could probably use a lookover to see if I missed anything:

mkdir build
cd build

$HOME/Android/Sdk/cmake/3.6.3155560/bin/cmake \
-DCMAKE_TOOLCHAIN_FILE=$HOME/android/android-ndk-r14/build/cmake/android.toolchain.cmake \
-DANDROID_ABI=armeabi-v7a \
-DANDROID_NDK=$HOME/Android/Sdk/ndk-bundle \
-DCMAKE_BUILD_TYPE=Release \
-DANDROID_NATIVE_API_LEVEL=android-21 \
-DANDROID_PLATFORM=android-21 \
-DANDROID_FUNCTION_LEVEL_LINKING=OFF \
-DANDROID_TOOLCHAIN_NAME=arm-linux-androideabi-clang \
-DANDROID_STL=gnustl_shared \
-DTHIRDPARTY=ON \
-DCOMPILE_EXAMPLES=OFF \
-DCMAKE_CXX_FLAGS=-std=c++11 -frtti -fexceptions \
..

@richiprosima richiprosima merged commit 5301ef2 into eProsima:master Mar 7, 2017
@richiprosima richiprosima removed the in progress Issue or PR which is being reviewed label Mar 7, 2017
@esteve esteve deleted the cpp11-android branch March 7, 2017 09:35
@esteve
Copy link
Contributor Author

esteve commented Mar 7, 2017

@spiderkeys thanks for trying out this branch! That CMake incantation is perfect, the only thing I might change is that you don't have to pass any extra flags to the C++ compiler. Starting with NDK r14, RTTI and exceptions are enabled by default (see https://github.com/pombredanne/android.googlesource.com-platform-ndk/commit/9b3fff0a117ab7c49eb97c1bc441b4bb8c3464fc) Additionally, -std=c++11 is not necessary since it's already in FastRTPS' CMakeLists.txt

Also, according to the FastRTPS README, you should pass -DEPROSIMA_BUILD=ON, instead of -DTHIRDPARTY=ON, I need update the ros2_java docs.

Let me know if you find any other issue, I've been using this code in conjunction with ros2_java, but haven't really used it standalone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants