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

Allow clang and gcc builds for src/e2e_test/vizier/planner:planner_test #2077

Merged
merged 5 commits into from
Jan 20, 2025
Merged

Allow clang and gcc builds for src/e2e_test/vizier/planner:planner_test #2077

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a4a392b
Remove restriction with no_libcpp as tests seems to run fine
ddelnano Jan 14, 2025
c486c81
Fix issue with script decoding due to mising difference_column field
ddelnano Jan 14, 2025
955ca1b
Update generated protos
ddelnano Jan 14, 2025
2b36223
Fix gcc build
ddelnano Jan 16, 2025
91a56ea
Fix linting
ddelnano Jan 16, 2025
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
303 changes: 180 additions & 123 deletions src/api/proto/vispb/vis.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions src/api/proto/vispb/vis.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,4 +458,7 @@ message StackTraceFlameGraph {
string node_column = 8;
// The label for the percentage.
string percentage_label = 9;
// The column containing the stacktrace's delta. This is only relevant for differential
// flamegraphs.
string difference_column = 10;
Copy link
Member Author

Choose a reason for hiding this comment

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

The recent differential flamegraph changes weren't made to this file. Without this change, the test target fails with the following error:

$ bazel test -c dbg src/e2e_test/vizier/planner:planner_test --test_output=all

INFO: From Testing //src/e2e_test/vizier/planner:planner_test:
==================== Test output for //src/e2e_test/vizier/planner:planner_test:
--- FAIL: TestAllScriptsCompile (0.04s)
    all_scripts_test.go:322:
                Error Trace:    src/e2e_test/vizier/planner/all_scripts_test.go:322
                Error:          Received unexpected error:
                                can't unmarshal Any nested proto *vispb.StackTraceFlameGraph: unknown field "differenceColumn" in vispb.StackTraceFlameGraph
                Test:           TestAllScriptsCompile
FAIL

The flamegraph script has been in a cloud release for a while now and I've used it extensively, so it appears that only the e2e tests here use this proto. Typescript does match on the proto package name, but it has its own data structure for working with it.

}
8 changes: 7 additions & 1 deletion src/common/exec/exec.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,14 @@

namespace px {

// GCC's ingnored-attribute warning is triggered without wrapping pclose. This is likely do to the
// nonnull attribute.
struct pclose_deleter {
void operator()(FILE* file) const { pclose(file); }
};

StatusOr<std::string> Exec(std::string cmd) {
std::unique_ptr<FILE, decltype(&pclose)> pipe(popen(cmd.c_str(), "r"), pclose);
std::unique_ptr<FILE, pclose_deleter> pipe(popen(cmd.c_str(), "r"));
if (pipe == nullptr) {
return error::Internal("popen() failed!");
}
Expand Down
2 changes: 0 additions & 2 deletions src/e2e_test/vizier/planner/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ pl_go_test(
],
tags = [
"no_asan",
"no_gcc",
"no_libcpp",
Copy link
Member

Choose a reason for hiding this comment

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

Did you check if it works with libcpp?

Copy link
Member Author

Choose a reason for hiding this comment

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

bazel build (without specifying a --config) couldn't find this test target until the no_libcpp was removed. Therefore I thought using libcpp was the default, however, I've tested that --config clang results in a successful build as well.

"no_msan",
"no_tsan",
],
Expand Down
6 changes: 6 additions & 0 deletions src/ui/src/types/generated/vis_pb.d.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions src/ui/src/types/generated/vis_pb.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading