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.

Sign up for GitHub

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

Jump to bottom

[core] Fix windows build for pipe logger #49780

Merged
merged 5 commits into from
Jan 14, 2025
Merged

[core] Fix windows build for pipe logger #49780

merged 5 commits into from
Jan 14, 2025

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Jan 11, 2025

This PR does two things:

  • Finish a TODO for using compat macro to represent windows HANDLE type and unix file descriptor
  • Fix compilation on windows

@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Jan 11, 2025
@dentiny dentiny requested review from jjyao and rynewang January 11, 2025 09:17
@dentiny dentiny force-pushed the hjiang/fix-windows-build branch from 1aa8bb3 to 54bd440 Compare January 11, 2025 11:51
@dentiny
fix windows build
ebcf2fa 
Signed-off-by: dentiny <[email protected]>
@dentiny dentiny force-pushed the hjiang/fix-windows-build branch from 54bd440 to ebcf2fa Compare January 11, 2025 11:52
@dentiny dentiny requested a review from aslonnie January 11, 2025 18:34
@jjyao
Copy link
Collaborator

jjyao commented Jan 13, 2025

Manually triggered windows test.

aslonnie
aslonnie reviewed Jan 13, 2025
View reviewed changes
Copy link
Collaborator

@aslonnie aslonnie left a comment

Choose a reason for hiding this comment

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

seems that the build is still broken?

@dentiny
fix type conversion
6505ddb 
Signed-off-by: dentiny <[email protected]>
@dentiny dentiny force-pushed the hjiang/fix-windows-build branch from 7dfaced to 6505ddb Compare January 13, 2025 20:46
@dentiny
fix ctor
6f76149 
Signed-off-by: dentiny <[email protected]>
@dentiny
fix function traits
898d632 
Signed-off-by: dentiny <[email protected]>
@aslonnie
Copy link
Collaborator

https://buildkite.com/ray-project/premerge/builds/33288#0194623b-8579-4598-9eff-48d02a91ece2/6-11737

[2025-01-14T02:25:15Z] bazel-out/x64_windows-opt/bin/src/ray/util/tests/_virtual_includes/unix_test_utils\ray/util/tests/unix_test_utils.h(20): fatal error C1189: #error:  "This header file can only be used in unix"

@dentiny
Copy link
Contributor Author

dentiny commented Jan 14, 2025

[2025-01-14T02:25:15Z] bazel-out/x64_windows-opt/bin/src/ray/util/tests/_virtual_includes/unix_test_utils\ray/util/tests/unix_test_utils.h(20): fatal error C1189: #error: "This header file can only be used in unix"

Just put a commit to fix

@dentiny
Copy link
Contributor Author

dentiny commented Jan 14, 2025

@jjyao / @aslonnie The failed CI test has nothing to do with my change, since it's not used in production now. Would appreciate if you could help me merge the PR, thanks!

@aslonnie
Copy link
Collaborator

seems that tests are all passing now.

@aslonnie
Copy link
Collaborator

I am merging since this at least fixes the window build, and the changes look mostly reasonable to me.

@aslonnie aslonnie merged commit 34c3f1e into ray-project:master Jan 14, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aslonnie aslonnie aslonnie approved these changes

@jjyao jjyao Awaiting requested review from jjyao

@rynewang rynewang Awaiting requested review from rynewang

Assignees

@jjyao jjyao

@rynewang rynewang

@aslonnie aslonnie

Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dentiny @jjyao @aslonnie @rynewang