kr / hk

Fast Heroku client
https://hk.heroku.com/
77 stars 6 forks source link

use execpath for cross-platform path determination #55

Closed bgentry closed 10 years ago

bgentry commented 10 years ago

Pull in execpath dependency for cross-platform path determination. Fixes #47.

This should also allow for full cross-compilation. Locally on darwin-amd64, I am now able to cross-compile for all platforms:

$ go-all build
go-darwin-386 build
go-darwin-amd64 build
go-freebsd-386 build
go-freebsd-amd64 build
go-freebsd-arm build
go-linux-386 build
go-linux-amd64 build
go-linux-arm build
go-windows-386 build
go-windows-amd64 build

@cyberdelia can you confirm that this now builds and runs for you?

Next steps will be to update our release tooling (hkdist) to enable cross-compilation and publishing of cross-compiled builds.

Also, @kr, with external dependencies like this, it might be a good time to consider working godep into this.

/cc @inconshreveable in case he has thoughts on his package getting used :)

kr commented 10 years ago

Nice. Another option would be to remove the code that depends on the binary path. The "feature" that uses this code is questionable to begin with.

Package execpath looks good. One thing that worries me is GetArg0, which uses the working directory, which has nothing to do with the executing program, even by convention. If we use execpath, I suggest sticking to function GetNative only.

kr commented 10 years ago

Watch out for cross-compiling with this package:

https://github.com/inconshreveable/go-execpath/blob/master/darwin.go#L10

kr commented 10 years ago

Yet another option would be to add a build constraint so this code only gets compiled when developing locally, and not when cross-compiling for release, since it just so happens that we use the executable path only for locally-compiled binaries.

bgentry commented 10 years ago

@kr ah, I was under the impression that you needed to know the executable path in order to update it in place. Is that not true? Can you elaborate on why we needed it in the first place?

inconshreveable commented 10 years ago

Sadly, go-execpath can only provide GetNative() on darwin when it's not cross-compiled because the darwin implementation requires cgo. If you have any suggestions for getting around this requirement, I'd be happy to accept a patch. I've considered the following possibilities:

  1. Create a Makefile which actually does all of the magic to make cross-compilation work. (you'd still need the darwin sources and libraries on your box to link against)
  2. Call NSGetExecutablePath() by dynamically linking to the library at runtime. This is how all Win32 API calls are made in golang windows programs. Unfortunately, I have no idea if this is even possible, or if it is, how to do it.
  3. Natively compile go-execpath on every platform and the host the .a files somewhere. Write a script that you can run which will pull them down and sticks them in your pkg/ directory with the correct mtime so that they don't get regenerated and you get the natively-compiled versions available for cross-compilation.

None of them are exactly trivial.

re @kr: Are you suggesting that GetArg0() is broken because the working directory means nothing because the program might have changed its working directory?

I also like your use of _NSGetExecutablePath more than mine, I might steal that code.

kr commented 10 years ago

@inconshreveable about GetArg0, it's not just that the program might have changed its working directory. Consider hk as an example. The binary is usually installed in /usr/local/bin/hk, and available in the search path. When the user runs this command, they are usually in a source code directory for a Heroku app, for example $HOME/src/myapp. If there happens to be a file called 'hk' there, it'll get picked up by GetArg0. That function would be much safer if you check that the arg has a / in it, which generally makes the shell skip the search path and instead interpret the command relative to .. It'd still have the sharp corners you document in the readme (the working directory may have changed; the first arg might not even be a file name at all).

@bgentry binPath is only used to decide if the current dev binary has expired. It's only an issue when hk is compiled from source, not for release. Honestly I'm starting to think we should just remove that expiration logic altogether.

cyberdelia commented 10 years ago

@kr I also think we should just kill the dev binary expiration too.

bgentry commented 10 years ago

I talked it over with @kr. We're going to go ahead and scrap the dev binary expiration thing for now, which will eliminate the need for this dependency. If it becomes a real problem, we'll take it from there.

bgentry commented 10 years ago

Closing this, #59 removes the dev binary expiration so we can get cross-compilation.