Skip to content
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

Revert "Add possibility to ask for libbenchmark version number (#1004)" #1417

Merged
merged 1 commit into from Jun 20, 2022

Conversation

dmah42
Copy link
Member

@dmah42 dmah42 commented Jun 20, 2022

Reverts #1403

due to two reported bugs:

version status has nothing useful (not a git checkout) and
#1416

@dmah42 dmah42 merged commit b7afda2 into main Jun 20, 2022
@dmah42 dmah42 deleted the revert-1403-add_lib_version_define branch June 20, 2022 16:52
@dmah42
Copy link
Member Author

dmah42 commented Jun 20, 2022

@Matthdonau sorry for reverting your work. feel free to revert the revert and make the improvements necessary to make this work:

  1. instead of exiting when we don't get a git version, we should return the default (l74-ish of the python script).
  2. figure out why cmake embedding fails... maybe we don't export the version.h correctly?

@Matthdonau
Copy link
Contributor

Regarding 1: I think I don't understand. What you describe is what should happen. if no git is available or we are not in a git checkout the default project version (currently 1.6.1) is used.
Regarding 2: @dominichamon I suspect the problem is that the version.h header is generated in CMAKE_BINARY_DIR while PROJECT_BINARY_DIR would be correct

@dmah42
Copy link
Member Author

dmah42 commented Jun 20, 2022

Regarding 1: I think I don't understand. What you describe is what should happen. if no git is available or we are not in a git checkout the default project version (currently 1.6.1) is used.

l71 .. we exit. we don't return the default. this is being tickled when using snapshot zip releases I think.

Regarding 2: @dominichamon I suspect the problem is that the version.h header is generated in CMAKE_BINARY_DIR while PROJECT_BINARY_DIR would be correct

ah maybe. we should do whatever the other generated header is doing as that seems to work.

@Matthdonau
Copy link
Contributor

Matthdonau commented Jun 20, 2022

I made a PR. The problem is: If there is no volatile-status.txt there is no default version to default to. Is this really the issue here or is it the issue I fixed in the PR? If not I'll guess we could move the definition of the default version to the BUILD.bazel file. If the result of the problem was that libbbenchmark reported DEFAULT_VERSION as version it is the bug fixed in the PR I guess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants