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

fix: Automatic detection of <filesystem> header #684

Merged
merged 2 commits into from
Jun 25, 2022

Conversation

marco-langer
Copy link
Contributor

@marco-langer marco-langer commented Jun 9, 2022

Description

PR #636 has added detection for the <filesystem> header, but now examples using the IO extensions can't be compiled with C++11 or C++14 anymore. This PR fixes this issue.

References

Compiling one of the examples, e.g.

g++ x_gradient.cpp -std=c++11 -ljpeg

results in

In file included from ../../boost/libs/gil/include/boost/gil/io/path_spec.hpp:11,
                 from ../../boost/libs/gil/include/boost/gil/io/get_read_device.hpp:13,
                 from ../../boost/libs/gil/include/boost/gil/io/get_reader.hpp:11,
                 from ../../boost/libs/gil/include/boost/gil/extension/io/jpeg/read.hpp:18,
                 from ../../boost/libs/gil/include/boost/gil/extension/io/jpeg.hpp:11,
                 from ../../boost/libs/gil/example/x_gradient.cpp:9:
../../boost/libs/gil/include/boost/gil/io/detail/filesystem.hpp:55:29: error: ‘filesystem’ is not a namespace-name
   55 | namespace filesystem = std::filesystem;
....

Tasklist

  • Add test case(s)
  • Ensure all CI builds pass
  • Review and approve

@mloskot mloskot added the cat/annoyance Not a bug, not a feature, but something that should be improved label Jun 9, 2022
@mloskot mloskot added this to the Boost 1.80 milestone Jun 9, 2022
@mloskot
Copy link
Member

mloskot commented Jun 9, 2022

Nice, thanks!

@marco-langer
Copy link
Contributor Author

I don't understand why it wasn't caught by the tests, are we using BOOST_GIL_IO_USE_BOOST_FILESYSTEM in the test runners?

@mloskot
Copy link
Member

mloskot commented Jun 9, 2022

@marco-langer

Good question. I don't understand why tests did not catch it. I don't see BOOST_GIL_IO_USE_BOOST_FILESYSTEM defined anywhere.

The GHA basically run b2 test:

./b2 -j3 libs/$LIBRARY/test toolset=${{matrix.toolset}} cxxstd=${{matrix.cxxstd}} variant=debug,release

which only runs minimal IO test called simple_all_formats:

run simple_all_formats.cpp
: # args
: # input files
: # requirements
<library>/boost/filesystem//boost_filesystem
[ ac.check-library /libjpeg//libjpeg : <library>/libjpeg//libjpeg : <build>no ]
[ ac.check-library /zlib//zlib : <library>/zlib//zlib : <build>no ]
[ ac.check-library /libpng//libpng : <library>/libpng//libpng : <build>no ]
[ ac.check-library /libtiff//libtiff : <library>/libtiff//libtiff : <build>no ]

So, for C++11 and C++14 none of these #define-s should happen

#if !defined(BOOST_NO_CXX17_HDR_FILESYSTEM) || defined(__cpp_lib_filesystem)
#include <filesystem>
#define BOOST_GIL_IO_USE_STD_FILESYSTEM
#elif defined(__cpp_lib_experimental_filesystem)
#include <experimental/filesystem>
#define BOOST_GIL_IO_USE_STD_FILESYSTEM
#define BOOST_GIL_IO_USE_STD_EXPERIMENTAL_FILESYSTEM
#endif

and for simple_all_formats test build for C++11 and C++14, this case should happen:

#define BOOST_FILESYSTEM_VERSION 3
#include <boost/filesystem.hpp>
#define BOOST_GIL_IO_USE_BOOST_FILESYSTEM

Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

And the CI-s are green now

@mloskot mloskot changed the title fixed detection of std::filesystem header fix: Detection of std::filesystem header Jun 25, 2022
@mloskot mloskot merged commit b9e652d into boostorg:develop Jun 25, 2022
@mloskot mloskot changed the title fix: Detection of std::filesystem header fix: Automatic detection of <filesystem> header Jun 25, 2022
mloskot added a commit that referenced this pull request Jun 27, 2022
* develop:
  docs!: Announce plan to require C++17 after Boost 1.80 (#694)
  feat: Added apply_rasterizer() free function (#695)
  refactor: Ellipse rasterizer according to the comment at (#692)
  refactor: Deprecate apply_operation in favor of variant2::visit for any_image (#656)
  refactor: Replace deprecated libtiff v4.3 typedefs with C99 fixed-size integers (#685)
  fix: Automatic detection of <filesystem> header (#684)
  test: Add tiled TIFF test case to simple_all_formats
  test: Add tests for RGB to HSL (#691)
  refactor: Move RGB to HSL tests to color_convert_rgb.cpp
  refactor: Make with_tolerance reusable across other tests
  chore: Correct include guard
  fix: Add missing #include <array>
  fix: Wrong RGB -> HSL convertion (#505)
@mloskot mloskot mentioned this pull request Jul 5, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/annoyance Not a bug, not a feature, but something that should be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants