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

Replace POSIX directory operations by Boost Filesystem #1090

Closed
wants to merge 1 commit into from

Conversation

gjasny
Copy link
Contributor

@gjasny gjasny commented Apr 23, 2016

To support compiling the generate_geocoding_data generator on non-POSIX systems like Windows I replaced the POSIX code with Boost Filesystem. After the change the generator produes exactly the same output as before. This should close Issue #824.


This change is Reviewable

@andyst
Copy link
Collaborator

andyst commented Apr 27, 2016

@hagbard @roubert for added C++ and Windows (kidding) expertise.

@@ -82,14 +82,14 @@ if (${USE_ALTERNATE_FORMATS} STREQUAL "ON")
endif ()

# Find all the required libraries and programs.
if (${USE_BOOST} STREQUAL "ON")
if (${USE_BOOST} STREQUAL "ON" OR ${BUILD_GEOCODER} STREQUAL "ON")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't look right to me.
We can't replace "USE_BOOST" check with "USE_BOOST OR BUILD_GEOCODER" when the check is guarding includes for Boost headers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I take this back. In this context this is doing the right thing. My initial reading was wrong.

@andyst
Copy link
Collaborator

andyst commented Apr 27, 2016

Still need to review the rest of the code. Just added an initial comment.

@andyst
Copy link
Collaborator

andyst commented Aug 19, 2016

@roubert could you please take a look at this CL in terms of C++?
Looks good to me but maybe you spot something.

@@ -31,6 +30,7 @@
#include <string>
#include <utility>
#include <vector>
#include <boost/filesystem.hpp>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code needs to continue to work without Boost.

Please follow the pattern in https://github.com/googlei18n/libphonenumber/blob/master/cpp/src/phonenumbers/base/memory/scoped_ptr.h to use Boost "#if defined(I18N_PHONENUMBERS_USE_BOOST)" and fall back to the current code based on dirent.h.

@ginggs
Copy link

ginggs commented Sep 8, 2016

line 58 of tools/cpp/CMakeLists.txt
should be:

target_link_libraries (generate_geocoding_data_test ${TEST_LIBS} ${Boost_LIBRARIES})

instead of:

target_link_libraries (generate_geocoding_data_test ${TEST_LIBS})

so that generate_geocoding_data_test also gets linked with Boost

@keghani
Copy link
Contributor

keghani commented Feb 16, 2017

Hi @gjasny,

Are you interested in / able to help us get this PR merged in the foreseeable future?
Sorry Windows hasn't seen much love lately, but we don't actively maintain Windows builds and couldn't test such a PR.
If someone else in the open-source community (@ginggs? Thanks for helping review too!) could test it out and help with the review we'd accept it.

Related issues in case anyone could help collaborate there:

  • #1555, to allow Windows to build cpp library with pthreads for multi-threading (would this still be an issue after this PR?)
  • #1307, to use readdir instead of readdir_r

@keghani
Copy link
Contributor

keghani commented Mar 13, 2017

This is infeasible given the lack of reviewers or a response.

We don't actively maintain windows builds and we'd need someone else in the open-source community to test and help review this PR. Please see the known Windows issues, maybe someone else there or in the
discussion group could help.

@keghani keghani closed this Mar 13, 2017
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