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

Grab-bag of fixes for OS X #1093

Merged
merged 8 commits into from
Aug 21, 2023
Rate limit · GitHub

Access has been restricted

You have triggered a rate limit.

Please wait a few minutes before you try again;
in some cases this may take up to an hour.

Merged

Grab-bag of fixes for OS X #1093

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e26c946
Use int64_t-typed literals in max().
grebe Aug 5, 2023
6aa3df0
Remove unused Mul() from bits_ops.
grebe Aug 5, 2023
134752d
Make strerror_r() call more portable.
grebe Aug 5, 2023
80b2f5a
Make subprocess tests more portable.
grebe Aug 5, 2023
99723ac
Make JIT more portable.
grebe Aug 5, 2023
6ef60de
Make log_message_test more portable.
grebe Aug 5, 2023
2d69678
Bump bazel toolchain and LLVM.
grebe Aug 5, 2023
a0528a7
Add missing override to matcher.
grebe Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Make subprocess tests more portable. 
/bin/sh on OS X is very limited. Also, do some very minor cleanup.
Rate limit · GitHub

Access has been restricted

You have triggered a rate limit.

Please wait a few minutes before you try again;
in some cases this may take up to an hour.

@grebe
grebe committed Aug 21, 2023
commit 80b2f5a5d4586dab97004264efb92d05f7ebe218
4 changes: 2 additions & 2 deletions xls/common/subprocess.cc
View file
Original file line number Diff line number Diff line change
@@ -246,8 +246,8 @@ absl::StatusOr<SubprocessResult> InvokeSubprocess(
if (!read_status.ok()) {
XLS_VLOG(1) << "ReadFileDescriptors non-ok status: " << read_status;
}
const auto& stdout_output = output_strings[0];
const auto& stderr_output = output_strings[1];
const std::string& stdout_output = output_strings[0];
const std::string& stderr_output = output_strings[1];

XLS_VLOG_LINES(2, absl::StrCat(bin_name, " stdout:\n ", stdout_output));
XLS_VLOG_LINES(2, absl::StrCat(bin_name, " stderr:\n ", stderr_output));
9 changes: 9 additions & 0 deletions xls/common/subprocess.h
View file
Original file line number Diff line number Diff line change
@@ -22,6 +22,7 @@
#include <utility>

#include "absl/status/statusor.h"
#include "absl/strings/str_format.h"
#include "absl/time/time.h"
#include "absl/types/span.h"

@@ -47,6 +48,14 @@ struct SubprocessResult {
bool timeout_expired;
};
std::ostream &operator<<(std::ostream &os, const SubprocessResult &other);
inline void PrintTo(const SubprocessResult& result, std::ostream* os) {
*os << absl::StreamFormat(
"SubprocessResult "
"{\n\tstdout=%s\n\tstderr=%s\n\texit_status=%d\n\tnormal_termination=%"
"d\n\ttimeout_expired=%d\n}}",
result.stdout, result.stderr, result.exit_status,
result.normal_termination, result.timeout_expired);
}

// Returns Status::kInternalError if the subprocess unexpectedly terminated or
// terminated with return code != 0 rather than StatusOk(). This is helpful when
16 changes: 8 additions & 8 deletions xls/common/subprocess_test.cc
View file
Original file line number Diff line number Diff line change
@@ -42,7 +42,7 @@ TEST(SubprocessTest, EmptyArgvFails) {

TEST(SubprocessTest, NonZeroExitWorks) {
auto result = InvokeSubprocess(
{"/bin/sh", "-c", "echo -n hey && echo -n hello >&2 && exit 10"},
{"/bin/bash", "-c", "echo -n hey && echo -n hello >&2 && exit 10"},
Copy link
Member

Choose a reason for hiding this comment

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

While /bin/sh is pretty universally installed on Unix-like systems, /bin/bash is not (and so this test fails now on minimal Unix systems or where bash is in a different path).
What were the things that didn't work in OSX, could we re-work it to work with the minimal bourne shell available there ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The flags passed to echo weren't supported by /bin/sh

std::nullopt);

EXPECT_THAT(result, IsOkAndHolds(FieldsAre(
@@ -55,7 +55,7 @@ TEST(SubprocessTest, NonZeroExitWorks) {

TEST(SubprocessTest, CrashingExitWorks) {
auto result = InvokeSubprocess(
{"/bin/sh", "-c", "echo -n hey && echo -n hello >&2 && kill -ABRT $$"},
{"/bin/bash", "-c", "echo -n hey && echo -n hello >&2 && kill -ABRT $$"},
std::nullopt);

EXPECT_THAT(result, IsOkAndHolds(FieldsAre(
@@ -68,7 +68,7 @@ TEST(SubprocessTest, CrashingExitWorks) {

TEST(SubprocessTest, WatchdogFastExitWorks) {
absl::Time start_time = absl::Now();
auto result = InvokeSubprocess({"/bin/sh", "-c", "exit 0"}, std::nullopt,
auto result = InvokeSubprocess({"/bin/bash", "-c", "exit 0"}, std::nullopt,
absl::Milliseconds(60000));
absl::Duration duration = absl::Now() - start_time;

@@ -82,7 +82,7 @@ TEST(SubprocessTest, WatchdogFastExitWorks) {
}

TEST(SubprocessTest, WatchdogWorks) {
auto result = InvokeSubprocess({"/bin/sh", "-c", "sleep 10s"}, std::nullopt,
auto result = InvokeSubprocess({"/bin/bash", "-c", "sleep 10"}, std::nullopt,
absl::Milliseconds(50));

EXPECT_THAT(result, IsOkAndHolds(FieldsAre(
@@ -95,7 +95,7 @@ TEST(SubprocessTest, WatchdogWorks) {

TEST(SubprocessTest, ErrorAsStatusFailingCommand) {
auto result = SubprocessErrorAsStatus(InvokeSubprocess(
{"/bin/sh", "-c", "echo hey && echo hello >&2 && /bin/false"},
{"/bin/bash", "-c", "echo hey && echo hello >&2 && /bin/false"},
std::nullopt));

ASSERT_THAT(result, StatusIs(absl::StatusCode::kInternal));
@@ -106,7 +106,7 @@ TEST(SubprocessTest, ErrorAsStatusFailingCommand) {
TEST(SubprocessTest, WorkingCommandWorks) {
absl::StatusOr<SubprocessResult> result_or_status =
SubprocessErrorAsStatus(InvokeSubprocess(
{"/bin/sh", "-c", "echo hey && echo hello >&2"}, std::nullopt));
{"/bin/bash", "-c", "echo hey && echo hello >&2"}, std::nullopt));

XLS_ASSERT_OK(result_or_status);
EXPECT_EQ(result_or_status->stdout, "hey\n");
@@ -116,7 +116,7 @@ TEST(SubprocessTest, WorkingCommandWorks) {
TEST(SubprocessTest, LargeOutputToStdoutFirstWorks) {
absl::StatusOr<SubprocessResult> result_or_status =
SubprocessErrorAsStatus(InvokeSubprocess(
{"/bin/sh", "-c", "/usr/bin/env seq 10000 && echo hello >&2"},
{"/bin/bash", "-c", "/usr/bin/env seq 10000 && echo hello >&2"},
std::nullopt));

XLS_ASSERT_OK(result_or_status);
@@ -127,7 +127,7 @@ TEST(SubprocessTest, LargeOutputToStdoutFirstWorks) {
TEST(SubprocessTest, LargeOutputToStderrFirstWorks) {
absl::StatusOr<SubprocessResult> result_or_status =
SubprocessErrorAsStatus(InvokeSubprocess(
{"/bin/sh", "-c", "/usr/bin/env seq 10000 >&2 && echo hello"},
{"/bin/bash", "-c", "/usr/bin/env seq 10000 >&2 && echo hello"},
std::nullopt));

XLS_ASSERT_OK(result_or_status);