Replace git_sha_rewriter.py with a generated linker option.#3021
Conversation
jmillikin-stripe
force-pushed
the
rm-git-sha-rewriter
branch
2 times, most recently
from
809af57 to
43dcd4a
Compare
|
Approach at a high level LGTM. Assigning to @htuch for review. @jmillikin-stripe looks like there are still some CI issues. |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
This is awesome @jmillikin-stripe, nice to see this landing.
There was a problem hiding this comment.
How does this work when Envoy is used as a dependency in another project to assemble a binary? Are there any issues with relative paths? Can you validate with envoy-filter-example?
There was a problem hiding this comment.
I don't fully understand how the build-id feature is being used now, so I don't know what the desired behavior is when building from a higher-level repo.
Do you want the build ID to be Envoy's commit ID, or the commit ID of the root workspace? What if it's not being built from a Git repo -- should we make the build ID empty, or leave the flag unset, or ...?
There was a problem hiding this comment.
@mattklein123 or other Lyfters will have to answer this one, as they have a similar setup to envoy-filter-example and use this for production build stamping.
There was a problem hiding this comment.
I think right now it's the SHA of the envoy repo. Optimally, it would be the SHA of the owning repo, but whatever is fine with us as long as we have something.
There was a problem hiding this comment.
I've tweaked this a bit so it'll always use BUILD_SCM_REVISION, which should be compatible with most/all external repos. This means the decision to relink on a Git status change will be left up to the root workspace.
There was a problem hiding this comment.
Curious if you think we can add a test to avoid regressions such as #2551?
There was a problem hiding this comment.
Not that I know of. Maybe we could hack something together in CI to add a fake empty commit, relink, and run envoy --version? Seems kinda fragile.
There was a problem hiding this comment.
FYI: I reverted changes to this file so we don't need to rename SCM status keys in everything that might build Envoy as a dependency.
There was a problem hiding this comment.
What is stamp doing here and in the other genrules? I can't see much in the way of docs for genrule+stamp?
There was a problem hiding this comment.
stamp=1 gives the rule access to the bazel-out/stable-status.txt and bazel-out/volatile-status.txt files. It also makes the genrule depend directly on the stable status, so it'll re-run if the status changes.
The param is undocumented :( : bazelbuild/bazel#4942
There was a problem hiding this comment.
Wondering if this needs to be @envoy//bazel... or similar in the envoy-filter-example scenario.
There was a problem hiding this comment.
I successfully built envoy-filter-example pointing to an Envoy src dir with my changes, and it has the correct build ID.
|
Out of curiosity, how was the It might make more sense to leave this flag alone and try to embed the version info as normal symbols. |
This is how I've always done it in the past. It gets embedded in core dumps, etc. and is easily accessible using normal tools. If there is a different way we can potentially change, but the main requirement is easily accessible in core dumps. |
|
@jmillikin-stripe friendly ping on this one. |
Fixes envoyproxy#2551 uses genrule workaround for missing STABLE_ keys in cc linkstamp, more details at bazelbuild/bazel#1992 Signed-off-by: John Millikin <jmillikin@stripe.com>
jmillikin-stripe
force-pushed
the
rm-git-sha-rewriter
branch
from
43dcd4a to
5975ae6
Compare
Fixes envoyproxy#2625 Signed-off-by: John Millikin <jmillikin@stripe.com>
Signed-off-by: John Millikin <jmillikin@stripe.com>
jmillikin-stripe
force-pushed
the
rm-git-sha-rewriter
branch
from
5975ae6 to
786acc7
Compare
|
Sorry for the delay, got nerdsniped into a non-Envoy project last week. Comments addressed. |
| | sed 's/^BUILD_SCM_REVISION //') \\ | ||
| > $@ | ||
| """, | ||
| stamp = 1, |
There was a problem hiding this comment.
Can you leave a comment on the undocumented stamp semantics and the link the GH issue? I feel this will be useful when later revisiting these rules.
Signed-off-by: John Millikin <jmillikin@stripe.com>
|
@jmillikin-stripe can you push an empty commit now that DCO bot is fixed? |
Signed-off-by: John Millikin <jmillikin@stripe.com>
|
Thanks! |
I tested envoyproxy#3021 with envoy-filter-example to verify it works for non-default workspace roots, but didn't realize the filter example's envoy binary is built without linkstamping so the testing was worthless. Experimental evidence is that the `deps` field needs `@envoy` but the linker options are fine without it. I don't understand why this would be true so I'm going to put `@envoy` in both places. Signed-off-by: John Millikin <jmillikin@stripe.com>
I tested #3021 with envoy-filter-example to verify it works for non-default workspace roots, but didn't realize the filter example's envoy binary is built without linkstamping so the testing was worthless. Experimental evidence is that the `deps` field needs `@envoy` but the linker options are fine without it. I don't understand why this would be true so I'm going to put `@envoy` in both places. Signed-off-by: John Millikin <jmillikin@stripe.com>
4 participants
Description:
This replaces Envoy's use of a Python script to poke at raw ELF headers with a generated
--build-idlinker flag, following the example of @oquenchil in #2625.Note: this means the
//source/exe:envoy-statictarget is now stamped, instead of having a separate implicitenvoy-static_stampedtarget.I also edited the workspace status command so it treated the Git commit ID as "stable" instead of "volatile", so Bazel won't ignore it when relinking.
Risk Level: Low
Testing:
Tested locally on a MacOS and Linux build. Both of them have the current Git commit ID embedded in the expected binary sections.
Fixes #2551
Fixes #2625