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

Do not depend on unversioned python binary #1496

Merged
merged 2 commits into from
Oct 10, 2022
Merged

Conversation

MatzeB
Copy link
Contributor

@MatzeB MatzeB commented Oct 3, 2022

Some linux distributions no longer provide python binary and require usage of python3 instead. This changes the scripts here and uses cmake find_package(Python3 when running python.

@google-cla
Copy link

google-cla bot commented Oct 3, 2022

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.

@LebedevRI
Copy link
Collaborator

Is the CMake change nessesary given the rest of the diff?
Put differently, would dropping it still solve your issue?

Originally posted by @LebedevRI in #1495 (comment)

@MatzeB
Copy link
Contributor Author

MatzeB commented Oct 3, 2022

Is the CMake change nessesary given the rest of the diff? Put differently, would dropping it still solve your issue?

Well either the cmake changes or the shebang #! changes fixes things for me on CentOS8. I thought going through find_package(Python3 was just good cmake style, but happy to drop it from the change.

@LebedevRI
Copy link
Collaborator

I think it's not right to have the shebang AND do that CMake dance.
I would guess the shebang thing should be enough,
but then we indeed probably want to check that python interpreter is present at CMake time.
But then if we run those scripts via interpreter, does the shebang help with anything?

Anyways let's just merge as is i guess.

@LebedevRI
Copy link
Collaborator

(please resolve the CLA issue, though.)

@MatzeB
Copy link
Contributor Author

MatzeB commented Oct 3, 2022

Well, the shebang still helps people running the scripts manually.

Still looking through my companies policies about signing the CLA...

Some linux distributions no longer provide `python` binary and require
usage of `python3` instead. This changes the scripts here and uses
cmake `find_package(Python3` when running python.
CMakeLists.txt Show resolved Hide resolved
@LebedevRI
Copy link
Collaborator

CLA still seems stuck, should have it unstuck?

@dmah42
Copy link
Member

dmah42 commented Oct 6, 2022

i checked manually and @MatzeB hasn't signed the Google CLA as far as i can tell.

@MatzeB
Copy link
Contributor Author

MatzeB commented Oct 7, 2022

I am told CLA should work now. And when I go to https://cla.developers.google.com/clas I see "You are covered by the following corporate agreements:" "

Google Corporate CLA | Facebook, Inc. | Apr 29, 2016 12:42 PDT

Google Corporate CLA Facebook, Inc. Apr 29, 2016 12:42 PDT

Not sure why it's not working, does it still require the old matthiasb@fb.com mails instead of matthiasb@meta.com?

@LebedevRI
Copy link
Collaborator

Let's see if reopening PR helps.

@LebedevRI LebedevRI closed this Oct 9, 2022
@LebedevRI LebedevRI reopened this Oct 9, 2022
@dmah42
Copy link
Member

dmah42 commented Oct 10, 2022

i just forced a rescan on my end

@LebedevRI LebedevRI merged commit 229bc5a into google:main Oct 10, 2022
@joshmcorreia
Copy link

This merge forced my CI/CD pipelines that use googlebenchmark to fail because our images don't have Python installed, even though googlebenchmark has never required Python before.

@LebedevRI
Copy link
Collaborator

This merge forced my CI/CD pipelines that use googlebenchmark to fail because our images don't have Python installed, even though googlebenchmark has never required Python before.

Looks like you need to specify -DBENCHMARK_ENABLE_TESTING=OFF.

@joshmcorreia
Copy link

Looks like you need to specify -DBENCHMARK_ENABLE_TESTING=OFF.

We want it on, and have had no issues with it in the past...

@LebedevRI
Copy link
Collaborator

Then i guess perhaps this change should be changed to use find_program() instead of requiring python3-dev package to be present, that would profile cmake files.

@joshmcorreia
Copy link

That would be great, this is currently blocking all of my company's pipelines so we can't release anything. If you could revert this change while coming up with an update it would be greatly appreciated.

LebedevRI added a commit to LebedevRI/benchmark that referenced this pull request Oct 12, 2022
…1496)"

As predicted, the cmake part of the change is contentious.
google#1496 (comment)

This partially reverts commit 229bc5a.
LebedevRI added a commit that referenced this pull request Oct 12, 2022
@LebedevRI
Copy link
Collaborator

That would be great, this is currently blocking all of my company's pipelines so we can't release anything. If you could revert this change while coming up with an update it would be greatly appreciated.

I would love to, but apparently i can not.

dmah42 pushed a commit that referenced this pull request Oct 13, 2022
…#1501)

As predicted, the cmake part of the change is contentious.
#1496 (comment)

This partially reverts commit 229bc5a.
@joshmcorreia
Copy link

Thanks for the revert!

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.

5 participants