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 distutils deprecation for Python 3.12 #141

Merged
merged 4 commits into from
Aug 14, 2024

Conversation

OCopping
Copy link
Contributor

@OCopping OCopping commented Apr 16, 2024

This PR aims to make changes that will allow for use in Python 3.12 programs.

As well as fixes to the code for p4p itself, this PR includes:

  • Dependency version updates (pinning to versions based on Python <= 3.11 or >= 3.12)
  • Numpy 2.0 support
  • Fixed actions builds (with updated runner images)

@OCopping
Copy link
Contributor Author

Waiting on epics-base/setuptools_dso#31

@OCopping
Copy link
Contributor Author

OCopping commented Jul 5, 2024

When building this and running tests with local copies of setuptools_dso, epicscorelibs and pvxslibs, I noticed that there is a circular import error when attempting to run the nose2 tests.

❯ python -m nose2 p4p                                21s  p4p  14:12:16
E
======================================================================
ERROR: p4p (nose2.loader.LoadTestsFailure)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/scratch/controls/dev/p4p/src/p4p/__init__.py", line 14, in <module>
    from .wrapper import Value, Type
  File "/scratch/controls/dev/p4p/src/p4p/wrapper.py", line 5, in <module>
    from . import _p4p
ImportError: cannot import name '_p4p' from partially initialized module 'p4p' (most likely due to a circular import) (/scratch/controls/dev/p4p/src/p4p/__init__.py)

----------------------------------------------------------------------
Ran 1 test in 0.000s

FAILED (errors=1)

The seems to occur no matter the Python version (at least >=3.8) .

@mdavidsaver
Copy link
Member

ImportError: cannot import name '_p4p' from partially initialized module 'p4p' (most likely due to a circular import) (/scratch/controls/dev/p4p/src/p4p/init.py)

I recall errors like this when accidentally running nose from within a partially built git clone. eg. if $PWD/p4p exists. This is why some of the GHA jobs will cd dist before running nose.

https://github.com/mdavidsaver/p4p/blob/70b030de2f30cf690bb317429f59bd092e65420e/.github/workflows/build.yml#L357-L359

@mdavidsaver
Copy link
Member

Looking again at this PR. Is it necessary to replace all of the quotes?

I am not opposed to code "beautification", but would much prefer these changes be separated. At least in separate commits, preferably separate PRs.

@OCopping
Copy link
Contributor Author

OCopping commented Jul 8, 2024

I recall errors like this when accidentally running nose from within a partially built git clone.

Following the Python Build commands listed in p4p/.github/workflows/build.yml seems to have fixed this issue.
The ones:

          python setup.py sdist --formats=gztar

and

          python -m pip wheel -v -w dist --no-index --no-deps \
           --no-build-isolation --no-use-pep517 \
           --build-option --plat-name=${{ matrix.piparch }} \
           $PRE \
           dist/*.tar.*

          cd dist
          python -m pip install p4p*.whl
          python -m ci_core_dumper exec python -m nose2 -v p4p

or anything similar are not mentioned in the docs for "Building" at all. It might be worth adding these so that anyone else trying to build from source doesn't get confused if these errors show when trying to run the unittests. Unless of course I missed an obvious step in the guide...

@OCopping
Copy link
Contributor Author

OCopping commented Jul 8, 2024

Looking again at this PR. Is it necessary to replace all of the quotes?

Oh whoops, that must have been my vscode auto-formatter and I didn't realise... I can change them back

@mdavidsaver
Copy link
Member

I recall errors like this when accidentally running nose from within a partially built git clone.

Forgive me for belaboring this point. Did switching to a different directory avoid this failure?

... not mentioned in the docs ...

These docs do not cover building wheels from a manually created git checkout. Only the pip --no-binary ... recipe to build from downloaded source. This was a deliberate decision on my part back in ~2018. I thought the process was too complex for reasonable "user" documentation. For that matter, I was having trouble getting pip to do what I wanted. This situation has evolved since then. Maybe there is now a sensible recipe to document? Does pip have a command for generating source tars?

@OCopping OCopping force-pushed the fix-distutils-deprecation branch 4 times, most recently from f54d8b7 to 443bdb5 Compare July 9, 2024 08:03
@tacaswell
Copy link

Does pip have a command for generating source tars?

That comes from the build system not pip. It looks like you are using setuptools so I expect python setup.py sdist to do what you want.

@OCopping OCopping marked this pull request as ready for review August 8, 2024 15:12
Copy link
Contributor

@AlexanderWells-diamond AlexanderWells-diamond left a comment

Choose a reason for hiding this comment

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

The fundamental changes look good to me, but there's probably some autoformat stuff that should be removed from this PR.

It should be highlighted that, due to the MacOS change from Intel to Arm64 runners, all cothread-dependant tests can no longer be run there (due to this cothread bug).

.github/workflows/build.yml Outdated Show resolved Hide resolved
example/dynamicbox_server.py Outdated Show resolved Hide resolved
example/dynamicbox_server.py Outdated Show resolved Hide resolved
makehelper.py Outdated Show resolved Hide resolved
src/p4p/test/test_value.py Outdated Show resolved Hide resolved
@OCopping OCopping force-pushed the fix-distutils-deprecation branch 3 times, most recently from 45c5661 to bdca08b Compare August 12, 2024 14:36
@OCopping
Copy link
Contributor Author

@mdavidsaver are you happy with these changes?

Oliver Copping and others added 4 commits August 14, 2024 11:49
Minor formatting changes

Add PyArrayObject cast to some lines
Add Python 3.5 build fix for TLS deprecation

Bump setup-python version to v5

Fix MacOS builds (now arm64)

Remove SETUPTOOLS_USE_DISTUTILS export as deprecated in Python 3.12
@OCopping OCopping merged commit 6355223 into epics-base:master Aug 14, 2024
36 checks passed
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.

4 participants