-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
Allow to use github.com/Code-Hex/vz/v2 from macOS 11/macOS 10 #48
Conversation
I forgot to mention that after these changes there is still these warnings when building:
I'd need help to silence them as I don't really manage to read the related objC code :( |
@cfergeau Thanks! |
Please discard this commit |
Apologies for this, I pushed some commits by mistake, the PR should have stopped at |
This was the plan after checking with this PR that the approach I took is acceptable, I'll work on it now!
What commit logs do I need to rebase? I removed some of the commits I added by mistake, do you want more changes on logs of the existing commits? |
@cfergeau It's ok. Please continue. Please write some test code. Thank you! |
I've added some test to this series. It's still macos11 only, macos10 is progressing well in https://github.com/cfergeau/vz/tree/macos10, it's missing:
Tests are slightly more complicated, as the test must be built on macOS11 (no Virtualization.h header on macOS10 machines), and then run on macOS10. I haven't looked yet into doing that in github actions. I can't manually test as I no longer have a macOS10 machine. |
1bbc1fc
to
18b71ff
Compare
I've added macos 10 "support". I've also added some tests. The macos10 tests are not run automatically in gh actions |
@cfergeau Could you please remove the part about the test code once you have written it, because to be honest the test code you write is unfortunately not of mergeable quality? |
You asked about adding tests ( #48 (comment) ) so I wrote them. They test if we get ErrUnsupportedOS when expected. |
@cfergeau A test to determine availability can be done by mocking the version information. Revert the test code, actions settings, and makefile modifications |
virtualization.go
Outdated
if len(osverArray) < 1 { | ||
return 0 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this handling because the result will be more than or equal 1
https://go.dev/play/p/BJBkxhG0-lQ
@@ -294,3 +348,23 @@ func (v *VirtualMachine) Stop(fn func(error)) { | |||
func (v *VirtualMachine) StartGraphicApplication(width, height float64) { | |||
C.startVirtualMachineWindow(v.Ptr(), C.double(width), C.double(height)) | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix like this
var (
majorVersion int
majorVersionOnce sync.Once
)
// This can be replaced in the test code to enable mock.
// It will not be changed in production.
var fetchMajorVersion = func() {
osver, err := syscall.Sysctl("kern.osproductversion")
if err != nil {
panic(err)
}
osverArray := strings.Split(osver, ".")
major, err := strconv.Atoi(osverArray[0])
if err != nil {
panic(err)
}
majorVersion = major
}
func macOSMajorVersion() int {
majorVersionOnce.Do(fetchMajorVersion)
return majorVersion
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, I will do this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest push should be closer to what you want. I kept the build tags. If you prefer, I can set majorVersion
at the beginning of each test.
Why can't you do what I commented on?
|
I've removed all of this in my latest push. I thought the addition of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably last requests 🙏
shared_folder.go
Outdated
import ( | ||
"runtime" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import ( | |
"runtime" | |
) | |
import "runtime" |
"unsafe" | ||
) | ||
|
||
var ErrUnsupportedOSVersion error = errors.New("unsupported macOS version") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add document (comment)
virtualization.h
Outdated
@@ -21,7 +21,7 @@ bool shouldAcceptNewConnectionHandler(void *listener, void *connection, void *so | |||
|
|||
/* VZVirtioSocketListener */ | |||
@interface VZVirtioSocketListenerDelegateImpl : NSObject <VZVirtioSocketListenerDelegate> | |||
- (BOOL)listener:(VZVirtioSocketListener *)listener shouldAcceptNewConnection:(VZVirtioSocketConnection *)connection fromSocketDevice:(VZVirtioSocketDevice *)socketDevice; | |||
- (BOOL)listener:(void *)listener shouldAcceptNewConnection:(void *)connection fromSocketDevice:(void *)socketDevice; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not do anything that is not related to the intent of this PR. Please revert.
- (BOOL)listener:(void *)listener shouldAcceptNewConnection:(void *)connection fromSocketDevice:(void *)socketDevice; | |
- (BOOL)listener:(VZVirtioSocketListener *)listener shouldAcceptNewConnection:(VZVirtioSocketConnection *)connection fromSocketDevice:(VZVirtioSocketDevice *)socketDevice; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indirectly related as this silences some warnings when building on older macOS versions. I've split the changes to a separate commit with an explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean. This library is supported only above macOS v11.0. So I think this is no problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#41 asks for macOS v10 'support'. There will be no virtualization on these platforms, but errors/exceptions will be returned. Returning errors/exceptions need to be done in Code-Hex/vz
. To add these, I use a lot compiler warnings:
# github.com/Code-Hex/vz/v2
In file included from _cgo_export.c:4:
In file included from socket.go:6:
../../virtualization.h:24:19: warning: 'VZVirtioSocketListener' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZVirtioSocketListener.h:25:12: note: 'VZVirtioSocketListener' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.13.0
../../virtualization.h:24:1: note: annotate 'listener:shouldAcceptNewConnection:fromSocketDevice:' with an availability attribute to silence this warning
../../virtualization.h:24:80: warning: 'VZVirtioSocketConnection' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZVirtioSocketConnection.h:18:12: note: 'VZVirtioSocketConnection' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.13.0
../../virtualization.h:24:1: note: annotate 'listener:shouldAcceptNewConnection:fromSocketDevice:' with an availability attribute to silence this warning
../../virtualization.h:24:136: warning: 'VZVirtioSocketDevice' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZVirtioSocketDevice.h:25:12: note: 'VZVirtioSocketDevice' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.13.0
../../virtualization.h:24:1: note: annotate 'listener:shouldAcceptNewConnection:fromSocketDevice:' with an availability attribute to silence this warning
If there are warnings, there is some code without @available
. If there are pages and pages of warnings, I will not notice new code missing @available
. This is why I prefer to remove all the warnings I can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My opinion. #41 (comment)
I don't support the compilation on macOS v10. because It depends on xcode sdk.
So you don't need to consider for v10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These warnings happen with latest macOS/xcode when you build a binary for macOS 10 or newer:
% sw_vers
ProductName: macOS
ProductVersion: 12.6
BuildVersion: 21G115
% CGO_CFLAGS=-mmacosx-version-min=10.13 make -C example/linux
go build -o virtualization .
# github.com/Code-Hex/vz/v2
In file included from virtualization.m:8:
../../virtualization_view.h:26:41: warning: 'VZVirtualMachine' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZVirtualMachine.h:67:12: note: 'VZVirtualMachine' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.13.0
../../virtualization_view.h:26:1: note: annotate 'initWithVirtualMachine:windowWidth:windowHeight:' with an availability attribute to silence this warning
virtualization.m:1249:37: warning: 'VZVirtualMachine' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZVirtualMachine.h:67:12: note: 'VZVirtualMachine' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.13.0
virtualization.m:1249:37: note: enclose 'VZVirtualMachine' in an @available check to silence this warning
# github.com/Code-Hex/vz/v2
They are not related to building on macOS 10. I agree we don't need to support building on macOS 10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cfergeau Ping
virtualization.m
Outdated
@@ -36,9 +36,13 @@ - (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(N | |||
@end | |||
|
|||
@implementation VZVirtioSocketListenerDelegateImpl | |||
- (BOOL)listener:(VZVirtioSocketListener *)listener shouldAcceptNewConnection:(VZVirtioSocketConnection *)connection fromSocketDevice:(VZVirtioSocketDevice *)socketDevice; | |||
- (BOOL)listener:(void *)listener shouldAcceptNewConnection:(void *)connection fromSocketDevice:(void *)socketDevice; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not do anything that is not related to the intent of this PR. Please revert.
- (BOOL)listener:(void *)listener shouldAcceptNewConnection:(void *)connection fromSocketDevice:(void *)socketDevice; | |
- (BOOL)listener:(VZVirtioSocketListener *)listener shouldAcceptNewConnection:(VZVirtioSocketConnection *)connection fromSocketDevice:(VZVirtioSocketDevice *)socketDevice; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reasons as #48 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this~
@@ -65,7 +65,7 @@ void *newVZMacMachineIdentifierWithPath(const char *machineIdentifierPath); | |||
void *newVZMacMachineIdentifierWithBytes(void *machineIdentifierBytes, int len); | |||
nbyteslice getVZMacMachineIdentifierDataRepresentation(void *machineIdentifierPtr); | |||
|
|||
VZMacOSRestoreImageStruct convertVZMacOSRestoreImage2Struct(VZMacOSRestoreImage *restoreImage); | |||
VZMacOSRestoreImageStruct convertVZMacOSRestoreImage2Struct(void *restoreImagePtr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not do anything that is not related to the intent of this PR. Please revert.
VZMacOSRestoreImageStruct convertVZMacOSRestoreImage2Struct(void *restoreImagePtr); | |
VZMacOSRestoreImageStruct convertVZMacOSRestoreImage2Struct(VZMacOSRestoreImage *restoreImagePtr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also related to #48 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is used only internally. So I don't think this is no problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think we need this ( #48 (comment) ). It is really useful for the maintainance of macOS 11/macOS 10 compat code. If there are too many warnings, we will not notice the new ones.
It also makes the code consistent with the other convert*
functions:
virtualization.h:VZVirtioSocketConnectionFlat convertVZVirtioSocketConnection2Flat(void *connection);
virtualization_arm64.h:VZMacOSConfigurationRequirementsStruct convertVZMacOSConfigurationRequirements2Struct(void *requirementsPtr);
virtualization_arm64.h:VZMacHardwareModelStruct convertVZMacHardwareModel2Struct(void *hardwareModelPtr);
They all use void*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cfergeau Ping
(Because I don't need to support os 10)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This specific change is about macOS11. Commit log mentions CGO_CFLAGS=-mmacosx-version-min=11.0
and warning: 'VZMacOSRestoreImage' is only available on macOS 12.0 or newer
.
I've reverted the vsock one (to avoid force push). I'd like to squash all the latest changes/remove the problematic commit and then force-push though as it's getting messy ^^.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is used only internally not in the Go world. So I want to revert this one (because this is not following other convert function rules).
I think we can remove it from the header file.
This is why I don't want to do code reviews.
I'd like to squash all the latest changes/remove the problematic commit and then force-push though as it's getting messy ^^.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is used only internally not in the Go world.
I know it is only internal. But the objc compiler warns about it.
(because this is not following other convert function rules).
All other convert functions are using void *
:
Line 124 in 37646e1
VZVirtioSocketConnectionFlat convertVZVirtioSocketConnection2Flat(void *connection); Lines 72 to 73 in 37646e1
VZMacOSConfigurationRequirementsStruct convertVZMacOSConfigurationRequirements2Struct(void *requirementsPtr); VZMacHardwareModelStruct convertVZMacHardwareModel2Struct(void *hardwareModelPtr);
convertVZMacOSRestoreImage2Struct
is the only one using VZMacOSRestoreImage *
and not void *
. This change makes all convert
functions the same. And it removes a compilation warning. This will be useful for future maintainance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning with CGO_CFLAGS=-mmacosx-version-min=11.0
is still present even if I remove convertVZMacOSRestoreImage2Struct
from the header
# github.com/Code-Hex/vz/v2
virtualization_arm64.m:330:68: warning: 'VZMacOSRestoreImage' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZMacOSRestoreImage.h:30:12: note: 'VZMacOSRestoreImage' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 11.0.0
virtualization_arm64.m:330:34: note: annotate 'convertVZMacOSRestoreImage2Struct' with an availability attribute to silence this warning
In the latest iteration, I've added exceptions. I wrote the code as described in #48 (comment)
It is important to write it this way to avoid compiler warnings. The compiler warnings will be useful when new code is added. They will tell us about APIs which require to be wrapped in @available. |
This adds @available(macOS 11, *) annotations to the classes related to virtual machine configguration to indicate virtualization framework is only available in macOS 11+ The corresponding go NewXXX methods return ErrUnsupportedOSVersion when running on macOS 10 systems.
This adds @available(macOS 11, *) annotations to the classes related to virtio-rng to indicate virtualization framework is only available in macOS 11+ The corresponding go NewXXX methods return ErrUnsupportedOSVersion when running on macOS 10 systems.
This adds @available(macOS 11, *) annotations to the classes related to virtio-balloon to indicate virtualization framework is only available in macOS 11+ The corresponding go NewXXX methods return ErrUnsupportedOSVersion when running on macOS 10 systems.
This adds @available(macOS 11, *) annotations to the classes related to virtio-net to indicate virtualization framework is only available in macOS 11+ The corresponding go NewXXX methods return ErrUnsupportedOSVersion when running on macOS 10 systems.
This adds @available(macOS 11, *) annotations to the classes related to virtio-serial to indicate virtualization framework is only available in macOS 11+ The corresponding go NewXXX methods return ErrUnsupportedOSVersion when running on macOS 10 systems.
This adds @available(macOS 11, *) annotations to the classes related to virtio-blk to indicate virtualization framework is only available in macOS 11+ The corresponding go NewXXX methods return ErrUnsupportedOSVersion when running on macOS 10 systems.
This adds @available(macOS 11, *) annotations to the classes related to VM handling to indicate virtualization framework is only available in macOS 11+ The corresponding go NewXXX methods return ErrUnsupportedOSVersion when running on macOS 10 systems.
This adds @available(macOS 11, *) annotations to the classes related to virtio-vsock to indicate virtualization framework is only available in macOS 11+ The corresponding go NewXXX methods return ErrUnsupportedOSVersion when running on macOS 10 systems.
shouldAcceptNewConnection currently has VZVirtioSocketListener *, VZVirtioSocketConnection * and VZVirtioSocketDevice * parameters, which cause the warning below when trying to build for an older version of macOS with CGO_CFLAGS=-mmacosx-version-min=10.13. This commit changes these types to `void *` to silence this warning: # github.com/Code-Hex/vz/v2 In file included from _cgo_export.c:4: In file included from socket.go:6: ../../virtualization.h:24:19: warning: 'VZVirtioSocketListener' is only available on macOS 11.0 or newer [-Wunguarded-availability-new] /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZVirtioSocketListener.h:25:12: note: 'VZVirtioSocketListener' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.13.0 ../../virtualization.h:24:1: note: annotate 'listener:shouldAcceptNewConnection:fromSocketDevice:' with an availability attribute to silence this warning ../../virtualization.h:24:80: warning: 'VZVirtioSocketConnection' is only available on macOS 11.0 or newer [-Wunguarded-availability-new] /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZVirtioSocketConnection.h:18:12: note: 'VZVirtioSocketConnection' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.13.0 ../../virtualization.h:24:1: note: annotate 'listener:shouldAcceptNewConnection:fromSocketDevice:' with an availability attribute to silence this warning ../../virtualization.h:24:136: warning: 'VZVirtioSocketDevice' is only available on macOS 11.0 or newer [-Wunguarded-availability-new] /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZVirtioSocketDevice.h:25:12: note: 'VZVirtioSocketDevice' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.13.0 ../../virtualization.h:24:1: note: annotate 'listener:shouldAcceptNewConnection:fromSocketDevice:' with an availability attribute to silence this warning
bbedaa5
to
f3fdf30
Compare
@cfergeau If you force push, I can't check what's been fixed, so please don't do from next. |
example/linux/main.go
Outdated
vmlinuz, | ||
vz.WithCommandLine(strings.Join(kernelCommandLineArguments, " ")), | ||
vz.WithInitrd(initrd), | ||
) | ||
if err != nil { | ||
panic(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you replace panic with log.Fatal(err)?
I used panic because it does not complete the setup for logger in line 41.
Ah yes, github is annoying, it does not keep old versions of PRs :( |
@cfergeau Thank you! |
This reverts commit f3fdf30.
This PR adds version annotation to the objective C API so that it's possible to
build vz/use it on a macOS 10/11 machine even if it does not implement all the
latest virtualization.framework API. When trying to use unsupported API,
vz.ErrUnsupportedOSVersion will be returned.
This addresses #41 as this
allows to support macOS 10, macOS 11 and macOS 12 in the same build.