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

Guard boost::filesystem usage #743

Merged
merged 1 commit into from
May 10, 2024

Conversation

simmplecoder
Copy link
Contributor

Description

CMake was unopinionated about what filesystem is used and required excessive flag setting. Now it clarifies that boost::filesystem is default and std::filesystem is used in case
BOOST_GIL_USE_BOOST_FILESYSTEM is off and CMAKE_CXX_STANDARD>=17.

Apparently we did not explicitly linked to boost libraries anyway. Either that, or I overlooked it.

References

#742

Tasklist

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

CMake was unopinionated about what filesystem is used and required
excessive flag setting. Now it clarifies that boost::filesystem
is default and std::filesystem is used in case
BOOST_GIL_USE_BOOST_FILESYSTEM is off and CMAKE_CXX_STANDARD>=17.
@mloskot
Copy link
Member

mloskot commented Apr 16, 2024

I think this is what we need, thank you!

@mloskot mloskot marked this pull request as ready for review May 10, 2024 07:35
@mloskot mloskot added the config/cmake Any issues about CMake configuration label May 10, 2024
@mloskot mloskot added this to the Boost 1.83+ milestone May 10, 2024
@mloskot mloskot added the cat/feature New feature or functionality label May 10, 2024
@mloskot mloskot merged commit 4fec872 into boostorg:develop May 10, 2024
18 checks passed
@striezel striezel mentioned this pull request Jun 30, 2024
6 tasks
@Osyotr
Copy link

Osyotr commented Sep 24, 2024

Boost::gil still unconditionally links to Boost::filesystem

Boost::filesystem

@mloskot
Copy link
Member

mloskot commented Sep 25, 2024

Thanks @Osyotr for reporting that.

I have not tested, but I think that line should read - if I still can remember CMake expressions

$<$<BOOL:${BOOST_GIL_USE_BOOST_FILESYSTEM}>:Boost::filesystem>

Additionally, this piece

gil/CMakeLists.txt

Lines 114 to 126 in e5f5bce

if (BOOST_GIL_USE_BOOST_FILESYSTEM)
set(filesystem_definition "BOOST_GIL_USE_BOOST_FILESYSTEM")
else()
set(filesystem_definition "BOOST_GIL_IO_USE_STD_FILESYSTEM")
endif()
target_compile_definitions(gil_compile_options
INTERFACE
$<$<CXX_COMPILER_ID:MSVC>:_CRT_NONSTDC_NO_DEPRECATE>
$<$<CXX_COMPILER_ID:MSVC>:_SCL_SECURE_NO_DEPRECATE>
$<$<CXX_COMPILER_ID:MSVC>:_CRT_SECURE_NO_WARNINGS>
$<$<CXX_COMPILER_ID:MSVC>:NOMINMAX>
$<$<CXX_COMPILER_ID:MSVC>:BOOST_CONFIG_SUPPRESS_OUTDATED_MESSAGE>
${filesystem_definition})

could be simplified to

target_compile_definitions(gil_compile_options
  INTERFACE
    $<$<CXX_COMPILER_ID:MSVC>:_CRT_NONSTDC_NO_DEPRECATE>
    $<$<CXX_COMPILER_ID:MSVC>:_SCL_SECURE_NO_DEPRECATE>
    $<$<CXX_COMPILER_ID:MSVC>:_CRT_SECURE_NO_WARNINGS>
    $<$<CXX_COMPILER_ID:MSVC>:NOMINMAX>
    $<$<CXX_COMPILER_ID:MSVC>:BOOST_CONFIG_SUPPRESS_OUTDATED_MESSAGE>
   $<$<BOOL:${BOOST_GIL_USE_BOOST_FILESYSTEM}>:BOOST_GIL_IO_USE_STD_FILESYSTEM>)

Am I correct?

@Osyotr
Copy link

Osyotr commented Sep 26, 2024

Honestly I'd prefer not to force a specific filesystem library implementation at build time because once cmake config is installed, it's impossible to switch to another implementation.
Effectively I propose reverting this PR and removing explicit linkage on Boost::filesystem from the public interface and keep it for tests.

@mloskot
Copy link
Member

mloskot commented Sep 27, 2024

@Osyotr I now see better what you mean. Although I don't mind reverting it, especially if it smoothens the vcpg integration, but I do need to point out at a couple of things related to our CMake configuration

  1. The (historic) status of our CMake:

    gil/CONTRIBUTING.md

    Lines 387 to 389 in e5f5bce

    **WARNING:** The CMake configuration is only provided for convenience
    of contributors. It does not export or install any targets, deploy
    config files or support subproject workflow.

  2. The fact that those deps where generated by Boost-wide tooling, so might be that not just GIL is affected by the deps annoyance:

    So, chances are the boost::filesystem will reappear.

  3. I'm not seeing how this PR here is causing the trouble. I think, it is more the change in PR Add a Boost-friendly subproject case to CMakeLists #614 that causes it, isn't it?

@Osyotr
Copy link

Osyotr commented Sep 27, 2024

Actually I think there's no problem. These options are meaningless in vcpkg because we take the "if" path:

gil/CMakeLists.txt

Lines 10 to 41 in 8994c2f

if(NOT CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
# Generated by `boostdep --cmake gil`
cmake_minimum_required(VERSION 3.8...3.23)
project(boost_gil VERSION "${BOOST_SUPERPROJECT_VERSION}" LANGUAGES CXX)
add_library(boost_gil INTERFACE)
add_library(Boost::gil ALIAS boost_gil)
target_include_directories(boost_gil INTERFACE include)
target_link_libraries(boost_gil
INTERFACE
Boost::assert
Boost::concept_check
Boost::config
Boost::container_hash
Boost::core
Boost::filesystem
Boost::integer
Boost::iterator
Boost::mp11
Boost::preprocessor
Boost::type_traits
Boost::variant2
)
target_compile_features(boost_gil INTERFACE cxx_std_14)
else()

I was confused by the lack of indentation inside if-else block and thought that these target_link_libraries calls are sequential.

@mloskot
Copy link
Member

mloskot commented Sep 27, 2024

@Osyotr No problem, thanks for your feedback. I'm happy to make package maintainers life easier. So, please feel free to report issues/suggest improvements. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/feature New feature or functionality config/cmake Any issues about CMake configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants