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

Add support for <filesystem> to IO #636

Conversation

mloskot
Copy link
Member

@mloskot mloskot commented Feb 5, 2022

Description

If neither nor <experimental/filesystem> is detected,
then require <boost/filesystem.hpp>.

If user defines BOOST_GIL_IO_USE_BOOST_FILESYSTEM macro,
then <boost/filesystem.hpp> is pre-selected and required,
and search for any of the C++ standard implementation is skipped.

Remove end-user macro BOOST_GIL_IO_ADD_FS_PATH_SUPPORT
Require tests to always build with support of either
detected C++ filesystem or pre-selected Boost.Filesystem.

References

Tasklist

  • Add note to RELEASES.md changelog.
  • Ensure all CI builds pass
  • Review and approve

@mloskot mloskot added ext/io boost/gil/extension/io/ core/io boost/gil/io labels Feb 5, 2022
@mloskot mloskot added this to the Boost 1.78+ milestone Feb 5, 2022
@mloskot mloskot force-pushed the ml/add-std-filesystem-make-boost-filesystem-optional branch from 1f0d39d to e634bcf Compare February 5, 2022 16:02
@mloskot mloskot marked this pull request as draft February 5, 2022 16:02
If neither <filesystem> nor <experimental/filesystem> is detected,
then require <boost/filesystem.hpp>.

If user defines BOOST_GIL_IO_USE_BOOST_FILESYSTEM macro,
then <boost/filesystem.hpp> is pre-selected and required,
and search for any of the C++ standard implementation is skipped.

Remove end-user macro BOOST_GIL_IO_ADD_FS_PATH_SUPPORT
Require tests to always build with support of either
detected C++ filesystem or pre-selected Boost.Filesystem.

Closes #boostorg#222
@simmplecoder
Copy link
Contributor

The filesystem library requires separate flag for gcc 7, IIRC. It seems like the build flag will be on top of standard defines, right? So basically people can pass correct cmake flag into GIL's CML or use it from headers directly and deal with flags themselves. I believe doing it cmake only will make it much easier, but I guess since it is Boost it has do be done the most portable way.

@mloskot
Copy link
Member Author

mloskot commented Feb 9, 2022

@simmplecoder

It seems like the build flag will be on top of standard defines, right?

Yes, CMake option will accompany the C++ macro.
Ideally, if there was no such option and not even macro, but

  • if there is any of the standard filesystem, use it
  • otherwise, assume Boost.Filesystem is used
  • if none available, fail hard

since it is Boost it has do be done the most portable way.

Yes

@mloskot
Copy link
Member Author

mloskot commented Feb 22, 2022

@lpranam @simmplecoder @sdebionne and anyone interested, I'd like to push this forward, so following my earlier comment #636 (comment), what do you think about handling the filesystem libraries this way:

  1. We KISS and we offer no option for CMake or Boost.Build to enable/disable support for filesystem
  2. If there is any of the standard filesystem-s, use it.
  3. Otherwise, auto-assume Boost.Filesystem is used
  4. If none available, compilation will fail hard

Comments?

@meshtag
Copy link
Member

meshtag commented Feb 22, 2022

I liked this idea of dealing with file system libraries completely on our own without bothering/nudging the user(basically getting rid of ADD_FS_PATH_SUPPORT macro). Proposed BOOST_GIL_IO_USE_BOOST_FILESYSTEM macro seems good for providing control if the user absolutely needs it.

Regardless of the final decision in this matter, adding support for std::filesystem seems like a good idea to me.

@simmplecoder
Copy link
Contributor

Great idea on pushing this forward in a simple form. As long as we do not close doors to important modifications (build system integration), CI seems to take priority.

@mloskot
Copy link
Member Author

mloskot commented Feb 22, 2022

Thanks for the comments. I'm going to hit Merge and let's see what happens. This is part of the library that can be improved at later time, if necessary.

@mloskot mloskot marked this pull request as ready for review February 22, 2022 18:14
@mloskot mloskot merged commit f4c70a8 into boostorg:develop Feb 22, 2022
@mloskot mloskot deleted the ml/add-std-filesystem-make-boost-filesystem-optional branch February 22, 2022 18:18
@mloskot mloskot mentioned this pull request May 12, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core/io boost/gil/io ext/io boost/gil/extension/io/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants