Skip to content

Comments

build: Adds a envoy_select_signal_trace Bazel macro#27155

Merged
abeyad merged 9 commits intoenvoyproxy:mainfrom
abeyad:select_signal_trace
May 4, 2023
Merged

build: Adds a envoy_select_signal_trace Bazel macro#27155
abeyad merged 9 commits intoenvoyproxy:mainfrom
abeyad:select_signal_trace

Conversation

@abeyad
Copy link
Contributor

@abeyad abeyad commented May 3, 2023

No description provided.

Signed-off-by: Ali Beyad <abeyad@google.com>
abeyad added 2 commits May 3, 2023 12:08
Signed-off-by: Ali Beyad <abeyad@google.com>
Signed-off-by: Ali Beyad <abeyad@google.com>
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for doing this. A couple of minor questions.

# Selects the given values if signal trace is enabled in the current build.
def envoy_select_signal_trace(xs, repository = ""):
return select({
repository + "//bazel:disable_signal_trace": [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we add the mobile-specific logic here too, or do you want to do so in a followup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, we can't add the mobile targets b/c disable_signal_trace is already set via .bazelrc and on CI.
(discussed this with Ryan offline)
So I reverted the change and put it back to the original.

}),
] + envoy_select_signal_trace(
["@envoy//source/common/signal:sigaction_lib"],
"@envoy",
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it is, for the reasons mentioned above

Signed-off-by: Ali Beyad <abeyad@google.com>
RyanTheOptimist
RyanTheOptimist previously approved these changes May 3, 2023
Signed-off-by: Ali Beyad <abeyad@google.com>
@abeyad abeyad enabled auto-merge (squash) May 3, 2023 17:29
@abeyad abeyad disabled auto-merge May 3, 2023 17:29
RyanTheOptimist
RyanTheOptimist previously approved these changes May 3, 2023
@abeyad
Copy link
Contributor Author

abeyad commented May 3, 2023

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #27155 (comment) was created by @abeyad.

see: more, trace.

@abeyad
Copy link
Contributor Author

abeyad commented May 3, 2023

added @keith for cross-company review

Signed-off-by: Ali Beyad <abeyad@google.com>
keith
keith previously approved these changes May 4, 2023
Copy link
Member

@keith keith left a comment

Choose a reason for hiding this comment

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

looks good!

.bazelrc Outdated
build:remote --google_default_credentials=true
build:remote --remote_download_toplevel
build:remote --nobuild_runfile_links
# build:remote --nobuild_runfile_links
Copy link
Member

Choose a reason for hiding this comment

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

unintentional check in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

undid this just now, sorry for the confusion! i was just trying out undoing a phlax change that was causing CI to fail. phlax is going to send in a fix for it tomorrow. thanks for reviewing!

Signed-off-by: Ali Beyad <abeyad@google.com>
@RyanTheOptimist
Copy link
Contributor

Now that phlax's PR has landed, I think this just needs a main merge.

@abeyad abeyad merged commit e2a40ea into envoyproxy:main May 4, 2023
@abeyad abeyad deleted the select_signal_trace branch May 4, 2023 15:20
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
Signed-off-by: Ali Beyad <abeyad@google.com>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants