circup icon indicating copy to clipboard operation
circup copied to clipboard

switch from setuppy to pyprojecttoml

Open sjev opened this issue 1 year ago • 9 comments

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.
  • :boom: ~~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 avatar May 05 '24 22:05 sjev

:boom: 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.

sjev avatar May 06 '24 08:05 sjev

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 https://github.com/adafruit/circup/issues/213#issue-2272872886. 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.

dhalbert avatar May 06 '24 13:05 dhalbert

... 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.

sjev avatar May 06 '24 13:05 sjev

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 avatar May 06 '24 14:05 FoamyGuy

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.

FoamyGuy avatar May 06 '24 14:05 FoamyGuy

:tada: 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.

sjev avatar May 06 '24 15:05 sjev

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.

FoamyGuy avatar May 06 '24 16:05 FoamyGuy

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 avatar May 06 '24 17:05 sjev

Note that pip install .[dev] does not work now :disappointed: . 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.

sjev avatar May 06 '24 17:05 sjev

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.

FoamyGuy avatar May 31 '24 14:05 FoamyGuy

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.

sjev avatar May 31 '24 18:05 sjev