Automatically run pywin32_postinstall.py for "local"/"from source" installs
#2447
Conversation
…alls (`pip install .`) - Also respect the verbose flag instead of always running quiet - Added support for relative path for the `-destination` argument
5219ca8 to
9ff63c2
Compare
| sys.executable, | ||
| filename, | ||
| "-install", | ||
| "-destination", | ||
| self.install_lib, | ||
| "-quiet", |
There was a problem hiding this comment.
At first I did
| "-quiet", | |
| *([] if self.verbose else ["-quiet"]), |
but realized pip without -v isn't printing any of this anyway.
| @@ -874,17 +864,16 @@ def _postinstall(self): | |||
| raise RuntimeError(f"Can't find '{filename}'") | |||
| # As of setuptools>=74.0.0, we no longer need to | |||
| # be concerned about distutils calling win32api | |||
| subprocess.Popen( | |||
There was a problem hiding this comment.
Note: This wouldn't replace #2392
That other PR also tests uninstalling
In fact both together should be better as it would test the install under 2 different contexts, and test a re-install.
…m/Avasam/pywin32 into run-postinstall-on-local-installs
| ) | ||
| return | ||
| self.execute(self._postinstall, (), msg="Executing post install script...") | ||
| # Hacky, skip for build-only commands |
There was a problem hiding this comment.
I don't understand this. Doesn't "setup.py build install" work? I don't think this check is related to the old check though, that's just about the target?
There was a problem hiding this comment.
As a clarification, calling setup.py directly should no longer be done. It has been long deprecated and setuptools is no longer in the business of acting as a build frontend and installer. (that's what #2396 / #2208 was about)
You should see setup.py as a dynamic configuration file, rather than a script to be run.
We currently use a mix of pip wheel (for regular builds + local installs) and https://github.com/pypa/build + https://github.com/pypa/wheel (for cross-compiled ARM64) as build frontends to create wheels.
For historical and backwards compatibility reasons, setuptools.build_meta:__legacy__ is the default fallback backend (https://peps.python.org/pep-0517/#summary-of-changes-to-pep-517), which is why that part hasn't broken for us, despite not specifying anywhere (in a pyproject.toml file) that we use setuptools as a build backend.
Now why change this code?
At the moment in the main branch, this my_install class is complete dead code. Installing from a wheel (which is what all pywin32 users installing from PyPI do, since we don't provide a source distribution) doesn't run any code from setup.py (it's all static and metadata).
Even when installing from source, that is, downloading the source code/cloning the repository, and running pip install ., (or whichever package manager, be it poetry, PDM, uv, etc.). This _postinstall method wouldn't trigger because the if self.root branch is always taken when running pip install .
So this PR restores automatically running the postinstall script only for those who install pywin32 from building the source code.
The hacky condition is because setuptool's bdist_wheel will internally call self.run_command("install") to install to the temporary build folder. Which is an issue for the ARM64 steps as they otherwise try to run the postinstall script, which obviously fail due to trying to load a DLL for a different architecture. And I don't know how to differentiate between "just build a distribution" and "build to install" pypa/setuptools#4791
That all being said. I'm also considering just removing this custom install command and any leftover assumption in comments/doc that there's a case in which the pywin32_postinstall script can be run automatically.
Edit: I went ahead and created said PR that is the inverse of this one: #2453
There was a problem hiding this comment.
Got a reply: pypa/setuptools#4791 (comment)
I'm really tempted to prefer #2453 over this.
pywin32_postinstall.pyfor "local"/"from source" installs (pip install .)-destinationargumentResolves #2408 (comment)
If you don't think that
pywin32_postinstallshould be run at all for installs from source, then the entiremy_installclass should be removed instead.