-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 msvcp140.dll to the wheel on windows #19062
Conversation
python/setup.py
Outdated
print('copy_file uses', build_ext.build_lib, ROOT_DIR) | ||
shutil.copy(r'c:\Windows\system32\msvcp140.dll', | ||
os.path.join(build_ext.build_lib, 'ray')) |
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.
print('copy_file uses', build_ext.build_lib, ROOT_DIR) | |
shutil.copy(r'c:\Windows\system32\msvcp140.dll', | |
os.path.join(build_ext.build_lib, 'ray')) | |
print("copy_file uses", build_ext.build_lib, ROOT_DIR) | |
shutil.copy(r"c:\Windows\system32\msvcp140.dll", | |
os.path.join(build_ext.build_lib, "ray")) |
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.
We use double quotes for literals
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.
thanks, fixed and removed the debug print
On the one hand this is a very small change, on the other it is a bit obscure how the dll ends up in the wheel. I guess |
Explicit emphasis and comments will help maintainers moving forward!
…On Sun, Oct 3, 2021 at 2:27 PM Matti Picus ***@***.***> wrote:
On the one hand this is a very small change, on the other it is a bit
obscure how the dll ends up in the wheel. I guess git grep msvcp140 will
find it, is that discoverable enough or should I make a copy_system_dlls()
function to emphasize it?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#19062 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABCRZZKXWGLGRXD4JJEQOIDUFDDFVANCNFSM5FH2F2ZA>
.
|
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.
Thanks for doing this, this is great! Looks like the windows tests failed for unrelated reasons, re-running them.
@mattip Looks like there is an unrelated doc building failure, can you merge master into this PR? that should make it go away! |
rebased off master |
Why are these changes needed?
Closes #13293. As described in the issue, on a minimal windows image the support dll
msvcp140.dll
is needed to load_raylet.pyd
.Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.