Skip to content

Update repository to conform to Beman project standards #133

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dhollman
Copy link

@dhollman dhollman commented Apr 1, 2025

Summary

  • Aligns the repository with Beman project standards and recommendations
  • Implements all the required changes defined in the Beman standards document
  • Fixes potential issues with target passive requirements and feature test macros

Changes

Top-level file structure

  • Renamed LICENSE.txt to LICENSE
  • Updated README.md to use standard badge format
  • Added NOTICE file with proper attribution

CMake configuration

  • Updated CMake project name to beman.execution (was beman_execution)
  • Renamed options to follow BEMAN_execution_* format
  • Made targets passive by removing target_compile_features with PUBLIC visibility
  • Added proper feature detection at configure time

Feature test handling

  • Added config.hpp.in template for feature-dependent code
  • Added CMake checks for compiler features
  • Generated config.hpp at build time to replace direct usage of feature test macros

Test plan

  • Clean build with cmake --workflow --preset release
  • Run tests with ctest --test-dir build/default --output-on-failure

🤖 Generated with Claude Code

dhollman and others added 3 commits April 1, 2025 20:42
This commit makes the following changes to align with Beman project standards:

1. Top-level files
   - Renamed LICENSE.txt to LICENSE
   - Updated README.md to use standard badge format

2. CMake structure
   - Updated CMake project name to beman.execution
   - Updated option names to follow BEMAN_execution_* format
   - Updated target names to match Beman conventions
   - Made targets passive by checking for features at config time

3. Feature test handling
   - Added config.hpp.in template for feature-dependent code
   - Added proper CMake checks for compiler features
   - Generated config.hpp at build time

These changes ensure full compliance with all requirements
specified in the Beman standard documentation.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
CMakeLists.txt Outdated
Comment on lines 10 to 42
# Setup C++ feature tests
include(CheckCXXSourceCompiles)

# Check for __cpp_explicit_this_parameter
check_cxx_source_compiles("
struct S {
void f(this S& self) {}
};
int main() { S s; s.f(); }
" BEMAN_EXECUTION_HAS_EXPLICIT_THIS_PARAMETER)

# Check for __cpp_lib_unreachable
check_cxx_source_compiles("
#include <utility>
int main() { std::unreachable(); }
" BEMAN_EXECUTION_HAS_LIB_UNREACHABLE)

# Check for __cpp_lib_forward_like
check_cxx_source_compiles("
#include <utility>
int main() {
int x = 42;
auto&& r = std::forward_like<int&>(x);
return 0;
}
" BEMAN_EXECUTION_HAS_LIB_FORWARD_LIKE)

# Configure the config.hpp file
configure_file(
"${CMAKE_CURRENT_SOURCE_DIR}/include/beman/execution/detail/config/config.hpp.in"
"${CMAKE_CURRENT_BINARY_DIR}/include/beman/execution/detail/config/config.hpp"
@ONLY
)
Copy link
Author

Choose a reason for hiding this comment

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

@bretbrownjr doesn't like this whole section. Remove everything except the first line (the beman.execution change is good.

set(TARGETS_EXPORT_NAME ${PROJECT_NAME}-targets)
set(TARGET_SHORT_NAME execution)
set(TARGET_ALIAS ${TARGET_NAMESPACE}::${TARGET_SHORT_NAME})
set(TARGET_PACKAGE_NAME ${TARGET_NAME}Config)
Copy link
Author

Choose a reason for hiding this comment

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

I like the -config version better -@bretbrownjr

NOTICE Outdated
Comment on lines 1 to 7
beman.execution
Copyright (c) 2025 The Beman Authors

This product includes software developed by The Beman Authors
(https://github.com/bemanproject/beman).

This product is governed by the Apache License 2.0 WITH LLVM-exception.
Copy link
Author

Choose a reason for hiding this comment

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

Delete this whole file

@@ -0,0 +1,16 @@
// include/beman/execution/detail/config/config.hpp -*-C++-*-
Copy link
Author

Choose a reason for hiding this comment

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

I don't like config.hpp files. Remove this and everything related to it

target_link_libraries(${TARGET_NAME} PUBLIC $<BUILD_INTERFACE:${TARGET_NAME}_project_options>)
target_link_libraries(${TARGET_NAME} PUBLIC $<BUILD_INTERFACE:${TARGET_NAME}_project_warnings>)
endif()

# Add the binary directory to include paths so config.hpp can be found
Copy link
Author

Choose a reason for hiding this comment

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

can be removed

Comment on lines 204 to 215
# Check for C++26 support at configuration time instead of using target_compile_features
include(CheckCXXCompilerFlag)
check_cxx_compiler_flag("-std=c++26" COMPILER_SUPPORTS_CXX26)
check_cxx_compiler_flag("-std=c++23" COMPILER_SUPPORTS_CXX23)

if(COMPILER_SUPPORTS_CXX26)
message(STATUS "Using C++26 for ${TARGET_NAME}")
elseif(COMPILER_SUPPORTS_CXX23)
message(STATUS "Using C++23 for ${TARGET_NAME}")
else()
message(FATAL_ERROR "The compiler ${CMAKE_CXX_COMPILER} does not support C++23 or later. Please use a different compiler.")
endif()
Copy link
Author

Choose a reason for hiding this comment

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

Delete this (but deleting what was there was a good change; we just didn't need to add anything)

Comment on lines 228 to 233

# Install the generated config.hpp file
install(
FILES ${PROJECT_BINARY_DIR}/include/beman/execution/detail/config/config.hpp
DESTINATION include/beman/execution/detail/config
)
Copy link
Author

Choose a reason for hiding this comment

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

Delete this

This commit addresses review feedback from PR 133:

- Removed NOTICE file as requested
- Removed all config.hpp.in related code
- Removed feature detection code
- Left only the essential changes for Beman compliance:
  - Project name change to beman.execution
  - Option name changes to follow BEMAN_execution_* format
  - Removal of target_compile_features with PUBLIC visibility

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@@ -125,4 +124,4 @@ install(
)

set(CPACK_GENERATOR TGZ)
include(CPack)
include(CPack)
Copy link

Choose a reason for hiding this comment

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

[pre-commit] reported by reviewdog 🐶

Suggested change
include(CPack)
include(CPack)

# cmake-format: on

install(
EXPORT ${TARGETS_EXPORT_NAME}1
FILE ${TARGETS_EXPORT_NAME}.cmake
DESTINATION "${INSTALL_CONFIGDIR}"
NAMESPACE ${TARGET_NAMESPACE}::
)
)
Copy link

Choose a reason for hiding this comment

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

[pre-commit] reported by reviewdog 🐶

Suggested change
)
)

@ClausKlein ClausKlein self-assigned this Apr 2, 2025
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.

2 participants