-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
VersionInfo: allow non-linkstamp rules to link in definitions #6803
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -251,6 +251,23 @@ genrule( | |
cmd = """echo "#define BUILD_VERSION_NUMBER \\"$$(cat $<)\\"" >$@""", | ||
) | ||
|
||
genrule( | ||
name = "generate_version_linkstamp", | ||
outs = ["lib/version_linkstamp.h"], | ||
cmd = "$(location //source/common/common:generate_version_linkstamp.sh) >> $@", | ||
# Undocumented attr to depend on workspace status files. | ||
# https://github.com/bazelbuild/bazel/issues/4942 | ||
# Used here because generate_version_linkstamp.sh depends on the workspace status files. | ||
stamp = 1, | ||
tools = ["//source/common/common:generate_version_linkstamp.sh"], | ||
) | ||
|
||
genrule( | ||
name = "generate_version_linkstamp_empty", | ||
outs = ["empty/version_linkstamp.h"], | ||
cmd = """>$@""", | ||
) | ||
|
||
envoy_cc_library( | ||
name = "version_includes", | ||
hdrs = [ | ||
|
@@ -262,11 +279,21 @@ envoy_cc_library( | |
envoy_cc_library( | ||
name = "version_lib", | ||
srcs = ["version.cc"], | ||
hdrs = select({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason I had to put them in hdrs is because as far as I could tell (empirically and from docs) is that |
||
"//bazel:manual_stamp": [":generate_version_linkstamp"], | ||
# By default the header file is empty. | ||
# This is done so that the definitions linked via the linkstamp rule don't cause collisions. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that nothing prevents someone from running a rule that respects |
||
"//conditions:default": [":generate_version_linkstamp_empty"], | ||
}), | ||
copts = envoy_select_boringssl( | ||
["-DENVOY_SSL_VERSION=\\\"BoringSSL-FIPS\\\""], | ||
["-DENVOY_SSL_VERSION=\\\"BoringSSL\\\""], | ||
), | ||
linkstamp = "version_linkstamp.cc", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mattklein123 as I have implemented here, this PR does not solve for #2181 for two reasons:
I am starting to think that the best way to resolve #2181 is to include a However, I am open to fixing by more bazel work here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @dio |
||
strip_include_prefix = select({ | ||
"//bazel:manual_stamp": "lib", | ||
"//conditions:default": "empty", | ||
}), | ||
deps = [ | ||
":version_includes", | ||
"//source/common/common:macros", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
#!/bin/bash | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have any bash in source. Open to moving this elsewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
# This script generates a header file that is used by version_lib whenever linkstamp is not allowed. | ||
# linkstamp is used to link in version_linkstamp.cc into the version_lib. | ||
# However, linkstamp is not available to non-binary bazel targets. | ||
junr03 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# This means that if the topmost target being used to compile version_lib is a envoy_cc_library or related, linkstamp will not be in effect. | ||
# In turn this means that version_linkstamp.cc is not linked, and the build_scm_revision and build_scm_status are unknown symbols to the linker. | ||
build_scm_revision=$(grep BUILD_SCM_REVISION bazel-out/volatile-status.txt | sed 's/^BUILD_SCM_REVISION //' | tr -d '\\n') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this work when Envoy is included transitively in a project? I'm thinking something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As long as the outer project is a git repo or it has a SOURCE_VERSION file it works the same as the current flow via linkstamp, i.e it uses the outer project's get_workspace_status command. In Envoy mobile this is set to Envoy's script -- envoy/bazel/get_workspace_status. |
||
|
||
echo "extern const char build_scm_revision[];" | ||
echo "extern const char build_scm_status[];" | ||
echo "const char build_scm_revision[] = \"$build_scm_revision\";" | ||
echo "const char build_scm_status[] = \"Library\";" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open to better names