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

Basic implementation of shared folders #28

Merged
merged 20 commits into from
Aug 10, 2022

Conversation

paulcacheux
Copy link
Contributor

Hello, I'd like to work on implementing shared folders. This PR is currently a basic PoC but I would be happy to continue working on it if it is of any interest to you.

@Code-Hex
Copy link
Owner

Code-Hex commented Mar 6, 2022

@paulcacheux Thanks. Could you please add more comments?

@Code-Hex Code-Hex added the good first issue Good for newcomers label Mar 6, 2022
@paulcacheux
Copy link
Contributor Author

I added support for VZMultipleDirectoryShare and more documentation and comments. What would be the best way to add tests in your opinion ?

@ghost
Copy link

ghost commented Mar 15, 2022

Probably could add it to the example for now and use the environment file to illustrate it. I think that normal *_test.go files might fall short of being able to test something like this appropriately. Something like how gvproxy is tested might be reasonable later on?

Edit, just finished testing this out by doing like the following.

Added to example/.env.example

SHAREDDIR_NAME="my_shared_dir"
SHAREDDIR_PATH="/Users/codehex/Desktop/shared_dir"

Added to example/main.go

	sharedDirPath := os.Getenv("SHAREDDIR_PATH")
	sharedDirName := os.Getenv("SHAREDDIR_NAME")

	...

	// permits guests vsock and virtio-fs support to mount directory from host for example:
	// mount -t virtiofs SHARED_DIR_NAME SHAREDDIR_PATH
	virtiofsSharedDirPathConfig := vz.NewVirtioFileSystemDeviceConfiguration(sharedDirName)
	virtiofsSharedDirPathConfig.SetDirectoryShare(vz.NewSingleDirectoryShare(vz.NewSharedDirectory(sharedDirPath, false)))

	config.SetDirectorySharingDevicesVirtualMachineConfiguration(
		[]vz.DirectorySharingDeviceConfiguration{
			virtiofsSharedDirPathConfig,
		},
	)

Thanks for the PR, I'm a fan to say the least. 🙂

@ghost
Copy link

ghost commented Mar 15, 2022

Another thing that occurred to me is that this brings the macOS requirement up to 12.0.0+ per the Virtualization Framework documentation:

vz/README.md

Line 16 in 71d0477

- Higher or equal to macOS Big Sur (11.0.0)

Something I don't quite know the answer to is: what happens if there is no checks in place when compiling this on older macOS versions?

There's this tool called vmcli that does version checking in the code. There's also some workarounds notated.

Is this something that should be accounted for in this codebase?

@cfergeau
Copy link
Contributor

Something I don't quite know the answer to is: what happens if there is no checks in place when compiling this on older macOS versions?

Trying to build your branch on macOS 11:

$ make
go build -o virtualization .
# github.com/Code-Hex/vz
virtualization.m:205:46: warning: 'setDirectorySharingDevices:' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZVirtualMachineConfiguration.h:116:79: note: 'setDirectorySharingDevices:' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 11.0.0
virtualization.m:205:46: note: enclose 'setDirectorySharingDevices:' in an @available check to silence this warning
virtualization.m:586:5: warning: 'VZSharedDirectory' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZSharedDirectory.h:16:12: note: 'VZSharedDirectory' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 11.0.0
virtualization.m:586:5: note: enclose 'VZSharedDirectory' in an @available check to silence this warning
virtualization.m:590:17: warning: 'VZSharedDirectory' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZSharedDirectory.h:16:12: note: 'VZSharedDirectory' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 11.0.0
virtualization.m:590:17: note: enclose 'VZSharedDirectory' in an @available check to silence this warning
virtualization.m:603:14: warning: 'VZSingleDirectoryShare' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZSingleDirectoryShare.h:21:12: note: 'VZSingleDirectoryShare' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 11.0.0
virtualization.m:603:14: note: enclose 'VZSingleDirectoryShare' in an @available check to silence this warning
virtualization.m:603:63: warning: 'VZSharedDirectory' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZSharedDirectory.h:16:12: note: 'VZSharedDirectory' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 11.0.0
virtualization.m:603:63: note: enclose 'VZSharedDirectory' in an @available check to silence this warning
virtualization.m:614:14: warning: 'VZMultipleDirectoryShare' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZMultipleDirectoryShare.h:21:12: note: 'VZMultipleDirectoryShare' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 11.0.0
virtualization.m:614:14: note: enclose 'VZMultipleDirectoryShare' in an @available check to silence this warning
virtualization.m:614:91: warning: 'VZSharedDirectory' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZSharedDirectory.h:16:12: note: 'VZSharedDirectory' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 11.0.0
virtualization.m:614:91: note: enclose 'VZSharedDirectory' in an @available check to silence this warning
virtualization.m:625:5: warning: 'VZVirtioFileSystemDeviceConfiguration' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZVirtioFileSystemDeviceConfiguration.h:21:12: note: 'VZVirtioFileSystemDeviceConfiguration' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 11.0.0
virtualization.m:625:5: note: enclose 'VZVirtioFileSystemDeviceConfiguration' in an @available check to silence this warning
virtualization.m:628:17: warning: 'VZVirtioFileSystemDeviceConfiguration' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZVirtioFileSystemDeviceConfiguration.h:21:12: note: 'VZVirtioFileSystemDeviceConfiguration' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 11.0.0
virtualization.m:628:17: note: enclose 'VZVirtioFileSystemDeviceConfiguration' in an @available check to silence this warning
virtualization.m:638:7: warning: 'VZVirtioFileSystemDeviceConfiguration' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZVirtioFileSystemDeviceConfiguration.h:21:12: note: 'VZVirtioFileSystemDeviceConfiguration' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 11.0.0
virtualization.m:638:7: note: enclose 'VZVirtioFileSystemDeviceConfiguration' in an @available check to silence this warning
virtualization.m:638:64: warning: 'VZDirectoryShare' is only available on macOS 12.0 or newer [-Wunguarded-availability-new]
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks/Virtualization.framework/Headers/VZDirectoryShare.h:24:12: note: 'VZDirectoryShare' has been marked as being introduced in macOS 12.0 here, but the deployment target is macOS 11.0.0
virtualization.m:638:64: note: enclose 'VZDirectoryShare' in an @available check to silence this warning

@paulcacheux
Copy link
Contributor Author

This is a great point, maybe we could use a build tag to gate this part of the API ? (per feature or per macOS version)

@ghost
Copy link

ghost commented Mar 17, 2022

Not sure how build tagging would work. In various containers related packages I see stubs put in place for OS's in Go.

What would probably work is placing stubs in the Obj-C code so that the function for macOS 12+ is there and anything <12 is a placeholder that might throw a warning to the CLI or something.

@Code-Hex
Copy link
Owner

Code-Hex commented Apr 6, 2022

I get a new macOS environment recently. I will check this PR after published current code

@cfergeau
Copy link
Contributor

I did more work on top of this PR to add macOS version checks
https://github.com/cfergeau/vz/tree/shared-folders

@cfergeau
Copy link
Contributor

@Code-Hex what are your plans with respects to this PR? Anything we can do to help move it forward?

cfergeau added a commit to cfergeau/vfkit that referenced this pull request Jul 21, 2022
This makes it possible to share files with the VM. For now it's limited
to one shared directory, which should be enough for our initial needs.

Code-Hex/vz#28 is not merged yet, so this relies
on a Code-Hex/vz fork in my own GitHub namespace which has the shared
folder API.
cfergeau added a commit to cfergeau/vfkit that referenced this pull request Jul 21, 2022
This makes it possible to share files with the VM. For now it's limited
to one shared directory, which should be enough for our initial needs.

Code-Hex/vz#28 is not merged yet, so this relies
on a Code-Hex/vz fork in my own GitHub namespace which has the shared
folder API.

This fixes crc-org#2
@ghost
Copy link

ghost commented Jul 22, 2022

Still watching in anticipation myself. If this needs further testing, I have an M1 and Intel MBP available now.

cfergeau added a commit to cfergeau/vfkit that referenced this pull request Jul 25, 2022
This makes it possible to share files with the VM. For now it's limited
to one shared directory, which should be enough for our initial needs.

Code-Hex/vz#28 is not merged yet, so this relies
on a Code-Hex/vz fork in my own GitHub namespace which has the shared
folder API.

This fixes crc-org#2
cfergeau added a commit to crc-org/vfkit that referenced this pull request Jul 25, 2022
This makes it possible to share files with the VM. For now it's limited
to one shared directory, which should be enough for our initial needs.

Code-Hex/vz#28 is not merged yet, so this relies
on a Code-Hex/vz fork in my own GitHub namespace which has the shared
folder API.

This fixes #2
@Code-Hex
Copy link
Owner

I'm sorry toooooooo late... 🙇

And Thanks everyone for watching this PR.

I've prepared the version strategy between macOS Big Sur and Monterey. and I wrote about this in README.

I will merge this PR

@Code-Hex
Copy link
Owner

@paulcacheux Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants