search

mhammond / pywin32

Python for Windows (pywin32) Extensions
5.08k stars 800 forks source link

Add test for postinstall #2392

Open Avasam opened 1 month ago

Avasam commented 1 month ago

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 commented 1 month ago

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

mhammond commented 1 month ago

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 commented 1 month ago

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)