code review 104650043: os: no /tmp on android

os: no /tmp on android

[crawshaw, 2014-07-09 17:31:34]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[minux, 2014-07-09 17:36:26]

do you expect to have other differences between linux and android
in the os package?

if yes, then i'd suggest we add two files:

//android.go
const isAndroid = true

// nonandroid.go
// +build !android
const isAndroid = false

and use isAndroid instead.

i don't like adding more init()s to core packages like os, and besides
using a constant boolean makes the compiler eliminate unused branches,
so it's practically free in terms of run time.

[crawshaw, 2014-07-09 17:43:47]

as long as we are adding two files, how about this?

On Wed, Jul 9, 2014 at 1:36 PM,  <minux@golang.org> wrote:
> do you expect to have other differences between linux and android
> in the os package?
>
> if yes, then i'd suggest we add two files:
>
> //android.go
> const isAndroid = true
>
> // nonandroid.go
> // +build !android
> const isAndroid = false
>
> and use isAndroid instead.
>
> i don't like adding more init()s to core packages like os, and besides
> using a constant boolean makes the compiler eliminate unused branches,
> so it's practically free in terms of run time.
>
> https://codereview.appspot.com/104650043/

[bradfitz, 2014-07-09 17:49:06]

I'd rather not duplicate a public function, because that means duplicating its
public docs, which is the more offensive part.

I like the isAndroid const idea. The compiler will remove dead code, and it
keeps the code easier to find and together with related stuff.

[minux, 2014-07-09 17:49:29]

On 2014/07/09 17:43:47, crawshaw wrote:
> as long as we are adding two files, how about this?
i still think the isAndroid approach is better, as the tempdir_$GOOS.go
approach duplicates too much.

If this is the only code change needed for android, then LGTM.

[crawshaw, 2014-07-09 17:53:49]

my thinking was that there are already three copies of this function,
in file_{plan9,windows,unix}.go. so introducing isAndroid would add
another mechanism.

i don't have any other changes to package os planned, but future is unclear.

On Wed, Jul 9, 2014 at 1:49 PM,  <minux@golang.org> wrote:
> On 2014/07/09 17:43:47, crawshaw wrote:
>>
>> as long as we are adding two files, how about this?
>
> i still think the isAndroid approach is better, as the tempdir_$GOOS.go
> approach duplicates too much.
>
> If this is the only code change needed for android, then LGTM.
>
> https://codereview.appspot.com/104650043/

[bradfitz, 2014-07-09 17:58:06]

Why not just modify only file_unix.go in this CL and add a:

   if runtime.GOOS == "android" {
      dir = "/data/local/tmp"
   } else {
      dir = "/tmp"
   }

The if will compile away anyway.



On Wed, Jul 9, 2014 at 10:53 AM, David Crawshaw <crawshaw@golang.org> wrote:

> my thinking was that there are already three copies of this function,
> in file_{plan9,windows,unix}.go. so introducing isAndroid would add
> another mechanism.
>
> i don't have any other changes to package os planned, but future is
> unclear.
>
> On Wed, Jul 9, 2014 at 1:49 PM,  <minux@golang.org> wrote:
> > On 2014/07/09 17:43:47, crawshaw wrote:
> >>
> >> as long as we are adding two files, how about this?
> >
> > i still think the isAndroid approach is better, as the tempdir_$GOOS.go
> > approach duplicates too much.
> >
> > If this is the only code change needed for android, then LGTM.
> >
> > https://codereview.appspot.com/104650043/
>

[crawshaw, 2014-07-09 18:02:33]

Nice. Done.

On Wed, Jul 9, 2014 at 1:58 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Why not just modify only file_unix.go in this CL and add a:
>
>    if runtime.GOOS == "android" {
>       dir = "/data/local/tmp"
>    } else {
>       dir = "/tmp"
>    }
>
> The if will compile away anyway.
>
>
>
> On Wed, Jul 9, 2014 at 10:53 AM, David Crawshaw <crawshaw@golang.org> wrote:
>>
>> my thinking was that there are already three copies of this function,
>> in file_{plan9,windows,unix}.go. so introducing isAndroid would add
>> another mechanism.
>>
>> i don't have any other changes to package os planned, but future is
>> unclear.
>>
>> On Wed, Jul 9, 2014 at 1:49 PM,  <minux@golang.org> wrote:
>> > On 2014/07/09 17:43:47, crawshaw wrote:
>> >>
>> >> as long as we are adding two files, how about this?
>> >
>> > i still think the isAndroid approach is better, as the tempdir_$GOOS.go
>> > approach duplicates too much.
>> >
>> > If this is the only code change needed for android, then LGTM.
>> >
>> > https://codereview.appspot.com/104650043/
>
>

[minux, 2014-07-09 18:02:43]

On Wed, Jul 9, 2014 at 1:58 PM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> Why not just modify only file_unix.go in this CL and add a:
>
>    if runtime.GOOS == "android" {
>       dir = "/data/local/tmp"
>    } else {
>       dir = "/tmp"
>    }
>
> The if will compile away anyway.
>
yeah, this is better for tmpdir.

[minux, 2014-07-09 18:04:02]

LGTM.

[bradfitz, 2014-07-09 18:09:08]

LGTM

[crawshaw, 2014-07-09 18:12:37]

*** Submitted as https://code.google.com/p/go/source/detail?r=141e6dead8db ***

os: no /tmp on android

LGTM=minux, bradfitz
R=golang-codereviews, minux, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/104650043

[gobot, 2014-07-09 18:24:02]

This CL appears to have broken the freebsd-amd64-race builder.
See http://build.golang.org/log/5c62dd14f3ad1e3f6a3756684446df7cdae2fc5c

[crawshaw, 2014-07-09 18:24:37]

Nope.

On Wed, Jul 9, 2014 at 2:24 PM,  <gobot@golang.org> wrote:
> This CL appears to have broken the freebsd-amd64-race builder.
> See http://build.golang.org/log/5c62dd14f3ad1e3f6a3756684446df7cdae2fc5c
>
> https://codereview.appspot.com/104650043/