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

Use system CMake if available #591

Merged
merged 2 commits into from
Feb 8, 2025

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Feb 6, 2025

Instead of depending on cmake and ninja executables unconditionally, add these dependencies only if system executables are not available. This ensures that system CMake is used when available (instead of being overridden by the PyPI build of CMake), so the user can benefit from improved portability due to downstream patching and avoids unnecessary dependencies.

This also adjusts the generator choice to support system ninja install, i.e. one without a ninja module.

Instead of depending on `cmake` and `ninja` executables unconditionally,
add these dependencies only if system executables are not available.
This ensures that system CMake is used when available (instead of being
overridden by the PyPI build of CMake), so the user can benefit from
improved portability due to downstream patching and avoids unnecessary
dependencies.

This also adjusts the generator choice to support system `ninja`
install, i.e. one without a `ninja` module.
@pseudo-rnd-thoughts
Copy link
Member

Thanks for the PR @mgorny
I suspect the issue with windows is related to

- name: Install windows cmake

Any ideas?

@mgorny
Copy link
Contributor Author

mgorny commented Feb 7, 2025

Hmm, that's an interesting problem. FWICS that implies that no entry points installed via pip install --user can work when you're inside another virtualenv. My first idea would be to try removing the workaround and see if it works.

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

Thanks @mgorny, the changes look good

@pseudo-rnd-thoughts pseudo-rnd-thoughts merged commit e57455a into Farama-Foundation:master Feb 8, 2025
28 checks passed
@mgorny mgorny deleted the system-cmake branch February 8, 2025 09:34
@mgorny
Copy link
Contributor Author

mgorny commented Feb 8, 2025

Thanks!

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