Prevent .whl installation (OOTB) on non Win #405
Conversation
|
I searched |
|
@junkmd: aaargh I forgot about this in my 3rd commit. thank you for pointing it out and sorry for the inconvenience. |
|
Out of curiosity, do you know where a cheat sheet of When I look up |
|
I don't have a clear location in mind (I'm not an expert in that area). There's always the source code (bdist_wheel.py), and for clear tasks (and source is too vast while time is too short) I browse the web. |
|
@jaraco Do you see any concerns with this PR? |
| 'Programming Language :: Python :: 3.7', | ||
| 'Programming Language :: Python :: 3.8', | ||
| 'Programming Language :: Python :: 3.9', | ||
| 'Programming Language :: Python :: 3.10', | ||
| 'Programming Language :: Python :: 3.11', | ||
| 'Programming Language :: Python :: 3.12', |
There was a problem hiding this comment.
I used to try to keep up with all of the supported Python versions, but found it to be a maintenance nightmare. Moreover, it's redundant with the python_requires directive. Just declare Python :: 3 and ignore the sub versions.
There was a problem hiding this comment.
Although this change aims to make wheels not installable, it's still not going to prevent their sdist counterparts from being downloaded and installed:
$ py -3.10 -m pip-run --use-pep517 git+https://github.com/CristiFati/comtypes@cfati_dev01
Python 3.10.8 (main, Oct 13 2022, 09:48:40) [Clang 14.0.0 (clang-1400.0.29.102)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import comtypes
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/var/folders/sx/n5gkrgfx6zd91ymxr2sr9wvw00n8zm/T/pip-run-m5g6fgu_/comtypes/__init__.py", line 7, in <module>
from _ctypes import COMError
ImportError: cannot import name 'COMError' from '_ctypes' (/opt/homebrew/Cellar/[email protected]/3.10.8/Frameworks/Python.framework/Versions/3.10/lib/python3.10/lib-dynload/_ctypes.cpython-310-darwin.so)
Let's not accept this and the maintenance burden it adds. Instead, if Cristi would like to pursue a more robust fix, I recommend to contribute a feature to the Python packaging ecosystem that allows any project to provide metadata that would satisfy this need.
| try: | ||
| from wheel.bdist_wheel import bdist_wheel | ||
| class bdist_wheel_win(bdist_wheel): | ||
| def get_tag(self): | ||
| win_plats = ( # Copied the list from DistUtils | ||
| "win-amd64", | ||
| "win32", | ||
| "win-arm64", | ||
| "win-arm32", | ||
| ) | ||
| tag = super().get_tag() | ||
| return tuple(tag[:-1]) + (".".join(wp.replace("-", "_") for wp in win_plats),) | ||
|
|
||
| except ImportError: | ||
| bdist_wheel_win = None | ||
|
|
||
|
|
There was a problem hiding this comment.
This code feels really brittle. I'd like to see this project and others move away from setup.py altogether. It feels wrong or at least a hack to try to present this module as requiring Windows. It does have a runtime dependency on Windows or at least an implementation of Ctypes that supports the com types, but none of that is part of this package. Moreover, there's nothing special about the wheel that it's not installable on Unix but the sdist is. I'm not confident this unusual, mangled bdist name is going to be supported or useful. It may block releasing to PyPI (though I suspect you've tested this design).
I'm inclined to say no, let's not try to implement platform specificity here. Instead, I'd say to bring this problem to pypa/packaging-problems and explain the use case and request/contribute a solution that any package can avail to declare platform specificity.
| @@ -162,6 +183,9 @@ def run(self): | |||
| "comtypes.tools", | |||
| "comtypes.test", | |||
| ], | |||
|
|
|||
| platforms=["Windows"], | |||
There was a problem hiding this comment.
This field is poorly documented, non-standardized, and ineffective. Let's avoid it.
|
Considering latest remarks, should I close this PR as well? |
|
After reading @jaraco's review, I now understand why I could not find sufficient description in the It means that I think @CristiFati can contribute to this project by moving the package settings to As for introducing a configuration value that would limit installation to Windows only, I think that will come after pypa implements that feature. I am trying to install an auto-formatter in #418, which may cause conflicts in If @CristiFati can contribute on the migration from |
|
I’m sorry. I restored branch. |
|
It will probably change to a command like running |
|
I didn't work with pyproject.toml or setup.cfg, I don't know the differences between them and why someone would choose one over the other. So, even if I'd try doing it, I'm sure that an expert on the area would be able to complete it much faster and better. I was aiming to solve a problem (although it's not such a big deal, and I also mentioned this package in [SO]: Install win32com on MacOs and Linux (@CristiFati's answer)) that I discovered accidentally on SO, (also the place / way that I chose to solve it was a bad one), but apparently there's way more going on, that people like me (outside main maintainers group) are unaware of. Therefore I propose to close this, and when things will settle (and directions will be clear), "outsiders" will have more chances to contribute. |
|
OK. I will close this in the coming days. Thank you for your suggestion. Followings is that I have been able to find out. Previous versions of https://github.com/pywinauto/pywinauto/blob/f78302c7df7a71980953d3007ed72de6a2a8e604/setup.py#L59-L74 From these examples, I believe that the responsibility for not allowing the package to be installed that cannot be used with the OS type lies not with pypa or the package itself, but with the project that uses it as a dependency. So, if you propose to set up the |
This is the same as #394, but somehow I got the branches messed up, and ended in a situation that some other commits (already present in the target branch) were included in my PR.
Cherry-picked the commits from the old branch in this one.
The .whl will no longer: