-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Enable symlink to installed executables for win #3760
Conversation
Hi @nanaceba, @eugene-sevostianov-sc thank you very much for the contribution. I will probably defer the review of this PR to other maintainers that know better about this part of setuptools, but I think we need tests to be added to the PR. This also seems to introduce a new piece in the build tool (cmake), which I am not entirely sure is desirable. Quick comment, is it related to the entry-point executable wrappers? |
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.
First, thanks. This looks like a solid contrib. Thorough work on revisiting this technique, which is very old (and maybe broken since I last checked).
abravalheri is correct. The script wrappers / launcher supplied by Setuptools is deprecated and would be removed except that it's relied upon by editable-installed packages (pip install -e
).
I'd prefer to see these custom-built wrappers obviated entirely by pip-installed wrappers or replace these wrappers with distlib-backed launchers.
I'd be okay with this PR except that it's expanding the duplication found in the build scripts and adding more complex logic to them (creating directories, deleting existing directories, changing directories). If we're going to accept any more complexity to the scripts, I'd like to see them ported to Python and consolidated (avoiding repetition).
Before accepting, I'll want the binaries to be generated by one of the maintainers, so they're committed/signed by a maintainer.
abravalheri suggested adding test(s). Tests would be nice to capture the failure and prevent a regression. I'd be okay to skip tests given that this functionality is deprecated anyway.
Would you be interested in porting the build scripts to a Python script that does the same thing?
@jaraco @abravalheri thank you guys for your comments! To clarify, we build cpython per platform by ourselves to be more in control of how we distribute our environment and therefore by default it looks like it doesn't have Basically right now any package we install using legacy installation in our environment
I guess its something that needs to be fixed upstream in I rewrote the build script to python and tested it. Feel free to rebuild and sign executables using the new build script. |
This looks amazing. Thanks. I'll re-generate the binaries and make sure they produce no diffs (or accept the diffs they produce), but otherwise, this is good to go. |
Oh, spoke too soon. Looks like from the test failures, this change breaks |
looking into failed tests |
After installing cmake from cmake.org and running in a Visual Studio command prompt, I got some success, but still ran into a failure. It's quite possible the failure is due to the fact that I'm building on an arm64 Windows machine.
Can you investigate the output and confirm my suspicion that the issue is caused by Windows on ARM, or is there someithing else here that needs addressing? |
@jaraco just deployed the fix to binaries and explanation to ARM problem with visual studio. It's added to the build file docs. ARM building:
|
@jaraco can you ensure that you have https://devblogs.microsoft.com/dotnet/announcing-dotnet-framework-481/ 4.81 dotnet framework installed? Seems like your issue related to the CMake and arm64. https://gitlab.kitware.com/cmake/cmake/-/merge_requests/7511 seems like that issue was fixed if you are on the latest cmake installation. |
I'm unsure what version of .Net framework I have installed (there's no indication of the version in my installed programs). Since the last time I looked at this issue, though, I've reinstalled my Windows machine (Windows 11 again), so I have a very late version of Visual Studio, almost certainly with the latest .Net framework. I'm adding the indicated features of Visual Studio and I'll retry today. |
…turally to avoid reliance on shell.
@jaraco thank you! |
And likewise, thank you. Your work here has helped simplify the codebase and sustain the maintenance of these scripts. As much as I'd like to see them be replaced by something else, I feel much better about maintaining them than I ever have. |
@jaraco @abravalheri just had an incident - we actually migrated to the wheel but the case still living - if you try to install python package as editable with |
Summary of changes
Original issue:
Symlinks to installed generated executables are not working correctly on windows.
We expect that this symlink should work correctly.
Solution:
In order to add correct symlink support for windows we are using windows api
GetFinalPathNameByHandle
that allows us to get symlink destination. If case if it is not symlink this function will just return the original file path. If it is symlink it will return symlink destination.Changes:
GetFinalPathNameByHandle
from winapi that resolve symlink/file path inlauncher.c
. This api supported from the Windows 6.1 as in original build tools.set (CMAKE_SYSTEM_VERSION 6.1 CACHE TYPE INTERNAL FORCE)
- this allows us to stay at the same API as before.Closes #3759
Pull Request Checklist
changelog.d/
.(See documentation for details)