search

myspaghetti / macos-virtualbox

Push-button installer of macOS Catalina, Mojave, and High Sierra guests in Virtualbox on x86 CPUs for Windows, Linux, and macOS
GNU General Public License v2.0
13.53k stars 1.12k forks source link

CPU_PROFILE Configuration In Script, Better Support For VBox Installs On A Different Drive Than C:, And CPU Recognition For The Beginners #405

Closed TheBotlyNoob closed 3 years ago

TheBotlyNoob commented 3 years ago

Added the automatic CPU_PROFILE config variable so it is a lot simpler to change the cpu profile. And its easier to make more VMs with the same configuration.

TheBotlyNoob commented 3 years ago

I have completed all of your edits.

myspaghetti commented 3 years ago

Thanks.

Sorry, but it just occurred to me: if no CPU profile is required then this change does no harm, but if it is required then this change does no good since the user will have to manually choose the CPU profile anyway. This just adds an extra step. Instead of running VBoxManage modifyvm ... and then ./macos-guest-virualbox.sh populate..., the user has to edit the script then run ./macos-guest-virualbox.sh configure_vm populate...

On the other hand, if the user keeps the script for future virtual machines, then the configure_vm step will correctly configure their CPU profile.

Personally, I think running a terminal command six times is easier than editing a shell script six times, one less state change each time.

I'm not closing the pull request yet, but I have to think about it.

TheBotlyNoob commented 3 years ago

Okay, thank you. most CPU's are incompatible so I just leave it on Intel Core i7-3960X for convenience.

TheBotlyNoob commented 3 years ago

I mean maybe you could just link my respiratory in the CPUID section. This is just for convenience I really don't care if it get merged or not.

Yethal commented 3 years ago

@myspaghetti If the user knows for a fact they're going to change the cpu profile (so far all Ryzen systems required it for example) then they can edit the profile before the first run and have a working VM on the first try

myspaghetti commented 3 years ago

That's true. I believe only advanced users know beforehand which CPU profile or CPUID they require, so I'm not sure how much of a convenience it is to add a cpu_profile variable when all it does is add --cpu-profile "${cpu_profile} to line 645. It's definitely slightly more convenient to edit the variable at the top of the script than hunt around for line 645.

I'm considering it.

myspaghetti commented 3 years ago

This is good. I'll review/merge it soon.

TheBotlyNoob commented 3 years ago

@myspaghetti Just a quick question, why is the max VRAM 128? Because the command VBoxManage.exe modifyvm "macOS" --vram 256 works like a charm?

TheBotlyNoob commented 3 years ago

Ok, after a short decision I have decided that macOS really isn't my cup of tea. So I will keep my fork maintained and maybe even make more features, but im never going to use it (except for bug fixes).

myspaghetti commented 3 years ago

I see there have been further changes; they're all pretty positive but some cleanup is required, for example instead of specifying "C" "D" "E" ..." there should be a more expedient way to cycle through the alphabet.

myspaghetti commented 3 years ago

Just a quick question, why is the max VRAM 128? Because the command VBoxManage.exe modifyvm "macOS" --vram 256 works like a charm?

There might have been some old versions of VirtualBox that didn't support this. I'll have to check.

TheBotlyNoob commented 3 years ago

It seems to have been supported since 2015 Here is where I found the command

myspaghetti commented 3 years ago

It's true that VirtualBox has supported 256MB of VRAM since about 2010, but not across 5.2 and 6.0 when I first wrote the script. I guess I'll test it again now across the latest versions of 5.2, 6.0, and 6.1.

myspaghetti commented 3 years ago

I'm closing this pull request. Some of the contributions are good, others are excluded by design.

The VRAM will most likely be implemented (when I or someone else test it) and can be submitted in a separate pull request. The drive letters are fine and can be submitted in a separate pull request but the issue affects very few people, and the people it does affect are likely to have installed VirtualBox outside of the default "Program Files" path anyway. The CPU database (even with a data set of 1 entry) will not be implemented.