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

Fix gtest CMake target: do not add 'd' postfix on debug builds #1022

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

ctrl-z-9000-times
Copy link
Collaborator

I had to rename the gtest library target in order to get the project to build cleanly.
I don't know when or why gtest changed their cmake config, but I think we need to update it as well?

I only tested this on linux.
@dkeeney, I would greatly appreciate it if you could review this change, and also test it on windows?

@dkeeney
Copy link

dkeeney commented Sep 4, 2024

Hmmm, we seem to have a problem here.
I am building with python 3.12.4 and is seems that distutils has been removed.
The below log is during the Windows build at the point where it is putting together the python dependencies.

========
Running from numpy source directory.
C:\Users\Dave\AppData\Local\Temp\easy_install-2xqry4y_\numpy-1.23.0\setup.py:86: DeprecationWarning:

numpy.distutils is deprecated since NumPy 1.23.0, as a result
of the deprecation of distutils itself. It will be removed for
Python >= 3.12. For older Python versions it will remain present.
It is recommended to use setuptools \< 60.0 for those Python versions.
For more details, see:
https://numpy.org/devdocs/reference/distutils_status_migration.html

import numpy.distutils.command.sdist
error: Cython needs to be installed in Python as a module

=============

It dies at this point. So it seems there is more that needs to be done.

@dkeeney
Copy link

dkeeney commented Sep 4, 2024

Tried python 3.11 and set setuptools < 60.0 in requirements.txt
This worked. Everything compiled (with lots of warnings about depreciation) but unit tests ran successfully.
Tried python 3.12 and it did NOT work.

REF: https://numpy.org/devdocs/reference/distutils_status_migration.html
https://scikit-build-core.readthedocs.io/en/latest/

I do not like to cap the version of any component and certainly not Python. So, the question is, should we take the effort to migrate the python portion of the build from setuptools to Scikit-build-core? @ctrl-z-9000-times what is your feeling on this.

In either case, this problem was not caused by this PR. The fix for setuptools should be a separate PR.
I will go ahead and approve and merge this PR.

@ctrl-z-9000-times
Copy link
Collaborator Author

Thanks for reviewing and merging these PRs.

I agree that we should not cap the supported python version, which means that we will need to fix this issue at some point.

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