Add possibility to ask for libbenchmark version number #1004 #1418

Matthdonau · 2022-06-20T19:26:32Z

No description provided.

#1004) (#1403)" (#1417)" This reverts commit b7afda2.

Matthdonau · 2022-06-20T19:34:42Z

This should (to the best of my knowledge) fix the two bugs mentioned in #1417. I was able to reproduce both issues and fix them at least for my testing. However I cannot test with the setup of the two bug reporter. Therefore I cant be 100% sure if it fixes their issues as well. Sorry for introducing the bugs. I'll understand every scepticism merging this again.

dmah42 · 2022-06-21T08:29:10Z

no skepticism at all! we have a lot of platforms and a lot of build configurations to manage.

dmah42 · 2022-06-21T08:33:38Z

config/get_git_version.py

+        # In case volatile-status cannot be read, exit with an error
+        sys.exit("Cannot open volatile-status.txt")
+
+    if git_version == "" or is_dirty == "" or default_version == "":


i think this will still have the bug. if we didn't manage to read a git_version or is_dirty then we want to return the default version.

however, if default_version is "" then we have an API contract failure and should sys.exit.

so (earlier)

if default_version == "": sys.exit("default_version must be set")

and here

if git_version == "" or is_dirty == "": git_version = default_version else: git_version = normalize_version(git_version, is_dirty)

i think.

I cant think of a case where this happens. In workspace_status.py if git cant be called or we are not in a git checkout the git_version gets set to 0.0.0 or v.0.0.0 and git is_dirty gets set to true while the default version is set correctly.
This means either all of the fields are empty or none. I just or'ed them in case anyone changes workspace_status.py.

The only case where we do not have a git_version or is_dirty is when workspace_status.py isn't called . If this can happen there is no default_version we can set to either.

Going with your suggestion following may happen:
Someone changes workspace_status.py to store the git_version in a different variable. Then this script would not exit and just report the default version which would be a hard to catch bug I guess

@tkoeppe was the person who reported the issue to me. i think they were using a snapshot zip and then building it, so they didn't have a git checkout but were building with bazel.

did you change this behaviour in this new version?

Yes that problem should be fixed as long as workspace_status.py is called when building with bazel (to ensure that there is the .bazelrc). I don't know if it can happen e.g with snapshot zip that workspace_status.py isn't called when building.
If that is the case we have to move the default version from .bazelrc to the BUILD file.
Was the problem that the build failed or was the reported version "DEFAULT_VERSION"?

the build didn't start as the python script exited without outputing anything useful that bazel could use.

Do you have at any chance a log for this. Because if the sys.exit in the python file were triggered there should be output to stderr. namely:
Cannot open volatile-status.txt or No usable entry in volatile-status.txt
If I change the if clauses so the always catch for testing purposes I get:

INFO: Analyzed target //:benchmark (0 packages loaded, 0 targets configured). INFO: Found 1 target... ERROR: /Users/mad/Desktop/Verwaltung/Programs/gbench/benchmark/working2/benchmark/BUILD.bazel:21:24: Action include/benchmark/version.h failed: (Exit 1): get_git_version failed: error executing command bazel-out/host/bin/get_git_version --header bazel-out/darwin-fastbuild/bin/include/benchmark/version.h --header_input include/version.h.in --volatile_file bazel-out/volatile-status.txt ... (remaining 6 arguments skipped) Use --sandbox_debug to see verbose messages from the sandbox No usable entry in volatile-status.txt Target //:benchmark failed to build Use --verbose_failures to see the command lines of failed build steps. INFO: Elapsed time: 1.015s, Critical Path: 0.54s INFO: 2 processes: 2 internal. FAILED: Build did NOT complete successfully

if you encountered that I probably know how to fix that. Otherwise I don't know what happened there

Do you have at any chance a log for this. Because if the sys.exit in the python file were triggered there should be output to stderr. namely: Cannot open volatile-status.txt or No usable entry in volatile-status.txt If I change the if clauses so the always catch for testing purposes I get:

INFO: Analyzed target //:benchmark (0 packages loaded, 0 targets configured). INFO: Found 1 target... ERROR: /Users/mad/Desktop/Verwaltung/Programs/gbench/benchmark/working2/benchmark/BUILD.bazel:21:24: Action include/benchmark/version.h failed: (Exit 1): get_git_version failed: error executing command bazel-out/host/bin/get_git_version --header bazel-out/darwin-fastbuild/bin/include/benchmark/version.h --header_input include/version.h.in --volatile_file bazel-out/volatile-status.txt ... (remaining 6 arguments skipped) Use --sandbox_debug to see verbose messages from the sandbox No usable entry in volatile-status.txt Target //:benchmark failed to build Use --verbose_failures to see the command lines of failed build steps. INFO: Elapsed time: 1.015s, Critical Path: 0.54s INFO: 2 processes: 2 internal. FAILED: Build did NOT complete successfully

it was the "No usable entry" output. this is why i think the issue was that we weren't in a git checkout and the previous logic would just bail if it couldn't get a git version.

ah okay. perfect. That means there are ways of building the lib with bazel which ignore(?) the .bazelrc. That's fixable pretty easily. Give me a sec

dmah42 · 2022-06-21T10:18:35Z

config/get_git_version.py

+    except:
+        # In case volatile-status cannot be read, use the default version
+        git_version = args.default_version
+        is_dirty = "TRUE"


i think in this case it doesn't matter if is_dirty is true or false, but the logging in normalize_version might be surprising if it's unzipped from a snapshot or something.

dmah42 · 2022-06-21T10:19:10Z

config/get_git_version.py

+
+    # Read volatile-status.txt file
+    git_version = ""
+    is_dirty = ""


any reason why this isn't a boolean?

is_dirty = None and then just use True and False throughout?

config/get_git_version.py

dmah42 · 2022-06-21T11:58:13Z

config/get_git_version.py

@@ -64,22 +68,28 @@ def main():
                    if key == args.version_variable_name:
                        git_version = key_value[1].strip()
                    if key == args.is_dirty_name:
-                        is_dirty = key_value[1].strip()
+                        if key_value[1].strip() == "TRUE":


you can simplify this to:

is_dirty = (key_value[1].strip() == "TRUE")

tkoeppe · 2022-06-21T11:59:01Z

If you want to repro the problem, you can build this little demo benchmark: https://github.com/tkoeppe/tarpit/blob/bab67d3bbe7b077cebee06d05591a37ed977516b/sandbox/BUILD#L41-L46

Just adjust the repo's Bazel WORKSPACE to pin the benchmark revision to the broken one.

dmah42 · 2022-06-21T11:59:09Z

workspace_status.py

-        # Be pessimistic. In case git is not available
-        # assume the repository to be dirty.
-        return "TRUE"
+        return "FAlSE"


typo: FALSE not FAlSE

Matthdonau added 3 commits June 20, 2022 19:49

Revert "Revert "Add possibility to ask for libbenchmark version number (

51b93e9

#1004) (#1403)" (#1417)" This reverts commit b7afda2.

Change output dir of version header to project dir

b036cb9

Actually use default version in bazel if no git is available

13cfbc4

dmah42 reviewed Jun 21, 2022

View reviewed changes

Account for a bazle build which does not use the proj .bazelrc

24411b6

dmah42 reviewed Jun 21, 2022

View reviewed changes

config/get_git_version.py Show resolved Hide resolved

dmah42 reviewed Jun 21, 2022

View reviewed changes

Warn the user if git version could not be determined

0de7602

Matthdonau closed this by deleting the head repository Feb 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility to ask for libbenchmark version number #1004 #1418

Add possibility to ask for libbenchmark version number #1004 #1418

Matthdonau commented Jun 20, 2022

Matthdonau commented Jun 20, 2022

dmah42 commented Jun 21, 2022

dmah42 Jun 21, 2022

Matthdonau Jun 21, 2022 •

edited

dmah42 Jun 21, 2022

Matthdonau Jun 21, 2022

dmah42 Jun 21, 2022

Matthdonau Jun 21, 2022

Matthdonau Jun 21, 2022

dmah42 Jun 21, 2022

Matthdonau Jun 21, 2022

dmah42 Jun 21, 2022

dmah42 Jun 21, 2022

dmah42 Jun 21, 2022

tkoeppe commented Jun 21, 2022

dmah42 Jun 21, 2022

Add possibility to ask for libbenchmark version number #1004 #1418

Add possibility to ask for libbenchmark version number #1004 #1418

Conversation

Matthdonau commented Jun 20, 2022

Matthdonau commented Jun 20, 2022

dmah42 commented Jun 21, 2022

Choose a reason for hiding this comment

Matthdonau Jun 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkoeppe commented Jun 21, 2022

Choose a reason for hiding this comment

Matthdonau Jun 21, 2022 •

edited