comtypes icon indicating copy to clipboard operation
comtypes copied to clipboard

Replace setup.py with pyproject.toml

Open moi15moi opened this issue 10 months ago • 6 comments

Fix https://github.com/enthought/comtypes/issues/480#issuecomment-1565784242 Note that when building the dist (with python -m build --sdist), it won't create a zip file anymore. It will be a tar.gz file. There is an issue about this, see https://github.com/pypa/setuptools/issues/3916, but honestly, I don't think it really matters.

moi15moi avatar Jun 12 '25 16:06 moi15moi

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 84.72%. Comparing base (f2bd605) to head (1a1d9ad).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #848   +/-   ##
=======================================
  Coverage   84.72%   84.72%           
=======================================
  Files         125      125           
  Lines       11552    11552           
=======================================
  Hits         9787     9787           
  Misses       1765     1765           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jun 12 '25 16:06 codecov[bot]

I'm currently trying to investigate why the install-tests CI fails only for Python 3.8

moi15moi avatar Jun 12 '25 16:06 moi15moi

Oh, the problem is simple. The license in the pryproject.toml have been added in setuptools 77.0.0. The last version to support Python 3.8 is 75.3.2, so it cannot simply work.

I guess I will need to wait that you drop 3.8 support.

moi15moi avatar Jun 12 '25 21:06 moi15moi

I am planning to drop support for Python 3.8 soon. 1.4.12 will likely be the last release to support it. This means this PR might be merged after the next release.

junkmd avatar Jun 16 '25 14:06 junkmd

The migration of setup.cfg to pyproject.toml, as per this PR's title, is clear and understandable.

However, I believe the legacy setup.py code should be addressed in a separate scope. I'm not deeply familiar with packaging, so please inform me if these two tasks are interdependent.

junkmd avatar Jun 16 '25 14:06 junkmd

However, I believe the legacy setup.py code should be addressed in a separate scope.

It is indeed possible to keep setup.py. But, if you want to keep setup.py, this PR isn't usefull because the pyproject.toml would basically be the same thing has setup.cfg.

But, do you have any reason to keep setup.py? The only difference between this setup (with pyproject.toml) and setup.py is that I removed the cmdclass.

There are 2 cmdclass:

  • post_install was just a workaround for a old setuptools bug. It's clearly not needed anymore, so it is safe to remove it.
  • test. I'm not sure what it does. It's seems to do the same stuff has simply running the test with my pytest, but i'm not sure. Could you confirm it?

moi15moi avatar Jun 19 '25 23:06 moi15moi

@moi15moi

I apologize for the delay in my response; I've been quite busy.

My initial thought to keep setup.py was mainly because I wasn't entirely clear on what those two cmdclasses were doing. I appreciate the clarification on post_install. I now understand that it's no longer necessary in modern packaging, and I'm fine with its removal.

However, I still have some uncertainties about the test class. My understanding is that options like tests= and use-resources= seem to have very little meaning given the current use of test resources and the unittest suite. On the other hand, I don't fully understand the refcounts part.

The refcounts option appears to be doing something with sys.gettotalrefcount(), which is a function only available in Python's debug builds. However, the specific purpose of this test remains unclear to me. If you have any insights or conjectures about the behavior or intent of this test class, I would greatly appreciate it if you could share them.

junkmd avatar Jun 30 '25 13:06 junkmd

I tried to run python setup.py test in the CI. You can see the result here: https://github.com/moi15moi/comtypes/pull/10/checks As you can see, it currently simply fails. I guess it is was a way to detect if there is memory leak when executing test?

moi15moi avatar Jul 06 '25 17:07 moi15moi

@moi15moi

Thank you for your investigation.

I guess it was a way to detect if there is memory leak when executing tests?

I agree. I also suspect the code invoked when running test through setup.py relies heavily on a very old internal implementation of unittest.

I believe it's fine to remove cmdclasses and other specific functionalities from setup.py.

My main concern is whether we can safely delete setup.py entirely, even without requiring setuptools>=77.0.0.

If you have the resources to open a separate PR that solely focuses on removing setup.py (and making any necessary adjustments to other related files), I think that PR could be merged even before we drop Python 3.8 support.

junkmd avatar Jul 07 '25 14:07 junkmd

license-files has been introduced with 77.0.0, so I cannot directly use a older version. It is now recommand to use license-files, so, in my opinion, we should just wait to drop support for python 3.8 before merging this PR.

moi15moi avatar Jul 14 '25 23:07 moi15moi

I just rebased because you dropped python 3.8 support in https://github.com/enthought/comtypes/pull/839 Except for this point, I think this PR is ready to be reviewed/merged.

moi15moi avatar Oct 12 '25 15:10 moi15moi

Assuming that this patch is applied, I checked that I could upload the package to TestPyPI after building it with python -m build --sdist --wheel. I also confirmed that the license information remained in the metadata.

image

junkmd avatar Nov 29 '25 14:11 junkmd