This repository has been archived by the owner. It is now read-only.

Add VirtualBox shared folders creation #258

Merged
merged 1 commit into from Sep 23, 2014

Conversation

Projects
None yet
7 participants
@tianon
Copy link
Contributor

tianon commented Sep 17, 2014

This includes three new commandline flags to control this behavior, for those who don't want the default (which is currently automount /Users for OS X, and automount C:\Users for Windows, but will change to automount homeDir() at some point in the future):

  • --no-vbox-share=false: Disable VirtualBox share creation during 'init'
  • --vbox-share="": Change VirtualBox shared folder created during 'init' from the default autodetection logic
  • --vbox-share-name="": Change VirtualBox shared folder name created during 'init' from the default of being automatically determined based on the share directory path
@tianon

This comment has been minimized.

Copy link
Contributor

tianon commented Sep 17, 2014

@SvenDowideit since these options are for "advanced" users, do you think the help text is sufficient for documenting them?

@tiborvass

This comment has been minimized.

Copy link

tiborvass commented Sep 17, 2014

@tianon ❤️

1 similar comment
@bfirsh

This comment has been minimized.

Copy link
Contributor

bfirsh commented Sep 17, 2014

@tianon ❤️

@aanand

This comment has been minimized.

Copy link
Contributor

aanand commented Sep 17, 2014

tumblr_mkfm9uwhzw1qzp9weo1_400

@@ -89,6 +93,10 @@ func ConfigFlags(B2D *driver.MachineConfig, flags *flag.FlagSet) error {
}
flags.StringVar(&cfg.VBM, "vbm", cfg.VBM, "path to VirtualBox management utility.")

flags.BoolVar(&cfg.disableShare, "no-vbox-share", false, "Disable VirtualBox share creation during 'init'")

This comment has been minimized.

@gmlewis

gmlewis Sep 17, 2014

Contributor

The double-negative tweaks my brain... how about:
.., &cfg.enableShare, "vbox-share", true, "Enable...
and the next line could be:
..., "vbox-share-dir",

This comment has been minimized.

@tianon

tianon Sep 17, 2014

Contributor

That's fair, although can't you use a boolean flag like --no-vbox-share directly (ie, no =true, since that's implied)? Also, we already have --no-dummy="": Example parameter for the dummy driver., so this isn't without precedent.

This comment has been minimized.

@tianon

tianon Sep 17, 2014

Contributor

So does that change your mind, or should I go rename some options? 😄

I'm fine either way. That would have the benefit of making them all sort together, so that'd be nice.

This comment has been minimized.

@SvenDowideit

SvenDowideit Sep 18, 2014

Contributor

yes, please do not use negative options - they get ugly fast, and are much harder to write about - --no-dummy isn't really something anyone has seen :)

@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Sep 18, 2014

oh. this isn't boot2docker share <dir>.

@tianon tianon force-pushed the tianon:vbox-sharing branch from c8d2bd7 to 30edfb6 Sep 18, 2014

@tianon

This comment has been minimized.

Copy link
Contributor

tianon commented Sep 18, 2014

Updated:

$ ./boot2docker-linux help 2>&1 | grep vbox
      --vbox-share=true: Enable VirtualBox share creation during 'init'
      --vbox-share-dir="": Change VirtualBox shared folder created during 'init' from the default autodetection logic
      --vbox-share-name="": Change VirtualBox shared folder name created during 'init' from the default of being automatically determined based on the share directory path
@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Sep 18, 2014

testing on OSX by running make binary in a docker checkout (having removed the BUNDLES_DIR := $(if $DOCKERHOST

weirdly, now that I'm using the new master b2d.iso, my vm is getting no nameserver set - need to find out if that's the iso, or some local fluke :(

fixed in #545

@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Sep 18, 2014

yup, make cross works on OSX, and its not horribly slower.

@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Sep 18, 2014

truely horrible UX on windows.

  1. the user doesn't know what random path is mounted - you could tell them in boot2docker up
  2. boot2docker ssh docker run -v $(pwd):/data busybox sh fails hard - its got spaces in it.
  3. I wonder how many others have their Users dir not on the small c:\, and rather on the huge data disk..

remind me again why its hardcoded at vm boot, rather than setting things up as boot2docker share, which may have defaults in boot2docker up? Oh yeah, can't change vbox shares once the vm is booted. yay

@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Sep 18, 2014

I've given you tianon#1

now the help is mildly more helpful, and probably doesn't make the linux version die horribly

      --vbox-share=true: Enable VirtualBox share creation during 'init'
      --vbox-share-dir="/Users": Change VirtualBox shared folder created during 'init' from the default autodetection logic
      --vbox-share-name="Users": Change VirtualBox shared folder name created during 'init' from the default of being automatically determined based on the share directory path

LGTM once you merge my PR - well, more like 'its a beginning'

I'd also load the defined shares into the Machine Config so they print in boot2docker info.....

@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Sep 19, 2014

@tianon we need something like

--- a/Makefile
+++ b/Makefile
@@ -2,7 +2,7 @@

 # to allow `make BINDDIR=. shell` or `make BINDDIR= test`
 # (default to no bind mount if DOCKER_HOST is set)
-BINDDIR := $(if $(DOCKER_HOST),,bundles)
+BINDDIR := bundles
 # to allow `make DOCSPORT=9000 docs`
 DOCSPORT := 8000
@bjaglin

This comment has been minimized.

Copy link
Contributor

bjaglin commented Sep 19, 2014

I guess the commit/PR title is a bit misleading - I was happy to see that several folders could be passed (which would fulfil my need for mounting both /Users and the /var/folders, as I use /var/folders on OS X in builder containers as temporary input/output across host and containers), but that's not the case.

I understand that it would make the current 2-flags-based UI even harder to allow repeated shares though... Maybe allowing repeated --vbox-share=/vboxhost/path[:/vboxguest/path] would do the trick?

@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Sep 22, 2014

@bjaglin true

perhaps a better UX would be boot2docker init --share Users:/Users --share folders:/var/folders

which we can then extend as boot2docker init --share users:sshfs://otherhost/home --share data:nfs://bighost:/export1/data

@tianon

This comment has been minimized.

Copy link
Contributor

tianon commented Sep 22, 2014

Ah, I really like that; nice and simple. Although, I'd swap the order and use ; instead of : (since : is valid in Windows paths, and they're already special-cased enough as it is), and that allows for the label to be optional (ie, boot2docker init --vbox-share /Users). Also, naming it --vbox-share punts on the question of future compatibility, and we can discuss a general --share orthogonally.

The only problem left is that the ever-illustrious pflag doesn't support list arguments that I can find...

@tianon

This comment has been minimized.

Copy link
Contributor

tianon commented Sep 22, 2014

Ah, you have to jump through all sorts of hoops. Ok, working on it.

@bfirsh

This comment has been minimized.

Copy link
Contributor

bfirsh commented Sep 22, 2014

I'd say we should use :for the parallels with Docker's -v. It's escapable in Windows paths, surely?

@tianon

This comment has been minimized.

Copy link
Contributor

tianon commented Sep 22, 2014

boot2docker --vbox-share C:\Users would then parse into sharing C as \Users, without special casing, which is why I'm not a fan - we could instead require labels to be explicit (ie, label:path always), but that seems pretty gross for UX too, especially when we want to encourage them to be mostly the same (that's kind of the whole point).

@tianon

This comment has been minimized.

Copy link
Contributor

tianon commented Sep 22, 2014

Perhaps we instead check to see if it's a valid directory, and if it isn't, split it and try again?

@bfirsh

This comment has been minimized.

Copy link
Contributor

bfirsh commented Sep 22, 2014

Oh, yeah. I forgot about C:\. Erm. Maybe split by : rightwards? That's also pretty gross.

@tianon tianon force-pushed the tianon:vbox-sharing branch from 30edfb6 to 081a853 Sep 22, 2014

@tianon

This comment has been minimized.

Copy link
Contributor

tianon commented Sep 22, 2014

Updated to use =, for simplicity:

This includes a new commandline flag to control this behavior, for those who don't want the default (which is currently automount /Users for OS X, and automount C:\Users for Windows, but will change to automount homeDir() at some point in the future).

The commandline flag (--vbox-share=shareDirectory[=shareLabel]) defines a series of essentially key-value pairs for VirtualBox shared folders in the format shareDirectory[=shareLabel], where shareLabel becomes the in-guest mount location (and it defaults to the value of shareDirectory).

It also supports --vbox-share=disable in order to explicitly disable any automatic sharing.

For example:

$ ./boot2docker-linux --vbox-share=/home=Users -v init
...
2014/09/22 12:26:31 executing: VBoxManage sharedfolder add boot2docker-vm --name Users --hostpath /home --automount
...
@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Sep 23, 2014

bash-3.2$ ./boot2docker-v1.2.0-darwin-amd64  help
Usage: ./boot2docker-v1.2.0-darwin-amd64 [<options>] <command> [<args>]
.....
      --vbox-share=[]: List of directories to share during 'init' via VirtualBox Guest Additions, with optional labels (defaults to '/Users' if unspecified; use 'disable' to prevent the default share from being created)

can we change to:

      --vbox-share=[]: (defaults to '/Users') List of directories to share during 'init' via VirtualBox Guest Additions, with optional labels (use 'disable' to prevent the default share from being created)

(the default is lost in the mass of text atm)

mmm, this text also implies that boot2docker init --vbox-share=/opt --vbox-share=/var would result in 3 shares being created.

I guess you're lucky that boot2docker config doesn't print out the flags added by a driver :/ I wonder what happens if you set it in the profile file.

@tianon

This comment has been minimized.

Copy link
Contributor

tianon commented Sep 23, 2014

It probably works from the profile file, since that'd just call "set" on each flag defined, right?

How's this? --vbox-share=[]: (defaults to '/Users=Users' if no shares are specified; use 'disable' to explicitly prevent any shares from being created) List of directories to share during 'init' via VirtualBox Guest Additions, with optional labels

Add VirtualBox shared folders creation
This includes a new commandline flag to control this behavior, for those who don't want the default (which is currently automount `/Users` for OS X, and automount `C:\Users` for Windows, but will change to automount `homeDir()` at some point in the future).

The commandline flag (`--vbox-share=shareDirectory[=shareLabel]`) defines a series of essentially key-value pairs for VirtualBox shared folders in the format `shareDirectory[=shareLabel]`, where `shareLabel` becomes the in-guest mount location (and it defaults to the value of `shareDirectory`).

It also supports `--vbox-share=disable` in order to explicitly disable any automatic sharing.

@tianon tianon force-pushed the tianon:vbox-sharing branch from 081a853 to b161571 Sep 23, 2014

@tianon

This comment has been minimized.

Copy link
Contributor

tianon commented Sep 23, 2014

So that new wording is fine, and can be tweaked even more by subsequent PRs? 😉

@tianon

This comment has been minimized.

Copy link
Contributor

tianon commented Sep 23, 2014

Since the wording was the main gripe here, I'm going to merge now and we can iterate as needed or desired from there. 😉

tianon added a commit that referenced this pull request Sep 23, 2014

Merge pull request #258 from tianon/vbox-sharing
Add VirtualBox shared folders creation

@tianon tianon merged commit 6a05b48 into boot2docker:master Sep 23, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@tianon tianon deleted the tianon:vbox-sharing branch Sep 23, 2014

SvenDowideit pushed a commit to SvenDowideit/boot2docker-cli that referenced this pull request Nov 10, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.