-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Better handling for msvc env when compiling cpp extensions #38862
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
Better handling for msvc env when compiling cpp extensions #38862
Conversation
1. Error out if msvc env is activated but `DISTUTILS_USE_SDK` is not set. 2. Attempt to activate msvc env before running ninja build
💊 CI failures summary and remediationsAs of commit b0670df (more details on the Dr. CI page):
ci.pytorch.org: 1 failedThis comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 4 times. |
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.
@peterjc123 I know this stuff is hard to test, but I guarantee you it will bitrot if we don't exercise this somehow in Windows CI
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.
@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@ezyang I'll apply to the same logic to |
@peterjc123 is this fixing a regression (so we should add it to 1.5.1)? It doesn't seem like it, but I wanted to double check. |
@gchanan Using ninja to build c++ extensions is added in v1.5.0 and this pr fixes the problem that users attempt to trigger a extension build without activating the VC environment, which is an allowed behavior in pre-v1.5.0 builds. So maybe it can be considered as a regression to some extent. |
Fixes #38861 (comment).
DISTUTILS_USE_SDK
is not set.