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

switch from setuppy to pyprojecttoml #214

Merged
merged 31 commits into from
Jun 3, 2024

Conversation

sjev
Copy link
Contributor

@sjev sjev commented May 5, 2024

Closes #189

Changes:

  • switch from setup.py to pyproject.toml
  • clean up requirements list. there is only one dev extra list used for development and building.
  • add .devcontainer for reproduceable dev environment in VSCode

Notes

  • local build works with python -m build, however I'm not sure that automatic versioning with setuptools-scm works correctly.
  • 💥 ci script is broken (need to fix before merge). See comment further on.
  • config files like .pylint.rc could be consolidated to pyprojecct.toml , this is something to consider. Curent .pylint.rc is a bit long though.
  • in retrospect, I should have split this into two MRs, one for devcontainer and one for config.

@sjev
Copy link
Contributor Author

sjev commented May 6, 2024

💥 I've removed requirements.txt as these are now integrated in pyproject.toml. This however introduced an issue duiring build as https://github.com/adafruit/actions-ci-circuitpython-libs/blob/master/install.sh expects a requirements file.

Not sure what's the best way to resolve this, we could:

  1. change build.yml , but this would introduce a non-standard ci script.
  2. make install.sh in actions-ci robust for missing requirements. Maybe just show a warning. In any case, I don't want to toch scripts that are used in 200+ repos.

... found a workaround after some thinking -> using a dummy requirements.txt.

.devcontainer/Dockerfile Outdated Show resolved Hide resolved
pyproject.toml Outdated

dependencies = [
"semver~=3.0",
"build",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "build" a new dependency? I'm not familiar with that, but I noticed it wasn't listed in the old requirements.txt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

build is required for packaging. Should not be in normal deps. I'll fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

is it a new requirement? Was it required in the past? I'm not familiar with it.

I didn't see it listed in requirements or optional_requirements from before and the packaging was always working properly as far as I understand.

Is there some other specific change within this PR that makes this module now a requirement? Or was it always required and being installed in some other way before?

@dhalbert
Copy link
Contributor

dhalbert commented May 6, 2024

If you upgrade the version of pylint in .pre-commit-config.yaml to 3.something, that should fix the pre-commit problem, and you could then try 3.12.

Re setuptools or the lack of it: The recommended replacement for pkg_resources, included in setuptools, is importlib, as mentioned in #213 (comment). There are backports of part or all of importlib to versions of Python before 3.12. It would be nice to remove the setuptools dependency, though it may make sense to make that another PR.

@sjev
Copy link
Contributor Author

sjev commented May 6, 2024

... to make another PR

we definately should, this thing already started unraveling like a sweater with pyproject.toml on 3.12 ( pre-commit, ci etc).
I reverted back to 3.11 to avoid those. Otherwise this is going to become 'fix it all' MR.
BTW, regarding build, this is the fist time that I need to include it explicitly. In my other repos with similar setups python -m build would just work.

@FoamyGuy
Copy link
Contributor

FoamyGuy commented May 6, 2024

I think we will want to keep requirements.txt and optional_requirements.txt at least for now. I think there are some parts of the CI and other infrastructure that are expecting those to exist. They could eventually be updated to check pyproject.toml instead, but I don't know that we'd be ready to do that all at once, but it would be nice to keep consistent for the time being.

@FoamyGuy
Copy link
Contributor

FoamyGuy commented May 6, 2024

In our library repos they use an entry inside of pyproject.toml to make it check requirements.txt and optional_requirements.txt files https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/blob/69b6e97c93d94231a8e37b9dfee424bc1608e630/pyproject.toml#L45-L46

is it possible to do that same thing in this repo? that would make this consistent with the library repos.

@sjev
Copy link
Contributor Author

sjev commented May 6, 2024

🎉 I've managed to fix the ci by

  • creating a dummy requirements.txt.
  • adding pip install -e '.[dev]' to build.yml

Keeping requirements in pyproject.toml is more elegant imo.

@FoamyGuy
Copy link
Contributor

FoamyGuy commented May 6, 2024

Personally I am in favor of keeping the requirements.txt files how they were before.

It may be more elegant to hardcode them in pyproject.toml and then make this fake version of requirements.txt but it makes this repo different from all of the other ones that we develop and maintain.

Also if I am understanding correctly wouldn't having requirements.txt faked mean that running pip install -r requirements.txt would not result in getting the necessary libraries installed?

Maybe further down the line it could make sense to do a wholesale switch away from using requirements.txt if there are better solutions. But IMO for the time being the consistency of having this repo be the same as all the others is preferable.

@sjev
Copy link
Contributor Author

sjev commented May 6, 2024

It may be more elegant to hardcode them in pyproject.toml and then make this fake version of requirements.txt but it makes this repo different from all of the other ones that we develop and maintain.

You have a point there. I've changed PR accordingly.

@sjev
Copy link
Contributor Author

sjev commented May 6, 2024

Note that pip install .[dev] does not work now 😞 . Tried changing

optional-dependencies = {optional = {file = ["optional_requirements.txt"]}} to
optional-dependencies = {dev = {file = ["optional_requirements.txt"]}}

with no effect. Documentation says this fuctionality is BETA btw.

@FoamyGuy
Copy link
Contributor

We discussed the .devcontainer stuff a bit "in the weeds" during the meeting on 5/28. For now we are deciding not to include the .devcontainer and associated files into the repo. It's not something that we have much experience with so it would be a bit mroe difficult to maintain on an on-going basis if it were to be checked in to the repo.

If you'd still like to share it there are a few options for that:

  • keep a fork of this repo with a branch that includes the container
  • make a seperate "devcontainers" repo or similar and store it inside of there

If you do publish it in one of these ways let you could propose a link to that be added to the documentation, or if you're interested in writing out a bit more about what it is, how to use it, and how it makes development easier you could even write a Playground page in the Learn guide system.

If you'd like to just use it in your local instance and add it to your local .gitigore as well and then it'll stay in your local copy of the repo but not be checked in.

I'm intending to test this out and take a closer look at the pyproject.toml changes this weekend. If you see this message before that, and have the chance to add a commit that removes the .devcontainer you can do that. No worries if not though I can add a commit if the "allow maintainers to edit" check was enabled, or make a new branch from this and create a separate PR if not.

@sjev
Copy link
Contributor Author

sjev commented May 31, 2024

Sure, I've removed devcontainer for now. The devcontainer verision is on keep-devcontainer branch.
I'd be interested in writing a how-to on devcontainers, will put that on my todo list. Devcontainers are a great way to create consistent environments across machines.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

@sjev Thank you for working on this!

I think this is close, but will need a bit more work for the version and pypi deployment actions.

Currently when you install from this branch it installs as version 0.0.0. But when you installed from currently released branch it will fill in a version closer to the real one (but perhaps not exact, it tries to increment and find the 'next' version)

One possible solution for this seems to be adding:

[tool.setuptools_scm]

Into pyproject.toml. The documentation here: https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html#dynamic-metadata mentions about dynamic:

Currently the following fields can be listed as dynamic: version, classifiers, description, entry-points, scripts, gui-scripts and readme. When these fields are expected to be provided by setuptools a corresponding entry is required in the tool.setuptools.dynamic table [2].

There is some more info on setuptools-scm usage here: https://setuptools-scm.readthedocs.io/en/latest/usage/

I tested that locally and it does seem to allow the version to get filled in successfully. I'm not certain if that is the best solution or not.

The workflows files seem to still be making references to setup.py in a few spots. I'm wondering if those might cause trouble without further changes because it looks like they are checking for the existence of setup.py to control some of the actions tasks.

The workflows also still use python setup.py sdist here:
https://github.com/adafruit/circup/blob/main/.github/workflows/release.yml#L38C6-L38C30

Which is noted as deprecated here: https://packaging.python.org/en/latest/discussions/setup-py-deprecated/#is-setup-py-deprecated

So I think we'll want to find the non-deprecated new way to do that and update the workflows file.

Another thing I think worth considering at this time is what minimum version of Python we want to continue supporting. Right now this is listing 3.6, but that is EoL and no longer maintained version of Python, so I learn towards us dropping support for it in circup as well. I'll try to get input on this during the next meeting.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

We discussed minimum version support during the meeting and came to the consensus of support 3.9 as the minimum, though 3.8 does have security releases for a few more months, if we have user(s) pop up trying to use on 3.8 maybe we can lower it for a bit.

I have pushed commits to change the versions in these files, as well as bumped to a newer version of python inside the actions container for build actions. I've updated the release.yml action config as well to a version that should support pyproject.toml I believe. It was taken from the config used by the majority of circuitpython libraries, but tweaked slightly since the version automation works differently in this repo.

I think this is looking good at this point. I believe everything that I noted in prior comments is handled now.

Thanks again for working on this.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

Actually not quite ready yet. The new release action seems to be failing https://github.com/adafruit/circup/actions/runs/9357375764 also it's kind of strange that it's even running, I think I've done something wrong in it.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

Okay, I think the release action should be good now.

I think the one that I had used before was not intended to be used directly as an action but rather called in using the with syntax. I ended up copying the one from circuitpython-build-tools and tweaking a py version and the build command.

If this works I'll make a PR over on build-tools with the same tweaks.

@FoamyGuy FoamyGuy merged commit 0227af6 into adafruit:main Jun 3, 2024
1 check 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.

Clean up circup requirements; update base Python version; switch to pyproject.toml?
3 participants