-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Not explicitly set the manifest filename in Windows #91988
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
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/91988
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 1 FailuresAs of commit d6821b6: NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
error C4996: 'getenv': This function or variable may be unsafe
// NB: | ||
// Issues a warning if the value of the environment variable is not 0 or 1. | ||
inline optional<bool> check_env(const char* name) { | ||
#ifdef _MSC_VER |
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.
torch_test_cpp_extension
does some compilation at testing time https://github.com/pytorch/pytorch/actions/runs/3890569745/jobs/6640722956 and it fails with the following error.
C:\actions-runner\_work\pytorch\pytorch\build\win_tmp\build\torch\include\c10/util/env.h(17): error C4996: 'getenv': This function or variable may be unsafe. Consider using _dupenv_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
This looks unrelated to the linker issue, so I suspect that a recent commit merged while Windows CPU build and tests are disable causes this ( no signal). The fix looks simple enough though.
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.
I just realize that the same error is also causing failures on periodic https://hud.pytorch.org/commit/pytorch/pytorch/364f526b9cdf9818a7647b5e637efdee825d61a1
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.
JFYI, getenv_s
should be used instead https://en.cppreference.com/w/c/program/getenv
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.
Noted, I'll fix this properly in another PR and will merge this once Windows CPU build and test pass.
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.
Alternatively including <cstdlib>
together with the std::
namespace clasifier might resolve the issue as https://stackoverflow.com/a/631696/685292 or other sources mentions, though I haven't tried it.
@pytorchbot merge -f 'Windows CPU build and test jobs have passed. MacOS failure is unrelated' |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
I'm at a loss to explain why this happens, but not setting the manifest file explicitly in the linker fixes it.
Testing locally
/MANIFESTFILE:bin\torch_python.dll.manifest
In both case, the
/MANIFEST
flag is set, so the manifest file is there. In the latter case, the filename comes by appending.manifest
suffix tobin\torch_python.dll
. Thus, it's still correctly bebin\torch_python.dll.manifest
. Weird.