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

migrate packaging to pyproject.toml #9056

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

deronnax
Copy link
Contributor

@deronnax deronnax commented Jul 27, 2023

Just an initiative of my own.

Don't forget to squash merge the PR.

@deronnax deronnax marked this pull request as draft July 27, 2023 13:40
@deronnax deronnax force-pushed the migrate_setuppy_to_pryoject.toml branch from 6344069 to 42d468b Compare July 27, 2023 13:41
@deronnax
Copy link
Contributor Author

deronnax commented Jul 29, 2023

OK, it's almost ready except:

  • It can't work on python 3.6 because having setuptools being able to read metadata from pyproject.toml requires setuptools > 61.2, which does not support python 3.6 anymore.
  • I need to figure out what is conftest.py doing with the package_root

@deronnax deronnax force-pushed the migrate_setuppy_to_pryoject.toml branch from 8c65e6d to 6e67258 Compare July 29, 2023 17:07
@terencehonles
Copy link
Contributor

  • I need to figure out what is conftest.py doing with the package_root

This is unrelated to your PR and I fixed it with #9129

@deronnax deronnax force-pushed the migrate_setuppy_to_pryoject.toml branch from 6e67258 to b16826c Compare October 5, 2023 16:28
@deronnax deronnax marked this pull request as ready for review October 5, 2023 22:48
@deronnax
Copy link
Contributor Author

deronnax commented Oct 5, 2023

Wow. It works indeed. Thank you so much.

@auvipy auvipy self-requested a review October 6, 2023 05:42
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

We need to hold this off for now. As we got other priorities.

@deronnax
Copy link
Contributor Author

deronnax commented Oct 6, 2023

Sure. A vague estimation of when it would become envisageable ?

@deronnax deronnax changed the title migrate setup.cfg to pyproject.toml migrate packaging to pyproject.toml Oct 7, 2023
@deronnax
Copy link
Contributor Author

deronnax commented Feb 5, 2024

To merge after #9210, I hope.

pyproject.toml Outdated Show resolved Hide resolved
@deronnax deronnax force-pushed the migrate_setuppy_to_pryoject.toml branch from 12a5a0b to cf25dcd Compare February 27, 2024 13:37
@deronnax
Copy link
Contributor Author

watch-out: this PR is intended to be squash-merged

@tomchristie
Copy link
Member

So... this is a good example of a nice little pr that's a bit stalled. I don't really have any extra bandwidth to keep this moving, given existing commitments. Should we be having a discussion about getting the project into jazzband.co so that we've got a lower barrier of entry for new maintainers?

@deronnax
Copy link
Contributor Author

Hey, that's a very interesting decision that is (way) beyond the scope of my humble little PR. Maybe let's open a dedicated issue ou GitHub discussion?

@sevdog
Copy link
Contributor

sevdog commented Jul 31, 2024

Can we reconsider this in the priorities of the project to reflect the latest changes in the Python environment and the problem which was caused by setuptools==72.0.0 (aka: libraries which use legacy and long deprecated setup/build configurations)?

@deronnax deronnax force-pushed the migrate_setuppy_to_pryoject.toml branch from 88a9c7f to 6f8da97 Compare October 13, 2024 16:59
pyproject.toml Outdated Show resolved Hide resolved
@deronnax deronnax force-pushed the migrate_setuppy_to_pryoject.toml branch from 05958fb to c8030fc Compare October 16, 2024 12:01
pyproject.toml Outdated Show resolved Hide resolved
@deronnax
Copy link
Contributor Author

@tomchristie that's ready to merge (let's not forget to squash merge).

setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
@deronnax deronnax force-pushed the migrate_setuppy_to_pryoject.toml branch from 2dfb5ed to 1278968 Compare November 15, 2024 16:58
@tomchristie
Copy link
Member

@deronnax Id be happy to see a release, tho someone else would need to take the lead on it.

@deronnax deronnax requested a review from auvipy November 16, 2024 10:41
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

looks better then before, but I want to do some more study before merging this

Copy link
Contributor

@terencehonles terencehonles left a comment

Choose a reason for hiding this comment

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

A couple comments I had when looking over this change

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated
Comment on lines 53 to 54
[tool.setuptools.packages.find]
namespaces = false
Copy link
Contributor

@terencehonles terencehonles Nov 19, 2024

Choose a reason for hiding this comment

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

Is there a reason you're disabling namespaces? I believe you can also just specify the packages directly since there is only one:

Suggested change
[tool.setuptools.packages.find]
namespaces = false
packages = ["rest_framework"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disabled them because they were disabled originally, using find_packages in the setup.py instead of find_namespace_packages. I tried your suggestion but it broke the CI, so I reverted it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for trying. It appears I misunderstood the default behavior for nested packages.

You'd need to try the following:

Suggested change
[tool.setuptools.packages.find]
namespaces = false
[tool.setuptools.packages.find]
include = ["rest_framework*"]

I used the build module to build a wheel and with my original suggestion it was warning that the sub-packages were not being included, but with the include statement it worked as intended and bundled the sub-packages.

Do you mind running that by the CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. But as it is changing the behavior, even if it works, I would like it in a separate PR :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well what you used doesn't match the original either, since you're missing the exclude so I'd just switch to the new style using include and you can leave off the namespace disabling or turn it off if you really prefer it to be off. I just don't think it needs to be off.

@deronnax
Copy link
Contributor Author

deronnax commented Dec 2, 2024

@auvipy can you give a deadline at which you would have done this study? I think (and some other) that's it's pretty important PR and it should not get spiraled into oblivion.

@deronnax deronnax requested a review from auvipy December 3, 2024 08:09
Copy link
Contributor

@terencehonles terencehonles left a comment

Choose a reason for hiding this comment

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

It looks like my comment was stuck pending 🤦

pyproject.toml Outdated
Comment on lines 53 to 54
[tool.setuptools.packages.find]
namespaces = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Well what you used doesn't match the original either, since you're missing the exclude so I'd just switch to the new style using include and you can leave off the namespace disabling or turn it off if you really prefer it to be off. I just don't think it needs to be off.

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.

8 participants