[golang-dev] code review 6736069: runtime, syscall, os: add os.ExecPath() (string, error) (issue 6736069)

I'd like to solicit opinion about the name and signature of the new
func.

package os
import "os"

FUNCTIONS
func ExecPath() (string, error)
ExecPath returns the absolute pathname of the current executing
file; if
error occurs, it will return whatever it current gets and the error.

https://codereview.appspot.com/6736069/

I like the API addition, but the name sounds too imperative to me (like
it's going to try go "exec" the provided path). It doesn't sound like a
getter.

How about GetExecPath() or GetProcPath() ?

Go generally avoids the prefix "Get". The existing functions named "Get"
in the os package are named after common libc/syscall functions.

how about os.CurrentExecutablePath() ?

this function won't be frequently used, so a slightly longer name is
acceptable imo.

"Current" implies that it can change over time. Which I guess is true, if
chroot is called?

CurrentExecutablePath seems okay to me, if a little long, but I defer to
others.

https://codereview.appspot.com/6736069/diff/8005/src/pkg/os/execpath_windows.go
File src/pkg/os/execpath_windows.go (right):

https://codereview.appspot.com/6736069/diff/8005/src/pkg/os/execpath_windows.go#newcode11
src/pkg/os/execpath_windows.go:11: // ExecPath returns the absolute
pathname of the current
I would only document this once, in a common file, like:

// ExecPath returns ...
func ExecPath() (string, error) {
return execPath()
}

And then implement the lowercase OS-specific execPath in
+build-restricted files.

https://codereview.appspot.com/6736069/

What about "ProgramPath"? unless program is ambiguous and could refer to
source code?

http://codereview.appspot.com/6736069/

i think it's ok. but i'd prefer StartupProgramPath
now.

if chroot is called?
unfortunately, at least on some systems the value won't change over time.
others.
how about StartupExecutablePath() then?
it emphasizes that the returned value is corresponding
to the moment the program starts, which is correct
meaning.

oh, it seems we must store cwd at program start to
give accurate path (some machenisms can return
relative paths).

LGTM,

but wait for others.

Alex


http://codereview.appspot.com/6736069/diff/16026/src/pkg/os/execpath_test.go
File src/pkg/os/execpath_test.go (right):

http://codereview.appspot.com/6736069/diff/16026/src/pkg/os/execpath_test.go#newcode12
src/pkg/os/execpath_test.go:12: oexec "os/exec"
why rename?

http://codereview.appspot.com/6736069/diff/16026/src/pkg/os/execpath_test.go#newcode21
src/pkg/os/execpath_test.go:21: t.Errorf("ExecPath failed: %v", err)
s/Errorf/Fatalf/

then you will not need the following "return".

Same everywhere else.

http://codereview.appspot.com/6736069/diff/16026/src/pkg/syscall/syscall_windows.go
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/6736069/diff/16026/src/pkg/syscall/syscall_windows.go#newcode774
src/pkg/syscall/syscall_windows.go:774: func GetModuleFileName(handle
Handle) (string, error) {
Please, move this to os. Provide GetModuleFileName, as it is published
in windows api, instead.

http://codereview.appspot.com/6736069/diff/16026/src/pkg/syscall/syscall_windows.go#newcode783
src/pkg/syscall/syscall_windows.go:783: if r == n {
I would revert the condition and break first. Then you will not need
continue.

http://codereview.appspot.com/6736069/

thank you.

On Oct 24, 2012 12:19 PM, wrote:
>
http://codereview.appspot.com/6736069/diff/16026/src/pkg/os/execpath_test.go#newcode12
because the os package has a function named exec.

>
http://codereview.appspot.com/6736069/diff/16026/src/pkg/syscall/syscall_windows.go#newcode774
i think os is just for portable functions.
doesn't this function belong to pkg syscall?

i can export the original verion of the api in
syscall, but the user still needs to write some
code to use it in Go properly.

btw, what's your opinion about the new api?
do you have suggestions about its name?

published
Do not export os.GetModuleFileName. You already have *_windows.go file -
put all your code in there. And do not export it from os.

Yes, please leave api as per Microsoft documentation (just leave //sys
line). If someone decides to use it in some particular way, it is as
documented.
Getppath. Just joking.
I do not have good suggestions.

Alex

http://codereview.appspot.com/6736069/

code in the
the fact that this API depends on procfs doesn't mean you cannot run Go
programs
without procfs, you just cannot use the new API.
Why? procfs is standard part of Linux, and lots of things will break if
it's missing.
I think you must understand this implication when you chroot to a
directory
without procfs.

for instance, the os package is already using /proc/sys/kernel/hostname
to
get the hostname. Because procfs is a integrated part of linux, there
are problems
that are not solvable without it (for example, ps(1)), AFAIK.

However, if you could provide a portable way to implement this API on
Linux
without resorting to procfs, i'd like to adapt it.
(by portable, I mean it needs to support Linux 2.6.23+)

https://codereview.appspot.com/6736069/

I'd personally like this to go it. If there is a dependency on /proc on
linux, then we can document that as a BUG for now(?). I've already coded
similar code for linux,windows, and osx (using cgo), so I personally
require and use the functionality this provides on multiple platforms. I
think this would also be fine for a go get package, but osx requires
runtime changes for this to work without cgo.

name suggestion? ProcessFilePath(), ProcessPath()

Use cases for this call:
* Get a path to the exec when creating a "service" on windows, upstart,
launchd.
* I often bundle resources next to the exec for easy deployment.
Without this I have to watch my working directory when I call it. Not
horrible,
but much easier just to get the path the OS already knows, no guesswork
required,
same deployment on all systems.

.. I understand these use cases are not relevant for many projects for many
people, but they
are useful to me, especially when I have a number of smaller one off
projects on different platforms,
it greatly simplifies my code and deployment to use this functionality...

ProcessPath is the best name I've heard.

PTAL. Patch Set 22 is using the name ProgramPath.

I've tested on Mac OS X 10.6/amd64, Linux/amd64, FreeBSD 9/amd64,
NetBSD 6/amd64, Windows/386.
(OpenBSD is known to fail, because it doesn't provide procfs by default,
and I couldn't find an alternative way)

https://codereview.appspot.com/6736069/

https://codereview.appspot.com/6736069/diff/32004/src/pkg/os/progpath_test.go
File src/pkg/os/progpath_test.go (right):

https://codereview.appspot.com/6736069/diff/32004/src/pkg/os/progpath_test.go#newcode12
src/pkg/os/progpath_test.go:12: oexec "os/exec"
You didn't tell me why you are renaming "os/exec" to oexec.

https://codereview.appspot.com/6736069/

https://codereview.appspot.com/6736069/#msg13

there is a function named exec in pkg os_test (file os_test.go).

Ah, yes. Thank you. :-)

Alex

https://codereview.appspot.com/6736069/

Sorry, but ProcessPath sounds like you are processing a path. Let's
wait on this until Rob's back (next week).

how about "Executable"?

without mentioning path?

is your name minuxName?

-rob

good point. PTAL.

btw, how to implement this on Plan 9?

https://codereview.appspot.com/6736069/

The file names are inappropriate now.

For Plan 9, it depends what you want this for. /proc/PID/text is
always a working name for the executable. Otherwise I don't know if
it's possible.

-rob

func executable() (string, error) {
f, err := Open("/proc/" + itoa(Getpid()) + "/text")
if err != nil {
return "", err
}
defer f.Close()
return syscall.Fd2path(int(f.Fd()))
}

do you have suggestions for the filenames? i think executable is
a bit too long, and exec might confuse with the exec syscall.
how about the original execpath?
I've thought about /proc/PID/text, in fact it can pass the test, but
i think the main use of this to determine the original binary's path
so that the binary can access its bundled asset files, for example.

i'd also like to test we can indeed find the other files around the
binary, but given the current go test's behavior, this is difficult
to test.

https://codereview.appspot.com/6736069/

PTAL.

Patch Set 24 is the same as Patch Set 23 except
s/progpath/executable/ in filenames.

https://codereview.appspot.com/6736069/

i'm still not convinced by the idea behind this. the idea seems
intrinsically too unportable to be elevated to the standard library. i
expect to be outvoted.




https://codereview.appspot.com/6736069/diff/43014/src/pkg/os/executable.go
File src/pkg/os/executable.go (right):

https://codereview.appspot.com/6736069/diff/43014/src/pkg/os/executable.go#newcode7
src/pkg/os/executable.go:7: // Executable returns the absolute pathname
of the current
"the absolute pathname" sounds definitive but is actually not
well-defined.

https://codereview.appspot.com/6736069/diff/43014/src/pkg/os/executable_darwin.go
File src/pkg/os/executable_darwin.go (right):

https://codereview.appspot.com/6736069/diff/43014/src/pkg/os/executable_darwin.go#newcode7
src/pkg/os/executable_darwin.go:7: var progPath string // set by
../runtime/progpath_darwin.c
you've still got some progpaths around, in different casings.

https://codereview.appspot.com/6736069/

On 2012/10/28 17:47:49, r wrote:
https://codereview.appspot.com/6736069/diff/43014/src/pkg/os/executable.go#newcode7
well-defined.
i agree it is difficult to define its precise behavior.
however, i think the following description is reasonable:
the return value of Executable is an absolute path that matches the path
used
to launch the executable (assuming no one is modifying the intermediate
paths).

https://codereview.appspot.com/6736069/diff/43014/src/pkg/os/executable_darwin.go#newcode7
All fixed.

https://codereview.appspot.com/6736069/

PTAL. All supported OSes have working implementations now.
Except perhaps OpenBSD, where procfs is not mounted by
default, but I don't know any alternatives.

+jsing.
Thank you. I originally assumed fd2path on /proc/PID/text would
return "/proc/PID/text".

https://codereview.appspot.com/6736069/

Yeah, that's a reasonable assumption but the man page for
fd2path(2) is careful with its words. There are only a few
places in the kernel where this distinction is made. The
prime example is the dup(3) device: when you open either
of the fd files, a reference to the underlying channel is
returned.

It's similar for /proc/$pid/text.

When you call exec(2) on an executable file, the kernel
opens the file internally and saves a reference to the
corresponding channel (along with some other state) into
something called the "image cache". Then, when you open
/proc/$pid/text, the original channel is retrieved from
the cache and returned.

https://codereview.appspot.com/6736069/diff/37032/src/pkg/os/executable.go
File src/pkg/os/executable.go (right):

https://codereview.appspot.com/6736069/diff/37032/src/pkg/os/executable.go#newcode7
src/pkg/os/executable.go:7: // Executable returns the absolute pathname
of the current
In general this is impossible, of course, so it would be nice to be more
specific about what the return value is good for.

For example, if we wrote:
// Executable returns a path that can be used to reinvoke the current
program.
// It may not be valid after the current program exits.

then on Linux you could just return "/proc/PID/exe" and on Plan 9
"/proc/PID/text".

On the other hand, if there are subtler reasons for wanting this
information, like trying to find associated files, then we'd need to be
more clear that those are not acceptable answers. Personally, I don't
believe we should try to support that use case, so my inclination would
be to use just the description above and not do any kind of readlink or
fd2path or anything like that.

https://codereview.appspot.com/6736069/

Why impossible in general? Assumes nothing changes the environment (that is,
nothing renames files, deletes files, etc.), this CL implemented the
behavior on
all supported OSes.

Names are not unique, well-defined, or stable. It's the kind of thing
that usually works but won't always, and the failures will be subtle.
Code that depends on this interface will be buggy and maybe insecure.

-rob

As an observation, what you have mentioned is true, but is true for
any path related api, such as os,Getwd(), or even os.Open. You don't
actually know for sure that is correct until tested. And granted, some
situations the path is more unstable then others. As an observer, I
continue to not see the argument against it, though I understand it
may not be useful to you. But this being my last comment here, should
this not go in, I'll move what I can (most everything except darwin)
into a third party package (if that's alright with minux to use his
code for some of the platforms I don't have).

-Daniel

No one has answered Russ's question (and mine). What is this _for_?

-rob

This is what I currently use this call for on Windows, Linux, and darwin
right now:

* Get a path to the exec when creating a "service" on windows, upstart,
launchd.
* I often bundle resources next to the exec for easy deployment.
Without this I have to watch my working directory when I call it. Not
horrible,
but much easier just to get the path the OS already knows, no guesswork
required,
same deployment on all systems.

Prior to this
* On Linux I had to be sure to set the WD before starting (with nohup).
* Couldn't get related files without this on Windows at all when running
as a windows service.

Does this answer your question?

Thanks,
-Daniel

Using "Executable" to find related files bothers me quite a bit. I
think Go doesn't have a good story for finding associated files, but
if we want to address that I think we should do it head on instead of
slipping it into a mostly unrelated function.

If this were just for re-running the current executable I wouldn't
mind. But it sounds like that's not why people want it. So my
inclination is to leave it out for now.

OK, fair enough. I think we can do this.
stop saying that the return path can be used to find related files
(it just happen to be able to do this on major platforms, just readlink
the returned path), and just address the re-running the current
executable problem.

I will remove all Readlink calls, and instead just return something
like /proc/self/exe (I will keep the Plan 9 code, because the solution
is not apparent).

what do you think?

If we limit Executable to that smaller scope it sounds like no one
wants it anymore. So I would be inclined to leave it out.

Russ

Darwin, FreeBSD, Windows and Plan 9 all have non-trivial implementation for
os.Executable (e.g. don't return a constant string)

The complexity of the implementation needs to be paid for by general
utility. I haven't heard anyone claiming they need or would use this
function (except to find associated files, which I don't want to
support this way).

Russ

I would use this in things like gorunas, launching child processes of
myself, since we don't have fork.

In tests I always just use os.Args(0) which isn't guaranteed to be portable
or even work always on Linux.

If you change the doc comment to

// Executable returns a path that can be used to reinvoke the current
program.
// It may not be valid after the current program exits.

and simplify the Plan 9 and Linux implementations, then I'll go along with this.
(On Plan 9 use /proc/+itoa(pid)+/text.)

I'd still prefer to leave it out, for the reasons rsc said. I still
believe it's a weak solution to a poorly-defined problem that's not
the one most people actually want solved. and once it goes in we can't
take it out or change its semantics to be something genuinely useful.

That's not a veto, just a statement of preference. One use case does
not a must-last-forever feature make.

-rob

PTAL.
now the problem is well defined: return a absolute path to re-invoke the
same program.
On Unix systems, you simply readlink the returned string, and you will
get what you want. we just don't make that guarantee.


https://codereview.appspot.com/6736069/

Hi, It seems this is hard to get it.

However, I would suggest that returning the /proc/../exe path on procfs
systems is a bad idea. We will simply have to write yet another wrapper to
readlink on some systems, and take the literal value on others.
ie
panic: readlink C:\code\bin\example.exe: not supported by windows

-Daniel

i think the best strategy is:
if readlink returns an error, just use the previously returned result from
Executable.