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

run_qemu: make git-qemu argument a path #17

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

Conversation

nmtadam
Copy link

@nmtadam nmtadam commented Jun 6, 2022

Remove hardcoded git-qemu path, if the argument is not provided and needed
for cxl options then set ~/git/qemu/ as the default

Signed-off-by: Adam Manzanares [email protected]

Remove hardcoded git-qemu path, if the argument is not provided and needed
for cxl options then set ~/git/qemu/ as the default

Signed-off-by: Adam Manzanares <[email protected]>
@@ -227,16 +227,18 @@ process_options_logic()
_arg_cxl="on"
fi
if [[ $_arg_cxl_legacy == "on" ]] || [[ $_arg_cxl == "on" ]]; then
_arg_git_qemu="on"
if [[ ! $_arg_git_qemu ]]; then
_arg_git_qemu=~/git/qemu/
Copy link
Member

Choose a reason for hiding this comment

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

This puts us in a weird situation where ~/git/qemu is default for CXL, but there's no default outside of CXL.
Also with CXL support soon to start appearing in upstream releases, and distros thereafter, I wonder if it is time to drop the CXL special case requirement of git-qemu altogether.

if [[ $_arg_git_qemu ]]; then
qemu=${_arg_git_qemu}/x86_64-softmmu/qemu-system-x86_64
qemu_img=${_arg_git_qemu}/qemu-img
qmp=${_arg_git_qemu}/scripts/qmp/qmp-shell
Copy link
Member

Choose a reason for hiding this comment

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

So this essentially duplicates the env way of setting qemu's location -

qemu=/path/to/qemu/binary run_qemu.sh ...

--git-qemu was supposed to be a shortcut to make that default to ~/git/qemu which happened to be the path in my case :)

Now that the user base is slightly bigger, I think it makes sense to clean it all up a bit:

  1. We can convert to --git=qemu= going forward, and make ~git/qemu the default in parser_generator.m4
  2. make qemu=<path> run_qemu.sh ... behave the same way (probably already the case)
  3. Remove the special case behavior for --cxl

Thoughts on this?

Copy link
Author

Choose a reason for hiding this comment

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

Can we get rid of all of these special cases and just use the env way of setting qemu? Let's assume cxl support moving forward, and figure out a way to add a check for cxl support if --cxl is set, maybe check qemu version?

@stellarhopper
Copy link
Member

I forgot to add above - if --git-qemu=~/git/qemu becomes the new default, we'd need a new option to use system installed qemu. Maybe we just add a new boolean option for that, --system-qemu

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.

2 participants