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

[core] Implement dup2 wrapper #50439

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Feb 11, 2025

It's used in logging redirection to achieve

  • cross-platform
  • RAII-style dup2

@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Feb 11, 2025
@dentiny dentiny requested review from jjyao and edoakes February 11, 2025 20:39
Signed-off-by: dentiny <dentinyhao@gmail.com>
@dentiny dentiny force-pushed the hjiang/dup2-with-restore branch from 2fa5410 to 75db43a Compare February 11, 2025 20:51
Signed-off-by: hjiang <hjiang@anyscale.com>
Comment on lines 333 to 334
"@platforms//os:windows": ["dup2_wrapper_windows.cc"],
"//conditions:default": ["dup2_wrapper_posix.cc"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the difference between this approach and the other approach the current codebase is using which is ifdef

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No difference,

  • select clause works at file level, ifdef works at source code level
  • I use select here because implementation for two platforms are completely different and almost nothing to share

Signed-off-by: dentiny <dentinyhao@gmail.com>
@dentiny dentiny requested a review from jjyao February 12, 2025 06:04
@@ -313,6 +313,8 @@ class RAY_EXPORT Status {
// Returns the string "OK" for success.
std::string ToString() const;

std::string StatusString() const { return ToString(); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the difference between StatusString and ToString

Copy link
Contributor Author

@dentiny dentiny Feb 12, 2025

Choose a reason for hiding this comment

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

Functionality-wise no difference, the reason to have this function is for pretty print status for GTEST macros.

In nuro, we have another way to handle:

template <typename Status>
struct ConsumeStatus {...};

template <>
struct ConsumeStatus<Status> {...};
template <>
struct ConsumeStatus<StatusOr> {...};
// other status classes to handle

Having a StatusString is easier to implement.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you elaborate, I don't think I get it. Why cannot the caller call ToString()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use it in the macro

CHECK_OK(s) << s.StatusString();

it's a macro thus duck typing, so we have to have the same function name for both Status and StatusOr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling ToString is OK for Status, but improper for StatusOr, because we haven't implement the stringify function for value-type

Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
@dentiny dentiny requested a review from jjyao February 12, 2025 06:51
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
@dentiny
Copy link
Contributor Author

dentiny commented Feb 13, 2025

@jjyao The failed CI test has nothing to do with my change, which hasn't been integrated with python yet.

Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
@dentiny
Copy link
Contributor Author

dentiny commented Feb 16, 2025

@jjyao This PR should be good to go, the only change related to windows platform is C++ side unit tests, which already passed. I don't know why my CI gets stuck for a day.

@dentiny
Copy link
Contributor Author

dentiny commented Feb 17, 2025

Hi @jjyao I think this PR should be good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants