-
Notifications
You must be signed in to change notification settings - Fork 810
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
base: main
Are you sure you want to change the base?
Changes from all commits
9ff63c2
d854542
d1fa232
71b349c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
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( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: This wouldn't replace #2392 |
||||||
[ | ||||||
subprocess.check_call( | ||||||
( | ||||||
sys.executable, | ||||||
filename, | ||||||
"-install", | ||||||
"-destination", | ||||||
self.install_lib, | ||||||
"-quiet", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first I did
Suggested change
but realized pip without |
||||||
"-wait", | ||||||
str(os.getpid()), | ||||||
] | ||||||
) | ||||||
) | ||||||
|
||||||
|
||||||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 apyproject.toml
file) that we use setuptools as a build backend.Now why change this code?
At the moment in the
main
branch, thismy_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 fromsetup.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 theif self.root
branch is always taken when runningpip 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 callself.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#4791That 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 thepywin32_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.
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.