Skip to content

enables make vm-create in gentoo linux #1

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

romulocollopy
Copy link

No description provided.

Copy link
Member

@turicas turicas left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I think spliting everything in little bash functions enhances the readability. I added some comments regarding required changes to make everthing in the same standard I'm using and removing unneeded commands.

Comment on lines +154 to +156
if is_gentoo; then
mkdir -p /var/lib/libvirt/images
fi
Copy link
Member

Choose a reason for hiding this comment

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

This line is not required by Gentoo only and also could be improved to reuse the variable where the image path is stored. I prefer changing it to:

Suggested change
if is_gentoo; then
mkdir -p /var/lib/libvirt/images
fi
mkdir -p $(dirname "$DEBIAN_QCOW2")

fi
}

function qemu_connect() {
Copy link
Member

Choose a reason for hiding this comment

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

This function name does not describe what it does (all virsh commands connect to qemu in a way or another). I suggest config_virsh_network.

Suggested change
function qemu_connect() {
function config_virsh_network() {

wget -c -t 0 -O "$DEBIAN_QCOW2" "$DEBIAN_QCOW2_URL"
install_libs
config_permissions
qemu_connect
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
qemu_connect
config_virsh_network

Comment on lines +79 to +84

if is_gentoo; then
mkdir /var/lib/libvirt/shared/debian12-pydokku
mkdir /var/lib/libvirt/boot
fi

Copy link
Member

Choose a reason for hiding this comment

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

I didn't understand why those lines are needed, since there's mkdir -p "$SHARED_FOLDER" inside create_shared_folder already (which does the same as the first line inside the if) and /var/lib/libvirt/boot is not used at all.
I suggest you trying to completely remove this if, completely remove the VM (virt-manager could help) and running the make vm-create without them to check if this is really required.

Suggested change
if is_gentoo; then
mkdir /var/lib/libvirt/shared/debian12-pydokku
mkdir /var/lib/libvirt/boot
fi

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