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

Correct CMake issues #68

Merged
merged 13 commits into from
Nov 13, 2023

Conversation

doodspav
Copy link
Contributor

@doodspav doodspav commented Nov 2, 2023

  • install properly without forcing us upon our consumers' consumers
  • remove unnecessary options (such as platform specific ones, or ones that have no impact on building/installing)
  • support alias targets helping consumers catch errors sooner
  • make CMake files more structured and readable
  • change minimum required version from 3.8 .. 3.23 to just 3.14
  • all symbols are now private (you should probably address this asap)
  • more stuff properly, i don't remember

@jeremy-rifkin
Copy link
Owner

Thank you for the PR! I will review as soon as I can.

@doodspav
Copy link
Contributor Author

doodspav commented Nov 2, 2023

Follow up things you should probably do:

  • check your flags; i removed a whole bunch because they don't affect the build (they're compiler warnings) and they belong in presets, not in the CML file
  • check your macro definitions; i removed some that set some macros that didn't affect build/install (they should stay removed, but you might want to update your docs concerning that)
  • fix the CMake for your tests, and maybe use a proper test framework
  • fix the CMake for your dependencies (like cpptrace) which gets installed along with libassert even when being linked as a private dependency
  • you now have a new header file, <assert/assert_export.hpp> that gets generated by CMake; you can include this without needing a relative include path, and you should mark all the symbols you want to be exported with ASSERT_EXPORT because right now all symbols are private (even on linux)
  • update documentation to specify that people should link against assert::assert instead of assert (which no longer exists)

…definitions

- until i can figure out a solution
@doodspav
Copy link
Contributor Author

doodspav commented Nov 2, 2023

Bear in mind that cpptrace's CMake breaks consuming an installed version of libassert consumed via find_package

@jeremy-rifkin
Copy link
Owner

jeremy-rifkin commented Nov 9, 2023

Thank you again for this PR, I have integrated much of your recommendations for structure and functionality of the CMake into cpptrace on the dev branch and things seem to work well there. I will give this PR a more thorough review this weekend.

Running the CI now, some test failures are expected. No worries that there are some.

@jeremy-rifkin jeremy-rifkin changed the base branch from main to dev November 13, 2023 04:44
Copy link
Owner

@jeremy-rifkin jeremy-rifkin left a comment

Choose a reason for hiding this comment

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

I think this looks really good overall. I commented on some super minor things I noticed, I can take care of them after I merge.

Comment on lines -64 to -69
target_compile_options(
assert
PRIVATE
$<$<NOT:$<CXX_COMPILER_ID:MSVC>>:-Wall -Wextra -Werror=return-type -Wshadow -Wundef>
$<$<CXX_COMPILER_ID:GNU>:-Wuseless-cast -Wnonnull-compare>
$<$<CXX_COMPILER_ID:MSVC>:/W4 /WX /permissive->
Copy link
Owner

Choose a reason for hiding this comment

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

I am likely going to re-add these warnings, they're important for development

CMakeLists.txt Show resolved Hide resolved
add_executable(basic tests/basic/basic_test.cpp)
target_link_libraries(basic PRIVATE libassert-lib)
# endif()
endif()
Copy link
Owner

Choose a reason for hiding this comment

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

A newline at the end of this file would be good

@jeremy-rifkin jeremy-rifkin merged commit 1668331 into jeremy-rifkin:dev Nov 13, 2023
25 of 32 checks passed
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