You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/06/09 16:01:54 UTC
[GitHub] [arrow] pitrou opened a new pull request, #13354: ARROW-16799: [C++] Create a self-pipe abstraction.
pitrou opened a new pull request, #13354:
URL: https://github.com/apache/arrow/pull/13354
Also create a FileDescriptor RAII wrapper to automate the chore of closing file descriptors, and make it more robust.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13354:
URL: https://github.com/apache/arrow/pull/13354#issuecomment-1151562338
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13354:
URL: https://github.com/apache/arrow/pull/13354#issuecomment-1151551466
While most CI failures are unrelated, the test-debian-10-cpp-i386 failure is directly caused by this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13354:
URL: https://github.com/apache/arrow/pull/13354#issuecomment-1151563130
Revision: dcf11356c22772b791d843453eb9ee59d0e6bb5c
Submitted crossbow builds: [ursacomputing/crossbow @ actions-9ec240e09d](https://github.com/ursacomputing/crossbow/branches/all?query=actions-9ec240e09d)
|Task|Status|
|----|------|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-build-cpp-fuzz)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-conda-cpp)|
|test-conda-cpp-valgrind|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-9ec240e09d-azure-test-conda-cpp-valgrind)|
|test-debian-10-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-debian-10-cpp-amd64)|
|test-debian-10-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-debian-10-cpp-i386)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-debian-11-cpp-amd64)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-debian-11-cpp-i386)|
|test-fedora-35-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-fedora-35-cpp)|
|test-ubuntu-18.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-ubuntu-18.04-cpp)|
|test-ubuntu-18.04-cpp-release|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-ubuntu-18.04-cpp-release)|
|test-ubuntu-18.04-cpp-static|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-ubuntu-18.04-cpp-static)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-ubuntu-20.04-cpp)|
|test-ubuntu-20.04-cpp-14|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-ubuntu-20.04-cpp-14)|
|test-ubuntu-20.04-cpp-17|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-ubuntu-20.04-cpp-17)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-ubuntu-20.04-cpp-bundled)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-ubuntu-20.04-cpp-thread-sanitizer)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-9ec240e09d-github-test-ubuntu-22.04-cpp)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a diff in pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on code in PR #13354:
URL: https://github.com/apache/arrow/pull/13354#discussion_r894088698
##########
cpp/src/arrow/util/io_util.h:
##########
@@ -124,14 +125,46 @@ Result<bool> DeleteFile(const PlatformFilename& file_path, bool allow_not_found
ARROW_EXPORT
Result<bool> FileExists(const PlatformFilename& path);
+// TODO expose this more publicly to make it available from io/file.h?
+/// A RAII wrapper for a file descriptor.
Review Comment:
nit nit: `An RAII`?
##########
cpp/src/arrow/util/io_util_test.cc:
##########
@@ -159,6 +170,282 @@ TEST(WinErrorFromStatus, Basics) {
}
#endif
+class TestFileDescriptor : public ::testing::Test {
+ public:
+ Result<int> NewFileDescriptor() {
+ // Make a new fd by dup'ing C stdout (why not?)
+ int new_fd = dup(1);
+ if (new_fd < 0) {
+ return IOErrorFromErrno(errno, "Failed to dup() C stdout");
+ }
+ return new_fd;
+ }
+
+ void AssertValidFileDescriptor(int fd) {
+ ASSERT_FALSE(FileIsClosed(fd)) << "Not a valid file descriptor: " << fd;
+ }
+
+ void AssertInvalidFileDescriptor(int fd) {
+ ASSERT_TRUE(FileIsClosed(fd)) << "Unexpectedly valid file descriptor: " << fd;
+ }
+};
+
+TEST_F(TestFileDescriptor, Basics) {
+ int new_fd, new_fd2;
+
+ // Default initialization
+ FileDescriptor a;
+ ASSERT_EQ(a.fd(), -1);
+ ASSERT_TRUE(a.closed());
+ ASSERT_OK(a.Close());
+ ASSERT_OK(a.Close());
+
+ // Assignment
+ ASSERT_OK_AND_ASSIGN(new_fd, NewFileDescriptor());
+ AssertValidFileDescriptor(new_fd);
+ a = FileDescriptor(new_fd);
+ ASSERT_FALSE(a.closed());
+ ASSERT_GT(a.fd(), 2);
Review Comment:
Will it fail if `stdin` or `stderr` is closed? Though I don't think it will happen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13354:
URL: https://github.com/apache/arrow/pull/13354#discussion_r893753515
##########
cpp/src/arrow/testing/gtest_util.cc:
##########
@@ -574,6 +578,40 @@ std::shared_ptr<Array> TweakValidityBit(const std::shared_ptr<Array>& array,
return MakeArray(data);
}
+// XXX create a testing/io.{h,cc}?
Review Comment:
No, I thought it would be better to not depend on `io/` from `util/`, but perhaps that's not important :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13354:
URL: https://github.com/apache/arrow/pull/13354#discussion_r893753515
##########
cpp/src/arrow/testing/gtest_util.cc:
##########
@@ -574,6 +578,40 @@ std::shared_ptr<Array> TweakValidityBit(const std::shared_ptr<Array>& array,
return MakeArray(data);
}
+// XXX create a testing/io.{h,cc}?
Review Comment:
No, I thought it would be better to not include from `io` from `util`, but perhaps that's not important :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13354:
URL: https://github.com/apache/arrow/pull/13354#issuecomment-1151460606
Revision: ddd73b3b3849751225cda78bf051419299f71fc4
Submitted crossbow builds: [ursacomputing/crossbow @ actions-177418cea1](https://github.com/ursacomputing/crossbow/branches/all?query=actions-177418cea1)
|Task|Status|
|----|------|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-build-cpp-fuzz)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-conda-cpp)|
|test-conda-cpp-valgrind|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-177418cea1-azure-test-conda-cpp-valgrind)|
|test-debian-10-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-debian-10-cpp-amd64)|
|test-debian-10-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-debian-10-cpp-i386)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-debian-11-cpp-amd64)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-debian-11-cpp-i386)|
|test-fedora-35-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-fedora-35-cpp)|
|test-ubuntu-18.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-ubuntu-18.04-cpp)|
|test-ubuntu-18.04-cpp-release|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-ubuntu-18.04-cpp-release)|
|test-ubuntu-18.04-cpp-static|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-ubuntu-18.04-cpp-static)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-ubuntu-20.04-cpp)|
|test-ubuntu-20.04-cpp-14|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-ubuntu-20.04-cpp-14)|
|test-ubuntu-20.04-cpp-17|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-ubuntu-20.04-cpp-17)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-ubuntu-20.04-cpp-bundled)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-ubuntu-20.04-cpp-thread-sanitizer)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-177418cea1-github-test-ubuntu-22.04-cpp)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on a diff in pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on code in PR #13354:
URL: https://github.com/apache/arrow/pull/13354#discussion_r894291116
##########
cpp/src/arrow/util/io_util.h:
##########
@@ -124,14 +125,46 @@ Result<bool> DeleteFile(const PlatformFilename& file_path, bool allow_not_found
ARROW_EXPORT
Result<bool> FileExists(const PlatformFilename& path);
+// TODO expose this more publicly to make it available from io/file.h?
+/// A RAII wrapper for a file descriptor.
Review Comment:
Interesting, there's one `a RAII` and one `an RAII` in below *official* RAII page, :-D
https://en.cppreference.com/w/cpp/language/raii
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13354:
URL: https://github.com/apache/arrow/pull/13354#issuecomment-1151335876
https://issues.apache.org/jira/browse/ARROW-16799
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13354:
URL: https://github.com/apache/arrow/pull/13354#discussion_r894286443
##########
cpp/src/arrow/util/io_util.h:
##########
@@ -124,14 +125,46 @@ Result<bool> DeleteFile(const PlatformFilename& file_path, bool allow_not_found
ARROW_EXPORT
Result<bool> FileExists(const PlatformFilename& path);
+// TODO expose this more publicly to make it available from io/file.h?
+/// A RAII wrapper for a file descriptor.
Review Comment:
Hmm.. I find 20k references for "a RAII" with Google and only 12k for "an RAII".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 merged pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
cyb70289 merged PR #13354:
URL: https://github.com/apache/arrow/pull/13354
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13354:
URL: https://github.com/apache/arrow/pull/13354#discussion_r894287163
##########
cpp/src/arrow/util/io_util_test.cc:
##########
@@ -159,6 +170,282 @@ TEST(WinErrorFromStatus, Basics) {
}
#endif
+class TestFileDescriptor : public ::testing::Test {
+ public:
+ Result<int> NewFileDescriptor() {
+ // Make a new fd by dup'ing C stdout (why not?)
+ int new_fd = dup(1);
+ if (new_fd < 0) {
+ return IOErrorFromErrno(errno, "Failed to dup() C stdout");
+ }
+ return new_fd;
+ }
+
+ void AssertValidFileDescriptor(int fd) {
+ ASSERT_FALSE(FileIsClosed(fd)) << "Not a valid file descriptor: " << fd;
+ }
+
+ void AssertInvalidFileDescriptor(int fd) {
+ ASSERT_TRUE(FileIsClosed(fd)) << "Unexpectedly valid file descriptor: " << fd;
+ }
+};
+
+TEST_F(TestFileDescriptor, Basics) {
+ int new_fd, new_fd2;
+
+ // Default initialization
+ FileDescriptor a;
+ ASSERT_EQ(a.fd(), -1);
+ ASSERT_TRUE(a.closed());
+ ASSERT_OK(a.Close());
+ ASSERT_OK(a.Close());
+
+ // Assignment
+ ASSERT_OK_AND_ASSIGN(new_fd, NewFileDescriptor());
+ AssertValidFileDescriptor(new_fd);
+ a = FileDescriptor(new_fd);
+ ASSERT_FALSE(a.closed());
+ ASSERT_GT(a.fd(), 2);
Review Comment:
At least CI is passing :-) We can relax if we encounter a failure IMHO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13354:
URL: https://github.com/apache/arrow/pull/13354#discussion_r893714804
##########
cpp/src/arrow/testing/gtest_util.cc:
##########
@@ -574,6 +578,40 @@ std::shared_ptr<Array> TweakValidityBit(const std::shared_ptr<Array>& array,
return MakeArray(data);
}
+// XXX create a testing/io.{h,cc}?
Review Comment:
Did this have to be moved because of a circular dependency or something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13354:
URL: https://github.com/apache/arrow/pull/13354#discussion_r893771913
##########
cpp/src/arrow/testing/gtest_util.cc:
##########
@@ -574,6 +578,40 @@ std::shared_ptr<Array> TweakValidityBit(const std::shared_ptr<Array>& array,
return MakeArray(data);
}
+// XXX create a testing/io.{h,cc}?
Review Comment:
Ah, it does feel weird to have it in gtest_util.h, but it's already rather a grab-bag of test helpers so it's not a big deal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #13354: ARROW-16799: [C++] Create a self-pipe abstraction
Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #13354:
URL: https://github.com/apache/arrow/pull/13354#issuecomment-1151455347
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org