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

cmake, dependency management, and formatting project updates #90

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

vorlac
Copy link

@vorlac vorlac commented Jan 1, 2025

No description provided.

1. replaced the guts of the previous deps script with the vcpkg submodule and configs. the deps should now just download, unpack, and build during configuration
2. made some adjustments to cmakepresets.json - not sure if the old structure was favored for others or if the simpler inheritance and overall hierarchy is easier the way it is now (all use ninja so more can be shared)
3.. added NINJA_STATUS env variable (in cmakepresets) to output compilation times and a more detailed progress message
4. reformatted all cmake files so the indentation / whitespace / styling in general is consistent. tried to keep things as close to original style as possible
5. added cmake script that will automatically invoke vcpkg (and update/init the submodule if it hasn't been yet) for everything involving deps management to be automatic. vcpkg.json was also added so it can be used in maniest mode (locallized install in the submodule rather than using a global install)
6. added a clang-format scrtipt that will iterator over every source file and reformat it during configuration. this project has some big files so this should be improved by using `ninja -d explain` to detect which files have updated timestamps since previous configuration and only reformat those. a filter is included now, but mostly just as an example.
7. updated clang-format to try and avoid as many formatting changes as possible in the codebase. (those will come in the next commit)
8. added general cmake utils script to output all library variables worth showing as well as compiler / linker paths, version numbers, etc to make it easy to verify the state of configuration matches what is expected.
9. moved the c++ standard version and compile commands output option out of cmakepresets since those won't change and should probably stay consistent for all when building the library.
10. added a command to copy compile_commands.json from the binary dir into the source dir so IDEs have a better time using it for intellisense. on windows this requires symlink permissions (enable developer mode in windows 10/11 oe can also update your machine's group policy to give yourself permissions necessary for it. it only needs to be updated once, but even still, that probably means this is better off just being copied instead of symlinked.
…the build to fail with /W4 and /WX. all other modifications were caused by running clang-format on the codebase (include, tests, examples, benchmark). the formatting changes were mostly header file resorting/regrouping, whitespace, and collapsing shorter for loops onto a single line. tried to also prevent it from doing weird things around the enable_if's (the cossts added to the config)
@vorlac vorlac requested a review from Rinzii as a code owner January 1, 2025 02:20
Copy link
Owner

@Rinzii Rinzii left a comment

Choose a reason for hiding this comment

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

First, I'd like to thank you for taking the time to contribute to CCMath! Contributions mean the world to me and every little bit helps no matter how large or small.

I have some minor comments, but overall everything looks pretty good!

{
"$schema": "https://raw.githubusercontent.com/microsoft/vcpkg-tool/main/docs/vcpkg.schema.json",
"documentation": "https://github.com/microsoft/vcpkg/blob/master/docs/users/manifests.md",
"name": "roguelike",
Copy link
Owner

Choose a reason for hiding this comment

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

What does the name here mean? (Sorry I'm somewhat unfamiliar with vcpkg and its details)

Copy link
Author

Choose a reason for hiding this comment

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

whoops, yeah, roguelike is the only wrong thing in there. i just stole that config from one of my projects and forgot to update the name.

the $schema is just the json format schema so IDEs can give you basic intellisense/autocomplete/hints when changing it. you can look at it here, it just defines the field names, possible values, sometimes a brief description... https://raw.githubusercontent.com/microsoft/vcpkg-tool/main/docs/vcpkg.schema.json

the documentation link, it exactly that... just a markdown page that explains a bit about what that config file is and what "manifest mode" is (where it's essentially automated through cmake/IDEs so you only have to make a few edits to the config and cmake files to add a dependency).
https://github.com/microsoft/vcpkg/blob/master/docs/users/manifests.md

# Create the INTERFACE library
add_library(${PROJECT_NAME} INTERFACE)
add_library(ccmath::ext ALIAS ${PROJECT_NAME})

# Link GTest only if tests are enabled
if (CCMATH_BUILD_TESTS)
target_link_libraries(${PROJECT_NAME} INTERFACE gtest::gtest)
Copy link
Owner

Choose a reason for hiding this comment

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

I would like to keep a fallback option for users that don't want to or can't easily use vcpkg. Overall, I am supportive of using vcpkg but I'd like to still keep options available to developers. :)

Copy link
Owner

Choose a reason for hiding this comment

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

I'd also be nice if find_package fails it reverts to using fetchcontent, but that is a nice to have but not required.

Copy link
Author

Choose a reason for hiding this comment

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

just tossing this here so we don't forget about it, it's probably worth a try before getting hackier with things:
https://cmake.org/cmake/help/v3.22/variable/CMAKE_DISABLE_FIND_PACKAGE_PackageName.html

@Rinzii Rinzii added the enhancement New feature or request label Jan 5, 2025
@Rinzii Rinzii added this to the Road to v1.0.0 milestone Jan 5, 2025
@vorlac
Copy link
Author

vorlac commented Jan 5, 2025

just a heads up that i should have some time to look at these builds next weekend. are all of the build failures legit above @Rinzii?

@Rinzii
Copy link
Owner

Rinzii commented Jan 7, 2025

just a heads up that i should have some time to look at these builds next weekend. are all of the build failures legit above @Rinzii?

Don't worry about build errors for building the project itself. Since all you changed was formatting and cmake stuff I doubt there will be a regression in the code itself. For the time being though I'm in the process of pushing a major update so CI will likely not be building successfully for at least another few days to a few weeks depending on the difficulty of the update.

@Rinzii
Copy link
Owner

Rinzii commented Jan 13, 2025

Hey just wanted to check in and ask what is the status of this PR?

@vorlac
Copy link
Author

vorlac commented Jan 14, 2025

work stuff took over last weekend, i'll do what i can to take care of the rest of this sometime this weekend.

@Rinzii
Copy link
Owner

Rinzii commented Jan 15, 2025

work stuff took over last weekend, i'll do what i can to take care of the rest of this sometime this weekend.

Sounds good and no rush!

I also massively updated a lot of the dev branch, but everything should now compile in the CI now. This may change at some point in the next few days, but generally it appears everything is pretty stable. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants