Skip to content

Conversation

@moi15moi
Copy link
Contributor

Fix #480 (comment)
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 pypa/setuptools#3916, but honestly, I don't think it really matters.

@codecov
Copy link

codecov bot commented Jun 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.69%. Comparing base (fb8fd58) to head (a0d6cd6).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #848   +/-   ##
=======================================
  Coverage   84.69%   84.69%           
=======================================
  Files         125      125           
  Lines       11532    11532           
=======================================
  Hits         9767     9767           
  Misses       1765     1765           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

description = "Pure Python COM package"
readme = "README.md"
requires-python = ">=3.8"
license = "MIT"
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'm not really sure what is the license of the project.
setup.cfg specify MIT, but the license file seems to says OSI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The clauses in LICENSE.txt are the same as those of the MIT License.

Although starting the file with the line MIT License is the practice generally accepted today, the wording might be consistent, as the MIT License is officially listed as an OSI-approved open source license.

comtypes/LICENSE.txt

Lines 1 to 2 in 8d40715

This software is OSI Certified Open Source Software.
OSI Certified is a certification mark of the Open Source Initiative.

@cfarrow
If you remember, I would like to confirm the intent behind this license wording with you, who committed LICENSE.txt in 8d40715.
Was this common practice when this commit was made?

@moi15moi
Copy link
Contributor Author

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

@moi15moi moi15moi force-pushed the Add-pyproject.toml branch from cae9b83 to c5c50fe Compare June 12, 2025 17:16
@moi15moi
Copy link
Contributor Author

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.

@junkmd
Copy link
Collaborator

junkmd commented Jun 16, 2025

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
Copy link
Collaborator

junkmd commented Jun 16, 2025

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.

@moi15moi
Copy link
Contributor Author

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?

@junkmd
Copy link
Collaborator

junkmd commented Jun 30, 2025

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

@moi15moi
Copy link
Contributor Author

moi15moi commented Jul 6, 2025

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?

@junkmd
Copy link
Collaborator

junkmd commented Jul 7, 2025

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

@moi15moi
Copy link
Contributor Author

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
Copy link
Contributor Author

I just rebased because you dropped python 3.8 support in #839
Except for this point, I think this PR is ready to be reviewed/merged.

- name: install comtypes
run: |
pip install --upgrade setuptools
pip install --upgrade setuptools build
Copy link

Choose a reason for hiding this comment

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

With PEP517 build isolation and setuptools specified in your [build-system].requires pre-installing setuptools here should do nothing.

Suggested change
pip install --upgrade setuptools build
pip install --upgrade build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I didn't see it.

It is needed for test_pip_install.py
This is necessary because setuptools added support for ``license`` and ``license-files`` in that version (and we use those in pyproject.toml)

Changelog: https://setuptools.pypa.io/en/latest/history.html#v77-0-0
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.

Modernising the dev tooling

3 participants