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

Auto-install cmake-format if the program is not found #34

Closed
wants to merge 3 commits into from

Conversation

threeal
Copy link

@threeal threeal commented Jun 28, 2023

This pull request introduces the capability to automatically install cmake-format using python -m pip if the program is not found on the system.

While it may be preferable to let users install their own cmake-format based on their preferences, this change proves useful in the context of GitHub Workflows. It simplifies the workflow by minimizing the need for explicit steps to install cmake-format, as the installation process is handled seamlessly by the script.

Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR! It definitely seems useful to have the format tools automatically installed. However, I see some issues with this approach as the configure script is now modifying the system's global state which is very unexpected.

A potential solution would be to install the cmake-format into a virtual environment, to not modify the global system, even though this can also be quite clunky.

CMakeLists.txt Outdated Show resolved Hide resolved
@threeal
Copy link
Author

threeal commented Oct 5, 2023

@TheLartians I updated the code so the cmakelang will be installed on a virtual environment with version 0.6.13 specified.

Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, this looks quite good! Some thoughts:

  • I think we still need to install pyyaml, otherwise it will fail if the config file is in yaml as the case in this repo.
  • At this point, we could vent auto install both cmake-format and clang-format through the venv pip install clang-format==14.0.6 cmake_format==0.6.11 pyyaml.
  • IMO it even makes sense to use the venv by default, even if clang/cmake-format is already installed on the system, as it prevents unexpected errors from version conflicts.
  • We could consider allowing users to override the installed versions.
  • To prevent unnecessary reinstallations of the venv, it might make sense to set the location of the virtual environment to the source directory of the Format.cmake project (and gitignore it). That way, it will be cached throughout different build directories when using a source cache with CPM.cmake.
  • Finally, as this may have quite the impact on the initial configuration time, how about lazily initialising the venv only when the format targets are called so it is only installed when actually needed?

@threeal
Copy link
Author

threeal commented Oct 9, 2023

@TheLartians nice suggestion, i'll work on that later when i have times.

@threeal threeal closed this Jan 12, 2024
@threeal threeal deleted the auto-install-cmake-format branch January 12, 2024 15:01
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.

None yet

2 participants