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

addresses #103 #105

Merged
merged 3 commits into from
Dec 17, 2023
Merged

addresses #103 #105

merged 3 commits into from
Dec 17, 2023

Conversation

2bndy5
Copy link
Contributor

@2bndy5 2bndy5 commented Dec 16, 2023

This is not formatted. I couldn't see anything about contributing guidelines other than code of conduct. Is black used for this project? It doesn't seem like it.

I did run this locally (on Windows with python v3.11.5) and everything seemed to work as expected. However, the newer importlib_metadata.version() returned a dev spec that wasn't PEP440 compatible; I'm not sure that really matters since I think only stable version specs are used for uploads to pypi.

tested with a single module package (circuitpython-cirque-pinnacle) and a multi-module package (circuitpython-nrf24l01)

other chores

  • removed an unused import
  • updated actions/checkout to v4 in workflows
  • updated actions/setup-python to v5 in workflows

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks, this is a good start. I have some feedback.

Comment on lines +41 to +45
if sys.version_info < (3, 8):
import importlib_metadata
else:
import importlib.metadata as importlib_metadata

Copy link
Member

Choose a reason for hiding this comment

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

yikes. maybe we should start by seeing if we can move relevant github actions workflows to newer python. For instance, the circuitpython bundle uses 3.10 presently: https://github.com/adafruit/Adafruit_CircuitPython_Bundle/blob/main/.github/workflows/build.yml#L24 and newly cookie-cut libs use 3.11 https://github.com/adafruit/workflows-circuitpython-libs/blob/main/build/action.yml#L11

Copy link
Contributor Author

@2bndy5 2bndy5 Dec 17, 2023

Choose a reason for hiding this comment

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

Since python v3.7 is deprecated, we could bump the minimum python version to 3.8 and the changes to requirements.txt and corresponding import here would get simplified.

Not sure what the motivation is behind bumping the minimum supported python version. Is it meant to keep the quantity of non-std deps low? Were you hoping for less complex imports?

Copy link
Member

Choose a reason for hiding this comment

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

yeah specifically I was reacting to needing to add this version dependent code. I don't know why the workflows here are at 3.7 while we have 3.10 and 3.11 where it's actually being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if you upgrade the version used in workflows, the pkg supported version is still set to

python_requires='>=3.7',

Again, python 3.7 has reached end-of-life in 2023-06-27 (see PEP537).

circuitpython_build_tools/scripts/build_bundles.py Outdated Show resolved Hide resolved
@jepler
Copy link
Member

jepler commented Dec 17, 2023

If at any point this starts to feel like too much of a moving target, feel free to say so and we can get some element of it merged because that's better than fixing anything (and part of why I split off the README into a separate issue)

@2bndy5
Copy link
Contributor Author

2bndy5 commented Dec 17, 2023

If at any point this starts to feel like too much of a moving target, feel free to say so and we can get some element of it merged

I hadn't considered the administrative side of this (upgrading minimum supported python, etc). My main focus was to just fix support for python v3.12 (& ensure backward compatible behavior).

As you pointed out, the newer CI (composite) workflows are pinned to using a python v3.11, so there's probably no rush on this from the average end-user perspective. It would be good to prevent upgrading the composite workflows' python pinned version (for now).

Comment on lines +41 to +45
if sys.version_info < (3, 8):
import importlib_metadata
else:
import importlib.metadata as importlib_metadata

Copy link
Member

Choose a reason for hiding this comment

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

yeah specifically I was reacting to needing to add this version dependent code. I don't know why the workflows here are at 3.7 while we have 3.10 and 3.11 where it's actually being used.

@jepler jepler merged commit b49cd84 into adafruit:main Dec 17, 2023
1 check passed
@2bndy5 2bndy5 deleted the replace-pkg_resources branch December 17, 2023 14:09
@jepler
Copy link
Member

jepler commented Dec 17, 2023

thank you

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