Skip to content

Conversation

@ZenithalHourlyRate
Copy link
Contributor

There are systems where /usr/bin/python is not avaiable (in my setup, ubuntu 22.04 even though both python2 and python3 are installed). The original workaround may introduce system bugs.

By this patch ovmf building process would resort to python3 instead of python, thus resolving the problem.

@Zildj1an
Copy link
Contributor

Zildj1an commented Dec 7, 2022

Congratulations on your first patch, @ZenithalHourlyRate! 🥇

Good idea, but I don't thinking adding a patch in the README.md is the best course of action. What happens when OVMF changes and the patch does not apply anymore?

If you want, we can instead add a note in the lines of "... if your error involves still not finding Python, you may try to...". This is, instead of providing a patch that fixes it, we tell developers something they can try. If you like the idea, go ahead and amend your commit. Use this chance to add to your commit message:

  1. A commit body, where you can briefly explain the change.
  2. Your Signed-off-by (See contribution guidelines and an example).
  3. My Reviewed-By (See example for 2.).

As a side note, our scripts and README.md should ease the job of developers, but do not intend to be exhaustive, or provide solutions to all problems.

@Zildj1an Zildj1an self-assigned this Dec 7, 2022
@Zildj1an Zildj1an added the documentation Improvements or additions to documentation label Dec 7, 2022
In systems (e.g. Ubuntu 22.04) where python is not aliased
as python2 or python3, scripts using python will
not find the executable.

One way to handle it is to install python-is-python3. However,
this may break the system and need great care from the user.

The original workaround is similar to the way above, but the
copied binary is not managed by the package manager.

Another way to fix it is to explicitly use python3 or python2
in the script, which has less impact on the system.

This commit introduces a way to fix the building script.

Signed-off-by: Hongren Zheng <i@zenithal.me>
Reviewed-by: Carlos Bilbao <carlos.bilbao@amd.com>
@ZenithalHourlyRate
Copy link
Contributor Author

Sorry for the late reply.

Use this chance to add to your commit message:

Made such changes now, could you please review again?

@Zildj1an
Copy link
Contributor

Looks good to go! :)

@Zildj1an Zildj1an merged commit 693b153 into AMDESE:main Dec 19, 2022
@ZenithalHourlyRate ZenithalHourlyRate deleted the fix-ovmf-python branch December 19, 2022 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants