Skip to content

2011: Installing contents from benchmark's tools/ subdirectory.#2016

Merged
LebedevRI merged 4 commits into
google:mainfrom
BuildMonkey:main
Aug 13, 2025
Merged

2011: Installing contents from benchmark's tools/ subdirectory.#2016
LebedevRI merged 4 commits into
google:mainfrom
BuildMonkey:main

Conversation

@BuildMonkey

@BuildMonkey BuildMonkey commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

This Pull Request (PR) relates to the following issue:
#2011 (comment)

I modeled my change against how benchmark docs are installed, except tools do not have a comparable CMAKE_INSTALL_DOCDIR variable recognised by build systems. Instead, I used CMAKE_INSTALL_PREFIX.

Additional, like docs, I have tools subdirectory installed by default, but we can change if this isn't the optimal behaviour.

Fixes #2011.

@google-cla

google-cla Bot commented Aug 6, 2025

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@dmah42

dmah42 commented Aug 6, 2025

Copy link
Copy Markdown
Member

let me know when you've signed the CLA and i will take a look. thank you!

Comment thread src/CMakeLists.txt Outdated
@BuildMonkey

Copy link
Copy Markdown
Contributor Author

Hello @dmah42, I have completed the CLA.

Comment thread src/CMakeLists.txt
dmah42
dmah42 previously approved these changes Aug 12, 2025
LebedevRI
LebedevRI previously approved these changes Aug 12, 2025
@LebedevRI

Copy link
Copy Markdown
Collaborator

@BuildMonkey you need to deal with the CLA check.

@BuildMonkey BuildMonkey dismissed stale reviews from LebedevRI and dmah42 via ef8b37b August 13, 2025 00:24
@BuildMonkey BuildMonkey force-pushed the main branch 2 times, most recently from 3d31c0c to cecc312 Compare August 13, 2025 00:30
@BuildMonkey

Copy link
Copy Markdown
Contributor Author

The CLA should be good now.

LebedevRI
LebedevRI previously approved these changes Aug 13, 2025
Comment thread src/CMakeLists.txt Outdated
Comment on lines +198 to +200
if (NOT DEFINED CMAKE_INSTALL_PYTOOLSDIR)
set(CMAKE_INSTALL_PYTOOLSDIR "${CMAKE_INSTALL_DATADIR}/googlebenchmark/tools" CACHE PATH "")
endif()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it more, i don't believe those guards should be there,
let's just always set()...

@LebedevRI

Copy link
Copy Markdown
Collaborator

Err, could you rebase this? All those commits should not be there.
This is generally why it's best to create a branch for the changes-to-be-PR'd.

@LebedevRI LebedevRI merged commit 7697796 into google:main Aug 13, 2025
84 checks passed
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.

Packaging tools subdirectory on install

4 participants