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

Add test for postinstall #2392

Merged
merged 20 commits into from
Jan 7, 2025
Merged

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Oct 12, 2024

I added it as an actual test in test files, rather than a CI only thing. Not sure if it belongs exactly there. Please let me know.

One thing I don't like is that it'll leave the dev's installation in a different state than they might've started it, maybe I should run

    find_and_run(maybes, ["-install"])
    find_and_run(maybes, ["-remove"])
    find_and_run(maybes, ["-install"])

or idk, maybe it should be locked behind a flag and/or be a CI only test

Maybe it'd be better as a build flag so the CI can say "run postinstall after install", also giving users the choice. And making the installation a single step (with he optional flag) for those who used to use the exe installers for that. (using https://pip.pypa.io/en/stable/cli/pip_install/#cmdoption-C once #2208 and #2349 are resolved)

@Avasam Avasam mentioned this pull request Oct 12, 2024
@Avasam
Copy link
Collaborator Author

Avasam commented Oct 12, 2024

hmmm, can't find DLL in CI. Is it because of the --user install or am I missing something ?

@mhammond
Copy link
Owner

The test scripts are run from the source dir, but the post-install script needs to be run from the installed dir. ie, it should probably be in the CI tasks rather than hacked into the test script. I think just executing it is fine - no need to run tests again or anything, just the fact it doesn't fail is enough. I'm not that worried about the installed state being changed as I'm fairly sure the wheels are created from the build dir and isn't going to touch the installed state.

I'm also not too worried about this for the release - have you been able to manually verify the postinstall script now works?

@Avasam
Copy link
Collaborator Author

Avasam commented Oct 12, 2024

the post-install script needs to be run from the installed dir. ie, it should probably be in the CI tasks rather than hacked into the test script.

Alright, that makes sense I'll update that.

I think just executing it is fine - no need to run tests again or anything, just the fact it doesn't fail is enough

Yeah I just want a "smokescreen" test, ie: does it even run

have you been able to manually verify the postinstall script now works?

As far as I can tell, yeah. I applied the same fix as #2381 in my installed site-packages on 307 and I'm no longer getting circular import errors)

Copy link
Owner

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Thanks!

@Avasam
Copy link
Collaborator Author

Avasam commented Jan 5, 2025

I finally figured out the issue! The -destination argument of pywin32_postinstall is trying to default to the location of the pywin32 installation, but was incorrectly assuming it'll always be in the global platlib location (C:\hostedtoolcache\windows\Python\3.8.10\x64\Lib\site-packages). Which isn't always true as demonstrated in our CI, where we install in the user location C:\Users\runneradmin\AppData\Roaming\Python\Python38\site-packages). Specifying -destination explicitly did the trick.

Surely there's a better default we can use, but I'll leave that to a different PR, and create an issue to track it.

@Avasam Avasam merged commit 0aadef0 into mhammond:main Jan 7, 2025
30 checks passed
@Avasam Avasam deleted the add-test-for-postinstall branch January 7, 2025 04:19
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.

2 participants