Output library / schema versions in JSON context block #1742

LebedevRI · 2024-01-23T23:43:00Z

Mainly, i want `get_git_version()` to return true version, not something sanitized.

LebedevRI · 2024-01-24T03:12:26Z

src/json_reporter.cc

+#if defined(BENCHMARK_VERSION)
+  const char library_version[] = BENCHMARK_VERSION;
+#else
+  const char library_version[] = "hello, bazel!";


I don't quite know what should be done for non-CMake.

in bazel we could add a define to the cc_library that maps to module_version() i think: https://bazel.build/rules/lib/toplevel/native#module_version

Yeah, i have no idea how to actually write that though. I've tried the obvious approach, but it may not work.

did you try native.module_version()? i think you were close. if it doesn't work we'll go with cmake and i'll hack on bazel.

Yes, i tried that first.

src/json_reporter.cc

LebedevRI · 2024-01-24T12:26:52Z

Let me look into that...

LebedevRI · 2024-01-24T12:47:52Z

Sorry, i have no clue how to do the bazel part.

dmah42 · 2024-01-24T12:53:55Z

Sorry, i have no clue how to do the bazel part.

blech ok. do cmake, i'll take bazel.

LebedevRI · 2024-01-24T12:53:56Z

With

        "BENCHMARK_VERSION=\""+native.module_version()+"\"",

we get

ERROR: /home/runner/work/benchmark/benchmark/BUILD.bazel:71:32: name 'native' is not defined

LebedevRI · 2024-01-24T12:55:55Z

With

        "BENCHMARK_VERSION=\""+module_version()+"\"",

(which is the right syntax i guess?) we get

ERROR: Traceback (most recent call last):
	File "/home/runner/work/benchmark/benchmark/BUILD.bazel", line 71, column 31, in <toplevel>
		"BENCHMARK_VERSION=\""+module_version()+"\"",
Error: unsupported binary operation: string + NoneType

Does that mean there is no version string?
But MODULE.bazel has

module(
    name = "google_benchmark",
    version = "1.8.3",
)

dmah42 · 2024-01-24T13:12:02Z

With

        "BENCHMARK_VERSION=\""+module_version()+"\"",

(which is the right syntax i guess?) we get

ERROR: Traceback (most recent call last):
	File "/home/runner/work/benchmark/benchmark/BUILD.bazel", line 71, column 31, in <toplevel>
		"BENCHMARK_VERSION=\""+module_version()+"\"",
Error: unsupported binary operation: string + NoneType

Does that mean there is no version string? But MODULE.bazel has

module(
    name = "google_benchmark",
    version = "1.8.3",
)

https://bazel.build/rules/lib/toplevel/native#module_version

"If this package is from a repo defined in WORKSPACE instead of MODULE.bazel, this is empty. "

i guess we have both WORKSPACE and MODULE.bazel so it's returning no version?

LebedevRI · 2024-01-24T13:25:54Z

Right, i guess that makes sense, but it does not really tell me how to fix this. Sorry. :/

dmah42 · 2024-01-24T13:34:25Z

Right, i guess that makes sense, but it does not really tell me how to fix this. Sorry. :/

don't be. drop bazel, i'll deal with it. i think i know what we need to do.

LebedevRI · 2024-01-24T14:15:35Z

src/json_reporter.cc

+  out << ",\n";
+
+  // NOTE: our json schema is not strictly tied to the library version!
+  out << indent << FormatKV("json_schema_version", int64_t(1));


Also, do we want the original (current) version to be 1 or 0 ?

current version is v1.8.3 so v1 to match the major.

dmah42 · 2024-01-29T13:20:18Z

just to confirm: with bzlmod enabled, this gets further. two things missing:

convert "none" to an empty string in BUILD.bazel for when bzlmod is disabled
figure out why it's failing with:

/usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer '-std=c++14' -MD -MF bazel-out/k8-fastbuild/bin/_objs/benchmark/benchmark.pic.d '-frandom-seed=bazel-out/k8-fastbuild/bin/_objs/benchmark/benchmark.pic.o' -fPIC -DBENCHMARK_STATIC_DEFINE '-DBENCHMARK_VERSION=1.8.3' '-D_FILE_OFFSET_BITS=64' -D_LARGEFILE64_SOURCE -D_LARGEFILE_SOURCE -iquote . -iquote bazel-out/k8-fastbuild/bin -Ibazel-out/k8-fastbuild/bin/_virtual_includes/benchmark '-Werror=old-style-cast' -fno-canonical-system-headers -Wno-builtin-macro-redefined '-D__DATE__="redacted"' '-D__TIMESTAMP__="redacted"' '-D__TIME__="redacted"' -c src/benchmark.cc -o bazel-out/k8-fastbuild/bin/_objs/benchmark/benchmark.pic.o)

<command-line>: error: too many decimal points in number
src/benchmark.cc:753:11: note: in expansion of macro 'BENCHMARK_VERSION'
  753 |   return {BENCHMARK_VERSION};
      |           ^~~~~~~~~~~~~~~~~
src/benchmark.cc: In function 'std::string benchmark::GetBenchmarkVersiom()':
src/benchmark.cc:753:28: error: could not convert '{<expression error>}' from '<brace-enclosed initializer list>' to 'std::string' {aka 'std::__cxx11::basic_string<char>'}
  753 |   return {BENCHMARK_VERSION};
      |                            ^
      |                            |
      |                            <brace-enclosed initializer list>

LebedevRI · 2024-01-29T13:23:46Z

figure out why it's failing with:

It needs to be -DBENCHMARK_VERSION="string" not -DBENCHMARK_VERSION=string.

LebedevRI · 2024-01-29T13:24:01Z

@dmah42 thank you!

dmah42 · 2024-01-29T13:25:34Z

i just kept adding \ and it works.

        "BENCHMARK_VERSION=\\\"" + module_version() + "\\\"",

google#1742 changed the placeholder version from `0.0.0` to `v0.0.0`, but this line which was further dealing with it, was not updated. Fixes google#1792

#1742 changed the placeholder version from `0.0.0` to `v0.0.0`, but this line which was further dealing with it, was not updated. Fixes #1792 Co-authored-by: dominic <510002+dmah42@users.noreply.github.com>

CMake: get_git_version(): just use --dirty flag of git describe

883279e

LebedevRI requested a review from dmah42 January 23, 2024 23:43

LebedevRI added 2 commits January 24, 2024 02:56

CMake: move version normalization out of get_git_version()

3894f49

Mainly, i want `get_git_version()` to return true version, not something sanitized.

JSON reporter: store library version and schema version in context

682bac9

LebedevRI force-pushed the json-version branch from 27cb27a to a063fc7 Compare January 23, 2024 23:57

Tools: discard inputs with unexpected json_schema_version

341783b

LebedevRI force-pushed the json-version branch from a063fc7 to 341783b Compare January 24, 2024 00:00

LebedevRI commented Jan 24, 2024

View reviewed changes

dmah42 reviewed Jan 24, 2024

View reviewed changes

src/json_reporter.cc Outdated Show resolved Hide resolved

Extract version string into GetBenchmarkVersiom()

a7a5822

LebedevRI force-pushed the json-version branch 2 times, most recently from 2737d16 to d4d344b Compare January 24, 2024 12:40

LebedevRI force-pushed the json-version branch from 12861f7 to 84f98b2 Compare January 24, 2024 12:52

LebedevRI force-pushed the json-version branch from 5407781 to a7a5822 Compare January 24, 2024 13:36

LebedevRI commented Jan 24, 2024

View reviewed changes

Merge branch 'main' into json-version

f50fea4

dmah42 approved these changes Jan 29, 2024

View reviewed changes

dmah42 merged commit 17bc235 into google:main Jan 29, 2024
77 of 80 checks passed

LebedevRI deleted the json-version branch January 29, 2024 13:23

LebedevRI mentioned this pull request May 24, 2024

CMake: unbreak version handling for tarballs #1793

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Output library / schema versions in JSON context block #1742

Output library / schema versions in JSON context block #1742

LebedevRI commented Jan 23, 2024

LebedevRI Jan 24, 2024

dmah42 Jan 24, 2024

LebedevRI Jan 24, 2024

dmah42 Jan 24, 2024

LebedevRI Jan 24, 2024

LebedevRI commented Jan 24, 2024

LebedevRI commented Jan 24, 2024

dmah42 commented Jan 24, 2024

LebedevRI commented Jan 24, 2024

LebedevRI commented Jan 24, 2024

dmah42 commented Jan 24, 2024

LebedevRI commented Jan 24, 2024

dmah42 commented Jan 24, 2024

LebedevRI Jan 24, 2024

dmah42 Jan 24, 2024

dmah42 commented Jan 29, 2024

LebedevRI commented Jan 29, 2024

LebedevRI commented Jan 29, 2024

dmah42 commented Jan 29, 2024 •

edited

Output library / schema versions in JSON context block #1742

Output library / schema versions in JSON context block #1742

Conversation

LebedevRI commented Jan 23, 2024

LebedevRI Jan 24, 2024

Choose a reason for hiding this comment

dmah42 Jan 24, 2024

Choose a reason for hiding this comment

LebedevRI Jan 24, 2024

Choose a reason for hiding this comment

dmah42 Jan 24, 2024

Choose a reason for hiding this comment

LebedevRI Jan 24, 2024

Choose a reason for hiding this comment

LebedevRI commented Jan 24, 2024

LebedevRI commented Jan 24, 2024

dmah42 commented Jan 24, 2024

LebedevRI commented Jan 24, 2024

LebedevRI commented Jan 24, 2024

dmah42 commented Jan 24, 2024

LebedevRI commented Jan 24, 2024

dmah42 commented Jan 24, 2024

LebedevRI Jan 24, 2024

Choose a reason for hiding this comment

dmah42 Jan 24, 2024

Choose a reason for hiding this comment

dmah42 commented Jan 29, 2024

LebedevRI commented Jan 29, 2024

LebedevRI commented Jan 29, 2024

dmah42 commented Jan 29, 2024 • edited

dmah42 commented Jan 29, 2024 •

edited