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

Use python3 interpreter in your path instead of hardcoding /bin/python3 #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dannf
Copy link

@dannf dannf commented Sep 9, 2020

On Debian-based systems, python3 is in /usr/bin.

Signed-off-by: dann frazier dann.frazier@canonical.com

On Debian-based systems, python3 is in /usr/bin.

Signed-off-by: dann frazier <dann.frazier@canonical.com>
@lersek
Copy link
Member

lersek commented Sep 10, 2020

@encukou can you please advise if the proposed shebang (#!/usr/bin/env python3) is aligned with the python invocation rules in Fedora and RHEL? Thank you.

@encukou
Copy link

encukou commented Sep 10, 2020

The proposed #!/usr/bin/env python3 is the best choice here.


In Fedora/RHEL, you'll get the most robust behavior with %{__python3}, a RPM macro. That is the system Python, regardless of the user's $PATH or virtual environment.
In Fedora, the %py3_shebang_fix macro can switch the shebang(s) for you. The macro is not in RHEL yet, so write it out as /usr/bin/pathfix.py -pni %{__python3} FILE...

@lersek
Copy link
Member

lersek commented Sep 10, 2020

@encukou thank you for the help!

I've now reviewed the edk2 package's SPEC file in RHEL8, where this python script is primarily meant to be invoked (down-stream), in the Brew build root. I now remember that we've already been careful enough to invoke the script from the edk2 spec file regardless of the shebang -- because this is what we have in RHEL8:

%{__python3} ovmf-vars-generator --verbose --verbose \
...

In Fedora, we have:

python3 qemu-ovmf-secureboot-%{qosb_version}/ovmf-vars-generator \
...

So the edk2 spec file in RHEL8 already uses the best possible approach. And the Fedora one, while it could be improved, is nonetheless independent of the script's own shebang.

Thus, given

  • your confirmation that the proposed change is the best for upstream, and

  • our already ensured downstream independence of the upstream shebang,

I'm going to approve this PR (as soon as I can figure out how to do that on this "modern" UI called "github").

Thanks!

Copy link
Member

@lersek lersek left a comment

Choose a reason for hiding this comment

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

Reviewed-by: Laszlo Ersek lersek@redhat.com

per above discussion.

@lersek
Copy link
Member

lersek commented Sep 10, 2020

@dannf I have zero clue why this PR is stuck in DCO Expected — Waiting for status to be reported state. I don't know what's missing, sorry.

@lersek
Copy link
Member

lersek commented Sep 10, 2020

@dannf ... because your commit message does carry your S-o-b.

@dannf
Copy link
Author

dannf commented Sep 10, 2020

@dannf ... because your commit message does carry your S-o-b.

Weird - it does have my S-o-b - and it is using an email address attached to my github account.

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.

3 participants