qPCR4vir / nana-demo

demos and tests for Nana C++ GUI library
https://github.com/cnjinhao/nana
Boost Software License 1.0
68 stars 24 forks source link

fix compilation with boost::filesystem #13

Closed pavelxdd closed 6 years ago

pavelxdd commented 6 years ago

see cnjinhao/nana#281 for related changes.

Travis build succeeded with this PR and changes to nana lib applied: https://travis-ci.org/pavelxdd/nana/builds/308570743

CMakeLists.txt:

CMake exported definitions with wrong name prefixes when compiling with Boost Filesystem, and nana used internal implementation of filesystem

Added boost system and thread libs to find_package, this comment from cmake documentation doesn't seem to be always true for all versions of CMake and Boost, also Travis build failed with the message that boost_system wasn't specified in linker flags, and threading demo failed due to missing linking to boost_thread lib. Chrono is needed for thread_pool demo.

demo programs:

boost::filesystem::path doesn't have a generic_u8string method, so changed it to nana::to_utf8(generic_wstring) in this demo.

boost::filesystem::directory_iterator doesn't have begin and end methods, so changed a for-loop to make it work.

pavelxdd commented 6 years ago

Updated.

Fixed crashes on accessing some system paths: Boost can throw an exception when accessing some system paths, like "C:\Config.Msi", so moved directory_iterator constructors to try-catch blocks.

qPCR4vir commented 6 years ago

Thank you! This is great.

But I'm having an small philosophical problem here, which I have been trying to solve. One can use Boost exclusively as an implementation for features that a modern c++ std:: have to provide but that some version/implementation may not have. In this case, Boost is just an implementation detail that we do not want to "leak". One could also decide to use Boost just because in the user code it is preferred for some reason. This is not the case that Nana supports, and as such it is the responsibility of the user to implement it correctly. One can expect such a user to know how he has to do that.

The fact that the demos have had to be modified indicates a failure in this. This is not a terrible problem and could be accepted. But it would be great if we found a way to reduce the changes, or at least isolate them so that it is very clear what is dependent on Boost, which otherwise is optional.

(related to cnjinhao/nana#281 )

pavelxdd commented 6 years ago

Since this repo is used as a testing suite for nana travis builds, I had to make the same code work on all three filesystem implementations in the same way, the huge part of this decision is also the fact, that nana provides a single API for all three filesystem backends.

The thing you're suggesting about isolating different implementations would be fine if this was just a demos repo, not a testing suite, and also it will probably requite some changes in the nana library.

Do you have specific suggestions for this PR?

pavelxdd commented 6 years ago

I'd say if the demos were implemented in a more universal way, instead of relying on a std::filesystem style of coding, the changes won't be necessary at all. Like, for example, the thing with for loops, many demos were already implemented properly via for (fs::directory_iterator i(path); i != fs::directory_iterator(); ++i) while the ones that were using std way: for (const auto& i: fs::directory_iterator(path)) had to be changed.

qPCR4vir commented 6 years ago

Hi, yes using nana-demo as both demo and test bring some problems.... but is the best form to enforce that demos are in sinc... And what is in this repo go automatically to the documentation too. We had too much problems with deprecated examples.

Do you have specific suggestions for this PR?

https://github.com/pavelxdd/nana/pull/1 I'm very sorry, I can't test it now.

pavelxdd commented 6 years ago

Hey, I've checked your commits out. Again, my point is, instead of making boost imitate the std and nana filesystems, just because the demos are written using only std- and nana- specific functions isn't going to end well, since aside from these u8string and directory_iterator differences, I'm sure there are going to be much more of them, and we will have to add more and more workarounds in the future. Rewriting the demos to be more universal is better.

Another simple solution is just drop the boost support completely, but it's not up to me to decide.

qPCR4vir commented 6 years ago

Yes, you are rigth. But it is not OK to drop a test that is not working. Now, thank to you it works !! ... and it is a good time to anunce that we are drooping boost / filesystem and gcc 4.9. Once again: thank you! And if you want to contribute in this direction you may consider to add suport to gcc>7 and clang in travis. It will not be deprecated for a long time.

Examples: travis CI with clang 5 and gcc 7 in Linux and OSX and including Boost. A book: how to use Travis CI with C++