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

Replaced boost by c++11 and asio #59

Merged
merged 7 commits into from
Jan 13, 2017
Merged

Replaced boost by c++11 and asio #59

merged 7 commits into from
Jan 13, 2017

Conversation

chohner
Copy link
Contributor

@chohner chohner commented Nov 18, 2016

This completely strips Boost from the network interface:

  • Include asio.hpp and define ASIO_STANDALONE
  • boost > std: (enabled_shared_from_this, shared_ptr, bind,
    asio::placeholders, system::, thread)
  • boost::asio > asio (ip::tcp::socket, ip::tcp::acceptor,
    ip::tcp::endpoint, io_service, error_code, streambuf, async_write)
  • boost::asio::deadline_timer becomes asio::steady_timer (to work with
    std::chrono)
  • boost::posix_time > std::chrono

This should compile already after installing asio (brew install asio on OSX, apt-get install libasio-dev on unix).

Todo:

  • Adjust build environment (check against asio instead of boost)
  • Update readme on installing asio and enforcing c++11

- include asio.hpp and define ASIO_STANDALONE
- boost > std: (enabled_shared_from_this, shared_ptr, bind,
asio::placeholders, system::, thread)
- boost::asio > asio (ip::tcp::socket, ip::tcp::acceptor,
ip::tcp::endpoint, io_service, error_code, streambuf, async_write)
- boost::asio::deadline_timer becomes asio::steady_timer (to work with
std::chrono)
- boost::posix_time > std::chrono
@chris-hld
Copy link
Contributor

chris-hld commented Nov 18, 2016

Yep, compiles on Ubuntu 16.04.

@mgeier
Copy link
Member

mgeier commented Nov 18, 2016

Cool, that works great for me on Debian testing!

All the ugly Boost stuff in configure.ac can now be replaced with those few lines:

ENABLE_FORCED([ip-interface], [network (TCP/IP) interface],
[
  CPPFLAGS_backup="$CPPFLAGS"
  CPPFLAGS="$CPPFLAGS -DASIO_STANDALONE"
  AC_CHECK_HEADER([asio.hpp], , [have_ip_interface=no])
  CPPFLAGS="$CPPFLAGS_backup"
])

I think it makes sense to define ASIO_STANDALONE in the code where you did it, therefore it should be un-defined again after checking for asio.hpp.

Or probably it would be even better to do something like this (and removing the CPPFLAGS_backup lines above):

#if !defined(ASIO_STANDALONE)
#define ASIO_STANDALONE
#endif

@chohner chohner mentioned this pull request Nov 18, 2016
Copy link
Member

@mgeier mgeier left a comment

Choose a reason for hiding this comment

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

Cool stuff, I've just added a little comment regarding the docs.

@@ -1312,17 +1318,6 @@ This happens when two or more instances of the SSR are started with the IP serve
Start all (or at least all instances higher than 1) with the ``-I`` flag to disable the
IP interface.

IP interface isn't selected although boost libraries are installed
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Copy link
Member

Choose a reason for hiding this comment

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

Please don't remove this. Instead, add "This issue was resolved in ...".

Copy link
Member

Choose a reason for hiding this comment

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

Please add the appropriate SSR version number.

Copy link
Contributor Author

@chohner chohner Nov 23, 2016

Choose a reason for hiding this comment

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

What would be the correct version number? 0.4.3? Or would a larger version jump be more appropriate? Especially if this is combined with the upgrade of qt5.

Copy link
Member

Choose a reason for hiding this comment

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

I think 0.5.0, but @JensAhrens will make the release and he will decide if this PR will be included or not.

Copy link
Member

Choose a reason for hiding this comment

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

@chohner When you are done, can you please squash your commits?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, 0.5.0 is it!

Copy link
Contributor

Choose a reason for hiding this comment

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

... and I'm happy to include this PR.

@umlaeute
Copy link
Contributor

umlaeute commented Nov 19, 2016

maybe the use of ASIO_STANDALONE should be made settable by the person who is compiling (i don't have any special use-case in mind though).

something like --disable-asio-standalone to be added to configure if you don't want it.

@chohner
Copy link
Contributor Author

chohner commented Nov 19, 2016

maybe the use of ASIO_STANDALONE should be made settable by the person who is compiling (i don't have any special use-case in mind though).

That switch would then add boost dependencies deep inside the asio library that you would need to tell the compiler/linker about and so on. I don't think there is a good reason for it anyways (as you also mention).

@umlaeute
Copy link
Contributor

are these dependencies build-deps or runtime dependencies?

why does asio provide a non-standalone option (and even defaults to that) if it is not desirable to use it?

@chohner
Copy link
Contributor Author

chohner commented Nov 19, 2016

are these dependencies build-deps or runtime dependencies?

Build time.

why does asio provide a non-standalone option (and even defaults to that) if it is not desirable to use it?

I think only for historical reasons. The standalone option is still (comparatively) new and relies on c11. Since afaik there's plenty of c11 code throughout SSR, the only upside of building with boost (building with on std<c11) disappears. Additionally, some problems we were seeing on OS X also disappeared on this build so I don't think there's any reason to allow building with boost.

@chohner
Copy link
Contributor Author

chohner commented Nov 22, 2016

Small update from my side: I can confirm the interface to be working with the SSR android app.

@chohner chohner mentioned this pull request Nov 23, 2016
@chohner
Copy link
Contributor Author

chohner commented Nov 24, 2016

Added a line on being fixed in version 0.5.0 and clarified the second line. The squashing can/should be done on your part when merging (squash&merge is available directly in the github GUI).
Feel free to use something like this as the commit description:

* replaced boost::asio by c++11 and asio
* cleaned up configure.ac
* rename src/boostnetwork to src/network
* update manual

@mgeier
Copy link
Member

mgeier commented Nov 24, 2016

Thanks @chohner.

You squashing your commits (or somehow else pushing them into shape) places the burden of coming up with commit messages on you, that's all.

Anyway, @JensAhrens will merge it, so I don't really care.

@chris-hld
Copy link
Contributor

Tested in a compound version (https://github.com/chris-hld/ssr/tree/beta) and it works great for WFS.

@JensAhrens JensAhrens merged commit ddc5e5e into SoundScapeRenderer:master Jan 13, 2017
@chohner chohner deleted the strip_boost branch January 13, 2017 13:48
@mgeier
Copy link
Member

mgeier commented Jan 15, 2017

Apparently, I've approved this too quickly. Creating the manual produces a few errors now.

@JensAhrens
Copy link
Contributor

I fixed the manual.

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.

5 participants