Skip to content
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

Fix Windows build due to winsock2.h conflicts #25447

Merged
merged 1 commit into from
Feb 28, 2019

Conversation

rongjiecomputer
Copy link
Contributor

@rongjiecomputer rongjiecomputer commented Feb 2, 2019

Some abseil header files include windows.h. Without WIN32_LEAN_AND_MEAN macro, windows.h will include winsock.h which will conflict with other files that want to include winsock2.h instead.

We should be able to revert these rollbacks after this PR.

@rthadur rthadur self-assigned this Feb 2, 2019
@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Feb 2, 2019
@rthadur rthadur requested a review from angerson February 2, 2019 17:48
@rthadur rthadur added size:XS CL Change Size: Extra Small awaiting review Pull request awaiting review and removed awaiting review Pull request awaiting review labels Feb 2, 2019
@rongjiecomputer
Copy link
Contributor Author

@perfinion Can you help me to trigger CI test so that I can check if this does fix Windows CI?

@perfinion perfinion added the kokoro:force-run Tests on submitted change label Feb 4, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Feb 4, 2019
@perfinion
Copy link
Member

@rongjiecomputer Just to be clear, do you want experimental_shortened_obj_file_path added or removed? If you want it removed, can you squash both your commits into one and rebase it on top of master once 25335 is merged into master? That will be much cleaner and less error-prone than the second commit which adds it back.

@rongjiecomputer
Copy link
Contributor Author

If you want it removed, can you squash both your commits into one and rebase it on top of master once 25335 is merged into master? That will be much cleaner and less error-prone than the second commit which adds it back.

Done.

@angerson
Copy link
Contributor

angerson commented Feb 4, 2019

I'll approve this once @perfinion has approved it, if needed.

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Feb 5, 2019
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Feb 5, 2019
Copy link
Member

@perfinion perfinion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Feb 5, 2019
@rthadur rthadur added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Feb 5, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Feb 5, 2019
@Harshini-Gadige Harshini-Gadige added the ready to pull PR ready for merge process label Feb 13, 2019
@rthadur rthadur added pending merge internally ready to pull PR ready for merge process and removed pending merge internally ready to pull PR ready for merge process labels Feb 21, 2019
Halo9Pan pushed a commit to Halo9Pan/dive-tensorflow that referenced this pull request Feb 28, 2019
@yifeif yifeif merged commit 31f10d2 into tensorflow:master Feb 28, 2019
PR Queue automation moved this from Approved by Reviewer to Merged Feb 28, 2019
@meteorcloudy
Copy link
Member

Adding build --copt=-DWIN32_LEAN_AND_MEAN --host_copt=-DWIN32_LEAN_AND_MEAN into bazelrc file will cause the whole TensorFlow build not being able to use some of the less frequently used APIs. Can we only add --copt=-DWIN32_LEAN_AND_MEAN --host_copt=-DWIN32_LEAN_AND_MEAN to the targets that actually need it?

FYI @gunan , this caused the failure

Execution platform: @org_tensorflow//third_party/toolchains/preconfig/win_1803:rbe_windows_1803
external/bazel_tools/src/main/cpp/util/file_windows.cc(526): error C2065: 'FSCTL_GET_REPARSE_POINT': undeclared identifier
external/bazel_tools/src/main/cpp/util/file_windows.cc(615): error C2065: 'FSCTL_GET_REPARSE_POINT': undeclared identifier

@rongjiecomputer
Copy link
Contributor Author

rongjiecomputer commented Mar 12, 2019

@meteorcloudy I don't think it is easy to figure out which files that need WIN32_LEAN_AND_MEAN. Having to add copts for the affected targets is also pretty tedious in Bazel.

A workaround is for files that need header filtered by WIN32_LEAN_AND_MEAN, explicitly include them instead of relying on windows.h to include them.

Here is a test:

#define WIN32_LEAN_AND_MEAN
#include <windows.h>
#include <WinIoCtl.h>

#ifndef FSCTL_GET_REPARSE_POINT
#error "Hello"
#else
#error "World"
#endif`

FSCTL_GET_REPARSE_POINT comes from WinIoCtl.h (https://msdn.microsoft.com/en-us/library/windows/desktop/aa364571(v=vs.85).aspx).

@meteorcloudy
Copy link
Member

@rongjiecomputer Thank you!

@rongjiecomputer rongjiecomputer deleted the fix_win_build branch March 12, 2019 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

10 participants