Minor fix for Windows

Minor fix for Windows

Our code uses the environment variable $HOME everywhere, but on windows the same logical environment variable is actually HOMEPATH. So, a quick patch at the beginning of the juju client makes $HOME work as expected.

https://code.launchpad.net/~natefinch/juju-core/008-windows/+merge/181916

(do not edit description out of merge proposal)

[natefinch, 2013-08-23 20:10:03]

Please take a look.

[jameinel, 2013-08-25 10:01:24]

Main doesn't seem like quite the right place for this.

We should be fairly centralized about how we deal with $HOME. Specifically, we
should only use $HOME in order to determine a $JUJU_HOME when it isn't set. As
such, I'd rather just move this into that code.

See juju/conn.go where it tries JUJU_HOME and then falls back to HOME. If you
just: grep -rnI "\<HOME"
you can see that there isn't a *huge* number of places where we touch it, and it
would be reasonable to pull those out to something in utils.

[jameinel, 2013-08-25 10:03:32]

I should mention, *I* didn't notice this because all of my Windows shells have
$HOME set. (Possibly from cygwin being installed.)

Actually, the other bit which makes this NOT LGTM, is that it isn't complete:
http://support.microsoft.com/kb/101507

Says that you need %HOMEDRIVE%%HOMEPATH%. Just having HOMEPATH will set it to
something like \Users\jameinel, but you need that C: otherwise when you are on
D: it will be trying the wrong directory.

[natefinch, 2013-08-30 19:44:03]

Please take a look.

[jameinel, 2013-08-30 20:09:45]

I'm a bit surprised to see the timeout change and the full windows installer end
up in the same patch as the changes to HOME.

I might have missed a resolution on the discussion about the timeout change, but
I thought we didn't actually decide to switch to 30s (mostly because of the
scripts-using-juju then have to poll themselves).

The actual $HOME changes and the windows installer scripts seem fine to me.

https://codereview.appspot.com/13079045/diff/6001/juju/osenv/vars_windows.go
File juju/osenv/vars_windows.go (right):

https://codereview.appspot.com/13079045/diff/6001/juju/osenv/vars_windows.go#...
juju/osenv/vars_windows.go:26: }
I personally prefer the pattern of "WinHome()" as separate functions that can be
tested cross platform, and then only 1 minor bit that actually binds which bit
you call based on platform.

That way the bot that lands everything has a *chance* to catch when someone
developing on Linux accidentally breaks something on Windows.

But it is a small chance, so I won't be too picky about it.

https://codereview.appspot.com/13079045/diff/6001/scripts/win-installer/LICEN...
File scripts/win-installer/LICENCE.txt (right):

https://codereview.appspot.com/13079045/diff/6001/scripts/win-installer/LICEN...
scripts/win-installer/LICENCE.txt:1: GNU AFFERO GENERAL PUBLIC LICENSE
Does it actually *have* to be 2 copies of this file? You can't just reference
the version of the file 2 directories up?

https://codereview.appspot.com/13079045/diff/6001/scripts/win-installer/modpa...
File scripts/win-installer/modpath.iss (right):

https://codereview.appspot.com/13079045/diff/6001/scripts/win-installer/modpa...
scripts/win-installer/modpath.iss:1: //
----------------------------------------------------------------------------
I didn't actually vet this file at all, since I assume you are copying it from
somewhere else. The license seems ok to use here.

https://codereview.appspot.com/13079045/diff/6001/state/open.go
File state/open.go (right):

https://codereview.appspot.com/13079045/diff/6001/state/open.go#newcode59
state/open.go:59: Timeout: 30 * time.Second,
I'm pretty sure this wasn't approved in the discussions we have. I *definitely*
don't expect it to come in on a patched based on "Minor fix for Windows".

[natefinch, 2013-08-30 20:24:47]

On 2013/08/30 20:09:45, jameinel wrote:
> I'm a bit surprised to see the timeout change and the full windows installer
end
> up in the same patch as the changes to HOME.
>
> I might have missed a resolution on the discussion about the timeout change,
but
> I thought we didn't actually decide to switch to 30s (mostly because of the
> scripts-using-juju then have to poll themselves).

The timeout change is not supposed to be committed at all. I somehow missed it
when I was reviewing the changes. I'll back it out.

So, yes, I probably should have put the installer in a separate commit. If you'd
like I can back it out, it's just kind of a pain to do so.

 
> The actual $HOME changes and the windows installer scripts seem fine to me.
> 
> https://codereview.appspot.com/13079045/diff/6001/juju/osenv/vars_windows.go
> File juju/osenv/vars_windows.go (right):
> 
>
https://codereview.appspot.com/13079045/diff/6001/juju/osenv/vars_windows.go#...
> juju/osenv/vars_windows.go:26: }
> I personally prefer the pattern of "WinHome()" as separate functions that can
be
> tested cross platform, and then only 1 minor bit that actually binds which bit
> you call based on platform.
> 
> That way the bot that lands everything has a *chance* to catch when someone
> developing on Linux accidentally breaks something on Windows.
> 
> But it is a small chance, so I won't be too picky about it.

I actually started that way, but he problem is that several of the os-specific
functions work completely differently on Windows and Linux, so you can't really
test them on both (for example filepath.VolumeName always returns an empty
string on Linux).

I figure, if we're going to start supporting Windows, we should probably start
testing on Windows, even if the tests are limited in scope.
 
>
https://codereview.appspot.com/13079045/diff/6001/scripts/win-installer/LICEN...
> File scripts/win-installer/LICENCE.txt (right):
> 
>
https://codereview.appspot.com/13079045/diff/6001/scripts/win-installer/LICEN...
> scripts/win-installer/LICENCE.txt:1: GNU AFFERO GENERAL PUBLIC LICENSE
> Does it actually *have* to be 2 copies of this file? You can't just reference
> the version of the file 2 directories up?

It actually has to be in the same directory. This is a limitation of innosetup.
I figured the license wasn't likely to change any time soon.
 
>
https://codereview.appspot.com/13079045/diff/6001/scripts/win-installer/modpa...
> File scripts/win-installer/modpath.iss (right):
> 
>
https://codereview.appspot.com/13079045/diff/6001/scripts/win-installer/modpa...
> scripts/win-installer/modpath.iss:1: //
> ----------------------------------------------------------------------------
> I didn't actually vet this file at all, since I assume you are copying it from
> somewhere else. The license seems ok to use here.

Yep, this was copied from ye olde internet. Evidently adding things to the
Windows path is oddly difficult in Windows installers... but this guy's script
was mentioned in several places as being the fix, and in testing it works great.

> https://codereview.appspot.com/13079045/diff/6001/state/open.go
> File state/open.go (right):
> 
> https://codereview.appspot.com/13079045/diff/6001/state/open.go#newcode59
> state/open.go:59: Timeout: 30 * time.Second,
> I'm pretty sure this wasn't approved in the discussions we have. I
*definitely*
> don't expect it to come in on a patched based on "Minor fix for Windows".

As above, my apologies. this was not supposed to go in.  I'll rip it out.

[natefinch, 2013-08-30 20:27:54]

Please take a look.

[jameinel, 2013-09-03 12:24:13]

I have some comments about where I'd like to see things, but you can probably
land this and we can iterate from there.

LGTM

https://codereview.appspot.com/13079045/diff/15001/juju/conn.go
File juju/conn.go (left):

https://codereview.appspot.com/13079045/diff/15001/juju/conn.go#oldcode392
juju/conn.go:392: }
Can we move these functions into osenv hidden behind osenv.JujuHome() ?

https://codereview.appspot.com/13079045/diff/15001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/13079045/diff/15001/juju/conn.go#newcode385
juju/conn.go:385: if runtime.GOOS == "windows" {
I would be really tempted to put all of this if/else logic into a JujuHome
function that just proxies for jujuHomeWin or jujuHomeLinux as appropriate.
It doesn't seem like runtime.GOOS should be evalated in juju/conn.go

[natefinch, 2013-09-03 20:01:06]

Please take a look.

[jameinel, 2013-09-04 04:22:37]

LGTM, though I think you removed the import check accidentally.

https://codereview.appspot.com/13079045/diff/20001/juju/osenv/vars_test.go
File juju/osenv/vars_test.go (right):

https://codereview.appspot.com/13079045/diff/20001/juju/osenv/vars_test.go#ne...
juju/osenv/vars_test.go:22: 
You had a:
func (*importSuite) TestDependencies(c *gc.C) {

before. Did you intend to get rid of that one?

[natefinch, 2013-09-04 14:35:47]

Please take a look.

[natefinch, 2013-09-04 14:43:08]

On 2013/09/04 04:22:37, jameinel wrote:
> LGTM, though I think you removed the import check accidentally.
> 
> https://codereview.appspot.com/13079045/diff/20001/juju/osenv/vars_test.go
> File juju/osenv/vars_test.go (right):
> 
>
https://codereview.appspot.com/13079045/diff/20001/juju/osenv/vars_test.go#ne...
> juju/osenv/vars_test.go:22: 
> You had a:
> func (*importSuite) TestDependencies(c *gc.C) {
> 
> before. Did you intend to get rid of that one?

That's in package_test.go, it wasn't removed.  It's funny, the diff says I added
that, but I totally didn't. Maybe I moved it by accident or something.

[jameinel, 2013-09-04 16:15:45]

If I LGTM a patch with some minor comments, and you feel like you've satisfied
those requirements, you don't need to wait for me to look at it again before
landing it.

With timezone differences that just delays everything by an extra day.

[mue, 2013-09-06 09:58:43]

LGTM