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: Enable selection of a specific python impl #787

Merged
merged 2 commits into from
Dec 22, 2022

Conversation

Kangie
Copy link
Contributor

@Kangie Kangie commented Nov 30, 2022

On distros with multiple python impls it can be useful to select a specific version rather than whatever CMake thinks is appopriate.

This patch enables users to instruct CMake to look for a specific version of python by passing -DPYTHON_FIND_VER.

@micahsnyder
Copy link
Contributor

Thanks for submitting this contribution, @Kangie
I will test it momentarily and get back to you on how that works.

Can you please find a place to document the new cmake option in the INSTALL.md file?
Perhaps somewhere in this section https://github.com/Cisco-Talos/clamav/blob/main/INSTALL.md?plain=1#L356
And perhaps also where it says that Python is a requirement, to give some explanation how Python is found and reference the option.

@Kangie
Copy link
Contributor Author

Kangie commented Dec 6, 2022

Can you please find a place to document the new cmake option in the INSTALL.md file? Perhaps somewhere in this section https://github.com/Cisco-Talos/clamav/blob/main/INSTALL.md?plain=1#L356 And perhaps also where it says that Python is a requirement, to give some explanation how Python is found and reference the option.

Will do, I'm travelling at the moment so expect an update in a couple of days. I figured there would be a docs update required, at least there's an opportunity to address any feedback too. 😄

There are "better" ways to do this (e.g. setting the magic python3 executable var) but they're incompatible with calling pytest directly, which works in the vast majority of cases; I didn't want to change up the existing logic and remove that, so we got a new variable.

@micahsnyder
Copy link
Contributor

Will do, I'm travelling at the moment so expect an update in a couple of days.

Sounds good. Just leave a comment when you have updated it and are ready for a re-review. I will also be traveling for about a week. I will respond when I'm back.

On distros with multiple python impls it can be useful to select
a specific version rather than whatever CMake thinks is appopriate.

This patch enables users to instruct CMake to look for a specific
version of python by passing `-DPYTHON_FIND_VER`.
- Reference `PYTHON_FIND_VER`
- Add generic instructions for selecting specific implementations
  of build-time tools (LLVM, Python)
@Kangie
Copy link
Contributor Author

Kangie commented Dec 11, 2022

@micahsnyder I think that this should cover it. The addition under 'Build Tools' is generic enough to cover LLVM, Python and anything else that comes up in the future.

If we need more detail on the specifics I can add a link to the CMake docs for 'find_package' however I think that this is fine.

Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

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

Thanks @Kangie looks great! Sorry about the delay.
Happy holidays!

@micahsnyder micahsnyder merged commit ebe59ef into Cisco-Talos:main Dec 22, 2022
@Kangie Kangie deleted the select-python-impl branch February 16, 2023 23:15
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