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

Automatically run pywin32_postinstall.py for "local"/"from source" installs #2447

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ https://mhammond.github.io/pywin32_installers.html.
Coming in build 309, as yet unreleased
--------------------------------------

* Added support for relative path for `pywin32_postinstall`'s `-destination` argument (#2447, @Avasam)
* Removed param `hIcon` from `win32comext.shell.ShellExecuteEx`. It was unusable since Windows Vista (#2423, @Avasam)
* Fixed `nbios.NCBStruct` packing (#2406, @Avasam)
* Restored axdebug builds on Python 3.10 (#2416, @Avasam)
Expand Down
7 changes: 5 additions & 2 deletions pywin32_postinstall.py
Original file line number Diff line number Diff line change
Expand Up @@ -642,9 +642,12 @@ def uninstall(lib_dir):
# Out of principle, we're still not using system exits.


def verify_destination(location):
def verify_destination(location: str) -> str:
location = os.path.abspath(location)
if not os.path.isdir(location):
raise argparse.ArgumentTypeError(f'Path "{location}" does not exist!')
raise argparse.ArgumentTypeError(
f'Path "{location}" is not an existing directory!'
)
return location


Expand Down
23 changes: 8 additions & 15 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -856,35 +856,28 @@ def run(self):
This is only run for local installs. Wheel-based installs won't run this code.
"""
install.run(self)
# If self.root has a value, it means we are being "installed" into some other
# directory than Python itself - in which case we must *not* run our installer.
# bdist_wininst used to trigger this by using a temp directory.
# Is this still a concern ?
if self.root:
print(
"Not executing post install script when "
+ f"not installing in Python itself (self.root={self.root})"
)
return
self.execute(self._postinstall, (), msg="Executing post install script...")
# Hacky, skip for build-only commands
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@Avasam Avasam Jan 8, 2025

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@Avasam Avasam Jan 8, 2025

Choose a reason for hiding this comment

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

Got a reply: pypa/setuptools#4791 (comment)
I'm really tempted to prefer #2453 over this.

if "build" in sys.argv:
print("Skipping post install script for build commands.")
else:
self.execute(self._postinstall, (), msg="Executing post install script...")

def _postinstall(self):
filename = os.path.join(self.install_scripts, "pywin32_postinstall.py")
if not os.path.isfile(filename):
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(
Copy link
Collaborator Author

@Avasam Avasam Jan 5, 2025

Choose a reason for hiding this comment

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

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.

[
subprocess.check_call(
(
sys.executable,
filename,
"-install",
"-destination",
self.install_lib,
"-quiet",
Copy link
Collaborator Author

@Avasam Avasam Jan 5, 2025

Choose a reason for hiding this comment

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

At first I did

Suggested change
"-quiet",
*([] if self.verbose else ["-quiet"]),

but realized pip without -v isn't printing any of this anyway.

"-wait",
str(os.getpid()),
]
)
)


Expand Down