Skip to content

Build //:benchmark as a static library only. #1373

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

Merged
merged 1 commit into from
Mar 17, 2022
Merged

Build //:benchmark as a static library only. #1373

merged 1 commit into from
Mar 17, 2022

Conversation

junyer
Copy link
Contributor

@junyer junyer commented Mar 16, 2022

If someone or something ever needs the dynamic library as a Bazel build
artifact, we can figure that out for them then, but right now, there is
no strong reason to be wrangling various export.h-controlling macros.

Fixes #1372.

If someone or something ever needs the dynamic library as a Bazel build
artifact, we can figure that out for them then, but right now, there is
no strong reason to be wrangling various `export.h`-controlling macros.

Fixes #1372.
@dmah42
Copy link
Member

dmah42 commented Mar 17, 2022

i thought that the python bindings required a dynamic library (so we should keep the bindings rule the same)... am i wrong?

@junyer
Copy link
Contributor Author

junyer commented Mar 17, 2022

This change doesn't affect how py_extension() builds the .so/.dll for the Python extension. Note that it was already linking the static library (linkstatic = True) and the previous issue was that it wasn't defining BENCHMARK_STATIC_DEFINE. With this change, it will continue to link the static library, but the macro will now be defined by //:benchmark (i.e. centrally) and propagated to dependent rules such as this.

@dmah42 dmah42 merged commit 6a894bd into google:main Mar 17, 2022
@dmah42
Copy link
Member

dmah42 commented Mar 17, 2022

thanks!

@dmah42
Copy link
Member

dmah42 commented Mar 17, 2022

@junyer
Copy link
Contributor Author

junyer commented Mar 17, 2022

Yikes! Digging... (I had run bazel test //:all //test:all //tools:all locally before posting this PR and everything passed, so I'm confused now.)

@dmah42
Copy link
Member

dmah42 commented Mar 17, 2022

yeah, and all the same tests ran on CI on the pull request too.

@junyer
Copy link
Contributor Author

junyer commented Mar 17, 2022

Ahh. We need to omit these as well:

posix_copts = [
    "-fvisibility=hidden",
    "-fvisibility-inlines-hidden",
]

I'm still baffled as to why this didn't break last night.

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.

[BUG] LNK4217 and LNK4286 warnings when linking //:benchmark_main
2 participants