feat: Add build info #278
feat: Add build info #278
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #278 +/- ##
==========================================
- Coverage 88.48% 88.46% -0.02%
==========================================
Files 42 42
Lines 1346 1344 -2
==========================================
- Hits 1191 1189 -2
Misses 155 155
Continue to review full report at Codecov.
|
devjgm
changed the title
There was a problem hiding this comment.
Reviewed 7 of 13 files at r1, 6 of 6 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @devjgm)
bazel/compiler_id.sh, line 2 at r2 (raw file):
#!/bin/bash #
nit: add the copyright boiler plate here.
bazel/compiler_id.sh, line 26 at r2 (raw file):
*msvc* | *microsoft*) echo "Clang"
s/Clang/MSVC/ ? And does MSVC actually understand --version? Asking for a friend.
bazel/compiler_version.sh, line 2 at r2 (raw file):
#!/bin/bash #
nit: boilerplate here.
ci/kokoro/docker/build.sh, line 123 at r2 (raw file):
elif [[ "${BUILD_NAME}" = "coverage" ]]; then export BUILD_TYPE=Coverage export CODE_COVERAGE=yes
I think we do not need CODE_COVERAGE anymore?
google/cloud/spanner/BUILD, line 29 at r2 (raw file):
CC_VERSION=$$($(location //bazel:compiler_version.sh) $(CC)); CC_ID=$$($(location //bazel:compiler_id.sh) $(CC)); sed -e "s;@CMAKE_CXX_COMPILER_ID@;$${CC_ID};" \
Just a thought, but maybe we should refactor this to a script too? Or we could consider something like:
https://github.com/bazelbuild/examples/blob/master/rules/expand_template/hello.bzl
google/cloud/spanner/internal/build_info.cc.in, line 42 at r2 (raw file):
} std::string CompilerFeatures() {
No action required.
I never thought about this before, but this function and the next could be moved to a regular .cc file instead of this .cc.in file.
google/cloud/spanner/internal/build_info.cc.in, line 87 at r2 (raw file):
std::string BuildMetadata() { return R"""(sha.@GOOGLE_CLOUD_CPP_SPANNER_BUILD_METADATA@)""";
Since this can be something other than a SHA-1 (when the application defines it), should sha. be there?
| /** | ||
| * Returns the compiler flags. | ||
| * | ||
| * Examples include "-c fastbuild" or "-O2 -DNDEBUG". |
There was a problem hiding this comment.
Is -c fastbuild really a compiler flag?
There was a problem hiding this comment.
Is -c fastbuild really a compiler flag?
We don't have the actual compiler flags that were used ... so we use the COMPILATION_MODE as a proxy.
OK. Then I'll only suggest that something like BuildFlags() might be a better name.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @coryan and @devbww)
bazel/compiler_id.sh, line 2 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
nit: add the copyright boiler plate here.
done.
bazel/compiler_id.sh, line 7 at r2 (raw file):
Previously, devbww (Bradley White) wrote…
What does existence have to do with usage?
Just since we're about to execute it... ?? Let me know if you want me to change it to something else.
bazel/compiler_id.sh, line 26 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
s/Clang/MSVC/ ? And does MSVC actually understand
--version? Asking for a friend.
Actually, I have no idea how to get the msvc compiler version. Filed an issue and added a comment in the code.
bazel/compiler_version.sh, line 2 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
nit: boilerplate here.
Done.
ci/kokoro/docker/build.sh, line 123 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
I think we do not need
CODE_COVERAGEanymore?
Good point. Fixed. Also fixed upload-coverage.sh
google/cloud/spanner/BUILD, line 29 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Just a thought, but maybe we should refactor this to a script too? Or we could consider something like:
https://github.com/bazelbuild/examples/blob/master/rules/expand_template/hello.bzl
I think factoring this out to a separate command may be a good idea. It gets a little less nice in some ways, because since we need to use bazel's $(location <target>) command, we'll need to do that outside of the script and pass the results into the script. I don't think this is particularly any worse than it was before, so I'm inclined to keep this as-is, and maybe factor out into a separate command in the future.
google/cloud/spanner/internal/build_info.h, line 44 at r2 (raw file):
Previously, devbww (Bradley White) wrote…
Is
-c fastbuildreally a compiler flag?
Well, it's a bazel flag. We don't have the actual compiler flags that were used. This comes from the genrule where we're trying to to extract info from bazel to fill in the ${CMAKE_CXX_FLAGS_(DEBUG|RELEASE}} variable. We don't have the real flags that bazel is using, so we use the COMPILATION_MODE as a proxy.
Think we should drop this?
google/cloud/spanner/internal/build_info.cc.in, line 42 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
No action required.
I never thought about this before, but this function and the next could be moved to a regular
.ccfile instead of this.cc.infile.
Yes, I think they could now since they're split apart. Previously they were part of other functions ((like language_version()) that also used the cmake template patterns.
I'm thinking maybe in a future PR.
google/cloud/spanner/internal/build_info.cc.in, line 87 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Since this can be something other than a SHA-1 (when the application defines it), should
sha.be there?
In general, I think "build metadata" can be more than just a sha, but afaict, we only ever use it as a sha or the string "unknown". The documentation at https://semver.org/#spec-item-10 is lacking IMO, but it does have a couple examples, and they use the "sha." prefix when the value is a sha.
I was tempted to rename this function to BuildSha() or BuildCommit() to make the meaning and intent clear. What the function returns is the build SHA. And we might use it somewhere as a semver "metadata".
I still think renaming this function may be a good idea.
Thoughts?
| exit 1 | ||
| fi | ||
|
|
||
| version="$("${1}" --version 2> /dev/null \ |
There was a problem hiding this comment.
If we're going to support multiple compilers, the script will probably grow larger as we detect the executable and use the appropriate switch/flags. Just spitballing, but what do you think about adopting an approach similar to this? TL;DR, compile a program in Bazel that uses compiler-specific version macros then spits out the information.
There was a problem hiding this comment.
Reviewable status: 6 of 14 files reviewed, 8 unresolved discussions (waiting on @coryan, @devbww, and @devjgm)
bazel/compiler_id.sh, line 7 at r2 (raw file):
Previously, devjgm (Greg Miller) wrote…
Just since we're about to execute it... ?? Let me know if you want me to change it to something else.
On second though, you're right. Changed to a more normal $# -ne 1 check. Fixed. thanks.
There was a problem hiding this comment.
Reviewed 3 of 5 files at r3, 5 of 5 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @coryan, @devbww, @devjgm, and @remyabel)
bazel/compiler_id.sh, line 26 at r3 (raw file):
Previously, remyabel wrote…
If we're going to support multiple compilers, the script will probably grow larger as we detect the executable and use the appropriate switch/flags. Just spitballing, but what do you think about adopting an approach similar to this? TL;DR, compile a program in Bazel that uses compiler-specific version macros then spits out the information.
I am starting to like @remyabel's approach, though we are repeating work that CMake does, we are doomed to repeat it to support Bazel.
And if we are detecting the compiler make and model in C++ then I suggest we change as much as we can from build_info.cc.in into a .cc and do the detection once in a lambda. Can be done if a future PR.
google/cloud/spanner/internal/build_info.h, line 44 at r2 (raw file):
Previously, devjgm (Greg Miller) wrote…
Well, it's a bazel flag. We don't have the actual compiler flags that were used. This comes from the genrule where we're trying to to extract info from bazel to fill in the
${CMAKE_CXX_FLAGS_(DEBUG|RELEASE}}variable. We don't have the real flags that bazel is using, so we use theCOMPILATION_MODEas a proxy.Think we should drop this?
Please try not to drop this, I need this information in the benchmarks. Knowing how the code was compiled matters there. Maybe this should be called CompilationMode() instead ?
google/cloud/spanner/internal/build_info.cc.in, line 87 at r2 (raw file):
Previously, devjgm (Greg Miller) wrote…
In general, I think "build metadata" can be more than just a sha, but afaict, we only ever use it as a sha or the string "unknown". The documentation at https://semver.org/#spec-item-10 is lacking IMO, but it does have a couple examples, and they use the "sha." prefix when the value is a sha.
I was tempted to rename this function to
BuildSha()orBuildCommit()to make the meaning and intent clear. What the function returns is the build SHA. And we might use it somewhere as a semver "metadata".I still think renaming this function may be a good idea.
Thoughts?
All I meant is that the the string as a sha. embedded in it, and we probably do not want that?
There was a problem hiding this comment.
Reviewable status: 12 of 16 files reviewed, 5 unresolved discussions (waiting on @coryan, @devbww, and @remyabel)
google/cloud/spanner/internal/build_info.cc.in, line 87 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
All I meant is that the the string as a
sha.embedded in it, and we probably do not want that?
Yes, that's what I was responding to. But maybe I'm still confused.
https://semver.org/#spec-item-10 says
"Build metadata MAY be denoted by appending a plus sign and a series of dot separated identifiers immediately following the patch or pre-release version"
and then they give an example of
"1.0.0-beta+exp.sha.5114f85"
And so since our @GOOGLE_CLOUD_CPP_SPANNER_BUILD_METADATA@ is always a sha, it seemed like we would want to keep the "sha." prefix.
Sorry if I'm misunderstanding this.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @devbww, @devjgm, and @remyabel)
.bazelrc, line 15 at r5 (raw file):
# limitations under the License. build --workspace_status_command=bazel/workspace_status.sh
When the library is used as a dependency of a larger Bazel project the .bazelrc file is not parsed, so this will have no effect.
So this may not be what we want?
google/cloud/spanner/internal/build_info.cc.in, line 87 at r2 (raw file):
Previously, devjgm (Greg Miller) wrote…
Yes, that's what I was responding to. But maybe I'm still confused.
https://semver.org/#spec-item-10 says
"Build metadata MAY be denoted by appending a plus sign and a series of dot separated identifiers immediately following the patch or pre-release version"and then they give an example of
"1.0.0-beta+exp.sha.5114f85"
And so since our
@GOOGLE_CLOUD_CPP_SPANNER_BUILD_METADATA@is always a sha, it seemed like we would want to keep the "sha." prefix.Sorry if I'm misunderstanding this.
The user can say:
cmake -DGOOGLE_CLOUD_CPP_SPANNER_BUILD_METADATA=auto_build_on_20190802 -H... -B...Which is not really a SHA, it is just an annotation.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @coryan, @devbww, and @remyabel)
.bazelrc, line 15 at r5 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
When the library is used as a dependency of a larger Bazel project the .bazelrc file is not parsed, so this will have no effect.
So this may not be what we want?
I have such a love-hate relationship with bazel. :-)
OK, so I have no idea how to get git commit stamps into bazel builds consistently. So I've removed that functionality. Now, bazel builds will always say "unknown" for the git commit hash (which is exposed via the BuildMetadata() function).
After I commit this PR, I'll file an issue linking to this code to to track figuring this out in the future (if we can).
bazel/compiler_id.sh, line 7 at r2 (raw file):
Previously, devbww (Bradley White) wrote…
Just since we're about to execute it... ?? Let me know if you want me to change it to something else.
On second though, you're right. Changed to a more normal $# -ne 1 check. Fixed. thanks.
Alright!
Done.
bazel/compiler_id.sh, line 26 at r3 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
I am starting to like @remyabel's approach, though we are repeating work that CMake does, we are doomed to repeat it to support Bazel.
And if we are detecting the compiler make and model in C++ then I suggest we change as much as we can frombuild_info.cc.ininto a.ccand do the detection once in a lambda. Can be done if a future PR.
Sounds good. After I merge this PR, I'll file an issue to track this idea and have the issue refer to this code.
google/cloud/spanner/internal/build_info.h, line 44 at r2 (raw file):
Previously, devbww (Bradley White) wrote…
Is -c fastbuild really a compiler flag?
We don't have the actual compiler flags that were used ... so we use the COMPILATION_MODE as a proxy.
OK. Then I'll only suggest that something like
BuildFlags()might be a better name.
SGTM. s/CompilerFlags/BuildFlags/g
google/cloud/spanner/internal/build_info.cc.in, line 87 at r2 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
The user can say:
cmake -DGOOGLE_CLOUD_CPP_SPANNER_BUILD_METADATA=auto_build_on_20190802 -H... -B...Which is not really a SHA, it is just an annotation.
OK. Removed the "sha." prefix.
There was a problem hiding this comment.
Reviewed 5 of 6 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @devbww and @remyabel)
Fixes googleapis/google-cloud-cpp-spanner#81 This PR mostly follows the "build_info" pattern established by https://github.com/googleapis/google-cloud-cpp/blob/master/google/cloud/internal/build_info.h with the following notable differences: Some functions are spelled differently. The naming follows C++ style convention (e.g., LanguageVersion() instead of language_version()) to avoid the need for NOLINT. Some functions have a different name to make their intent and semantics clearer. For example, rather than LanguageVersion() returning all of the compiler ID, the compiler version, and the language version, those parts are all returned by 3 clearly named separate functions. The bazel build uses scripts in the //bazel/ directory to extract the "compiler ID" and "compiler version" in a format that is compatible with the format provided by CMake. Bazel builds always report the git commit has as "unknown", because I don't know of any way to correctly extract this info from within a bazel build rule. I don't think this is actually a change, because I believe the GCS and BigTable bazel builds also always report "unknown" too.
Fixes #81
This PR mostly follows the "build_info" pattern established by https://github.com/googleapis/google-cloud-cpp/blob/master/google/cloud/internal/build_info.h with the following notable differences:
LanguageVersion()instead oflanguage_version()) to avoid the need for NOLINT.LanguageVersion()returning all of the compiler ID, the compiler version, and the language version, those parts are all returned by 3 clearly named separate functions.This change is