Allow clang and gcc builds for src/e2e_test/vizier/planner:planner_test
#2077
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ddelnano
commented
Jan 14, 2025
| string percentage_label = 9; | ||
| // The column containing the stacktrace's delta. This is only relevant for differential | ||
| // flamegraphs. | ||
| string difference_column = 10; |
Member
Author
There was a problem hiding this comment.
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.
ddelnano
changed the title
src/e2e_test/vizier/planner:planner_test
ddelnano
changed the title
src/e2e_test/vizier/planner:planner_testsrc/e2e_test/vizier/planner:planner_test
ddelnano
force-pushed
the
ddelnano/fix-all-scripts-tests
branch
from
d96d31a to
2ac771d
Compare
Signed-off-by: Dom Del Nano <ddelnano@gmail.com>
Signed-off-by: Dom Del Nano <ddelnano@gmail.com>
Signed-off-by: Dom Del Nano <ddelnano@gmail.com>
Signed-off-by: Dom Del Nano <ddelnano@gmail.com>
Signed-off-by: Dom Del Nano <ddelnano@gmail.com>
ddelnano
force-pushed
the
ddelnano/fix-all-scripts-tests
branch
from
2ac771d to
91a56ea
Compare
JamesMBartlett
approved these changes
Jan 18, 2025
Member
JamesMBartlett
left a comment
JamesMBartlett
left a comment
There was a problem hiding this comment.
lgtm if it works with libcpp
| tags = [ | ||
| "no_asan", | ||
| "no_gcc", | ||
| "no_libcpp", |
Member
There was a problem hiding this comment.
Did you check if it works with libcpp?
Member
Author
There was a problem hiding this comment.
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.
ddelnano
added a commit
to ddelnano/pixie
that referenced
this pull request
Aug 6, 2025
…est` (pixie-io#2077) Summary: Allow clang and gcc builds for `src/e2e_test/vizier/planner:planner_test` As far as I can tell, there isn't a bazel config that matches the tags of the `src/e2e_test/vizier/planner:planner_test` target. This change removes the tag preventing clang builds since they appear to run without problems. In addition to this, a `vispb` proto change was missing from the earlier differential flamegraph work. This prevented the `planner_test` from running properly (see comment for more details). Relevant Issues: N/A Type of change: /kind bugfix Test Plan: Verified gcc and clang builds work - [x] `bazel test --config clang src/e2e_test/vizier/planner:planner_test` is successful - [x] `bazel test -c opt --config gcc src/e2e_test/vizier/planner:planner_test` is successful --------- Signed-off-by: Dom Del Nano <ddelnano@gmail.com> GitOrigin-RevId: 685ed42
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary: Allow clang and gcc builds for
src/e2e_test/vizier/planner:planner_testAs far as I can tell, there isn't a bazel config that matches the tags of the
src/e2e_test/vizier/planner:planner_testtarget. This change removes the tag preventing clang builds since they appear to run without problems. In addition to this, avispbproto change was missing from the earlier differential flamegraph work. This prevented theplanner_testfrom running properly (see comment for more details).Relevant Issues: N/A
Type of change: /kind bugfix
Test Plan: Verified gcc and clang builds work
bazel test --config clang src/e2e_test/vizier/planner:planner_testis successfulbazel test -c opt --config gcc src/e2e_test/vizier/planner:planner_testis successful