code review 5608059: build: dist-based build for Plan 9

build: dist-based build for Plan 9

[ality, 2012-02-04 03:42:04]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

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

[iant, 2012-02-04 04:17:59]

FYI

http://codereview.appspot.com/5608059/diff/3/src/cmd/dist/a.h
File src/cmd/dist/a.h (right):

http://codereview.appspot.com/5608059/diff/3/src/cmd/dist/a.h#newcode13
src/cmd/dist/a.h:13: #define USED(x) if(x){}else{}
A better way to cleanly swallow exactly one semicolon would be to write
#define USED(x) do{x;}while(0)

http://codereview.appspot.com/5608059/diff/3/src/cmd/dist/goc2c.c
File src/cmd/dist/goc2c.c (right):

http://codereview.appspot.com/5608059/diff/3/src/cmd/dist/goc2c.c#newcode574
src/cmd/dist/goc2c.c:574: goto out;
What is the point of this change?

[rsc, 2012-02-04 04:41:12]

I don't think you need pcc for this anymore.
You should be able to write a single plan9.c
that is separate from unix.c (does not depend
on linking unix.c in too), and plan9.c just has
to include stdio.h.

I think that will be easier than trying to balance
plan9 customizations in unix.c.  The point
of having the single os-specific file is to have
a single os-specific file.

http://codereview.appspot.com/5608059/diff/3/src/cmd/dist/a.h
File src/cmd/dist/a.h (right):

http://codereview.appspot.com/5608059/diff/3/src/cmd/dist/a.h#newcode13
src/cmd/dist/a.h:13: #define USED(x) if(x){}else{}
Delete.  The compiler knows what USED means.

[ality, 2012-02-04 06:38:22]

PTAL.

I've changed to use native Plan 9 libraries instead of APE.

Thanks for the review.

https://codereview.appspot.com/5608059/diff/3/src/cmd/dist/a.h
File src/cmd/dist/a.h (right):

https://codereview.appspot.com/5608059/diff/3/src/cmd/dist/a.h#newcode13
src/cmd/dist/a.h:13: #define USED(x) if(x){}else{}
On 2012/02/04 04:41:12, rsc wrote:
> Delete.  The compiler knows what USED means.

I had to do this a bit differently now that I'm not using
APE.  Please review.

https://codereview.appspot.com/5608059/diff/3/src/cmd/dist/goc2c.c
File src/cmd/dist/goc2c.c (right):

https://codereview.appspot.com/5608059/diff/3/src/cmd/dist/goc2c.c#newcode574
src/cmd/dist/goc2c.c:574: goto out;
On 2012/02/04 04:18:00, iant wrote:
> What is the point of this change?

It silences a warning from the Plan 9 compiler,
"empty if body".

[rsc, 2012-02-04 06:52:56]

I didn't review the scripts yet; I am focusing on the C code.

If you find yourself making any significant changes 
(beyond introducing USED annotations or changing ; to {})
to a file other than plan9.c, then please rethink.
The only file that you should need to write code in
is plan9.c.

Thanks.

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/a.h
File src/cmd/dist/a.h (right):

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/a.h#newcode11
src/cmd/dist/a.h:11: #ifdef PLAN9
Sorry, but a.h must not include any other headers.
That's how we make sure the non-os files are not 
accidentally using non-portable functionality.

The only change this file should need is
#ifndef PLAN9 / #endif around the #define USED.

In plan9.c, you should be able to do:

#include <u.h>
#include <libc.h>
#undef nil
#undef nelem
#include "a.h"

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/arg.h
File src/cmd/dist/arg.h (right):

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/arg.h#newcode29
src/cmd/dist/arg.h:29: #ifndef PLAN9
Drop.

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/goc2c.c
File src/cmd/dist/goc2c.c (right):

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/goc2c.c#newcode574
src/cmd/dist/goc2c.c:574: goto out;
Please introduce braces instead of a goto.

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/plan9.c
File src/cmd/dist/plan9.c (right):

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/plan9.c#newcode719
src/cmd/dist/plan9.c:719: #endif // __WINDOWS__
PLAN9

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/unix.c
File src/cmd/dist/unix.c (right):

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/unix.c#newcode273
src/cmd/dist/unix.c:273: if(getcwd(buf, sizeof buf) == nil)
Revert all changes to this file.

[ality, 2012-02-04 07:23:13]

PTAL

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/a.h
File src/cmd/dist/a.h (right):

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/a.h#newcode11
src/cmd/dist/a.h:11: #ifdef PLAN9
On 2012/02/04 06:52:56, rsc wrote:
> Sorry, but a.h must not include any other headers.
> That's how we make sure the non-os files are not 
> accidentally using non-portable functionality.
> 
> The only change this file should need is
> #ifndef PLAN9 / #endif around the #define USED.
> 
> In plan9.c, you should be able to do:
> 
> #include <u.h>
> #include <libc.h>
> #undef nil
> #undef nelem
> #include "a.h"

Done.

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/arg.h
File src/cmd/dist/arg.h (right):

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/arg.h#newcode29
src/cmd/dist/arg.h:29: #ifndef PLAN9
On 2012/02/04 06:52:56, rsc wrote:
> Drop.

Done and fixed some warnings.

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/goc2c.c
File src/cmd/dist/goc2c.c (right):

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/goc2c.c#newcode574
src/cmd/dist/goc2c.c:574: goto out;
On 2012/02/04 06:52:56, rsc wrote:
> Please introduce braces instead of a goto.

Done.

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/plan9.c
File src/cmd/dist/plan9.c (right):

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/plan9.c#newcode719
src/cmd/dist/plan9.c:719: #endif // __WINDOWS__
On 2012/02/04 06:52:56, rsc wrote:
> PLAN9

Done.

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/unix.c
File src/cmd/dist/unix.c (right):

https://codereview.appspot.com/5608059/diff/3019/src/cmd/dist/unix.c#newcode273
src/cmd/dist/unix.c:273: if(getcwd(buf, sizeof buf) == nil)
On 2012/02/04 06:52:56, rsc wrote:
> Revert all changes to this file.

Mostly done.  Could we keep the changes down below
in main?  Using #if and #elif confuses the Plan 9
compiler since it sees the #endif and thinks it belongs to
the #ifndef PLAN9 at the top.

I figured we're already calling uname to get the
architecture.

[rsc, 2012-02-04 07:39:37]

Getting there.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/goc2c.c
File src/cmd/dist/goc2c.c (right):

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/goc2c.c#newcode573
src/cmd/dist/goc2c.c:573: if (rets != nil) {
Please revert this rewrite and then edit just the two lines:

if (rets == nil)
    ;

to:

if (rets == nil) {
    // nothing to do
}

No other lines need to change.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c
File src/cmd/dist/plan9.c (right):

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode28
src/cmd/dist/plan9.c:28: vsnprint(buf, sizeof buf, fmt, arg);
This isn't okay, because Plan 9's formats and stdio's are different.
Please #include <stdio.h> and then use printf throughout here.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode155
src/cmd/dist/plan9.c:155: binit(&b1);
Please put all the binit in one block here and 
all the bfree in one block at the bottom of the function; see README.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode160
src/cmd/dist/plan9.c:160: argv0 = btake(&b1);
I don't understand why this code edits argv0 and then never uses it.
I suspect this should be

free(argv->p[0]);
argv->p[0] = xstrdup(bstr(&b1));

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode285
src/cmd/dist/plan9.c:285: fd = open(".", 0);
I can't follow what's going on here.  I think this should be

fd = open(path, OREAD);
if(fd2path(fd, buf, sizeof buf) < 0)
     fatal("fdpath: %r");
close(fd);
breset(b);
bwritestr(b, buf);

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode340
src/cmd/dist/plan9.c:340: return (Time)t*1000000000LL;
(Time)t is fine.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode385
src/cmd/dist/plan9.c:385: fd = open(p, OREAD);
Seems like a lot of work.  Why not do

if(isdir(p))
    return;
fd = create(p, OREAD, 0777|DMDIR);
close(fd);
if(fd < 0)
    fatal("mkdir %s: %r", p);

(and that's it)

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode488
src/cmd/dist/plan9.c:488: fd = create(bstr(&b), OREAD, 0700|DMDIR);
OREAD|OEXCL

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode662
src/cmd/dist/plan9.c:662: int
Plan 9 main should be void.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode672
src/cmd/dist/plan9.c:672: if(hassuffix(p, "bin/tool/dist")) {
Delete.
default_goroot is now handled elsewhere.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode686
src/cmd/dist/plan9.c:686: rfork(RFENVG);
If this is important (and it probably is), it should be the first thing main
does.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode692
src/cmd/dist/plan9.c:692: return 0;
exits(nil);

https://codereview.appspot.com/5608059/diff/5022/src/run.rc
File src/run.rc (right):

https://codereview.appspot.com/5608059/diff/5022/src/run.rc#newcode32
src/run.rc:32: fn xcd {
Please delete here through 106.  As these get converted
to be bash free we can add them back.

[ality, 2012-02-04 08:43:35]

PTAL

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/goc2c.c
File src/cmd/dist/goc2c.c (right):

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/goc2c.c#newcode573
src/cmd/dist/goc2c.c:573: if (rets != nil) {
On 2012/02/04 07:39:37, rsc wrote:
> Please revert this rewrite and then edit just the two lines:
> 
> if (rets == nil)
>     ;
> 
> to:
> 
> if (rets == nil) {
>     // nothing to do
> }
> 
> No other lines need to change.

Done.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c
File src/cmd/dist/plan9.c (right):

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode28
src/cmd/dist/plan9.c:28: vsnprint(buf, sizeof buf, fmt, arg);
On 2012/02/04 07:39:37, rsc wrote:
> This isn't okay, because Plan 9's formats and stdio's are different.
> Please #include <stdio.h> and then use printf throughout here.

Done. I changed fatal to call errstr since we can't use
the "r" format in stdio print functions. The only place
I kept the Plan 9 style print is in genrun right before
the exec.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode155
src/cmd/dist/plan9.c:155: binit(&b1);
On 2012/02/04 07:39:37, rsc wrote:
> Please put all the binit in one block here and 
> all the bfree in one block at the bottom of the function; see README.

Done.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode160
src/cmd/dist/plan9.c:160: argv0 = btake(&b1);
On 2012/02/04 07:39:37, rsc wrote:
> I don't understand why this code edits argv0 and then never uses it.
> I suspect this should be
> 
> free(argv->p[0]);
> argv->p[0] = xstrdup(bstr(&b1));

Done. This was some stale code from a previous edit.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode285
src/cmd/dist/plan9.c:285: fd = open(".", 0);
On 2012/02/04 07:39:37, rsc wrote:
> I can't follow what's going on here.  I think this should be
> 
> fd = open(path, OREAD);
> if(fd2path(fd, buf, sizeof buf) < 0)
>      fatal("fdpath: %r");
> close(fd);
> breset(b);
> bwritestr(b, buf);

Done.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode340
src/cmd/dist/plan9.c:340: return (Time)t*1000000000LL;
On 2012/02/04 07:39:37, rsc wrote:
> (Time)t is fine.

Done.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode385
src/cmd/dist/plan9.c:385: fd = open(p, OREAD);
On 2012/02/04 07:39:37, rsc wrote:
> Seems like a lot of work.  Why not do
> 
> if(isdir(p))
>     return;
> fd = create(p, OREAD, 0777|DMDIR);
> close(fd);
> if(fd < 0)
>     fatal("mkdir %s: %r", p);
> 
> (and that's it)

Done.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode488
src/cmd/dist/plan9.c:488: fd = create(bstr(&b), OREAD, 0700|DMDIR);
On 2012/02/04 07:39:37, rsc wrote:
> OREAD|OEXCL

Done.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode662
src/cmd/dist/plan9.c:662: int
On 2012/02/04 07:39:37, rsc wrote:
> Plan 9 main should be void.

Done.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode672
src/cmd/dist/plan9.c:672: if(hassuffix(p, "bin/tool/dist")) {
On 2012/02/04 07:39:37, rsc wrote:
> Delete.
> default_goroot is now handled elsewhere.

Done.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode686
src/cmd/dist/plan9.c:686: rfork(RFENVG);
On 2012/02/04 07:39:37, rsc wrote:
> If this is important (and it probably is), it should be the first thing main
> does.

Done.

https://codereview.appspot.com/5608059/diff/5022/src/cmd/dist/plan9.c#newcode692
src/cmd/dist/plan9.c:692: return 0;
On 2012/02/04 07:39:37, rsc wrote:
> exits(nil);

Done.

https://codereview.appspot.com/5608059/diff/5022/src/run.rc
File src/run.rc (right):

https://codereview.appspot.com/5608059/diff/5022/src/run.rc#newcode32
src/run.rc:32: fn xcd {
On 2012/02/04 07:39:37, rsc wrote:
> Please delete here through 106.  As these get converted
> to be bash free we can add them back.

Done.

[iant2, 2012-02-04 19:29:57]

On Feb 3, 2012 10:38 PM, <ality@pbrane.org> wrote:
>
> It silences a warning from the Plan 9 compiler,
> "empty if body".

I know you are working with an existing compiler, but that is a silly
warning.  There is nothing wrong with an empty if body if the next token is
"else".

Ian

[ality, 2012-02-05 04:44:41]

Hello rsc@golang.org, iant@golang.org, iant@google.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[akumar, 2012-02-05 11:05:36]

Beyond that, we will get "mixed ansi/old function declaration" errors upon
compiling buildgc.c, if we don't comment out the `#include <stdio.h>' in that
file.

http://codereview.appspot.com/5608059/diff/4015/src/cmd/dist/plan9.c
File src/cmd/dist/plan9.c (right):

http://codereview.appspot.com/5608059/diff/4015/src/cmd/dist/plan9.c#newcode18
src/cmd/dist/plan9.c:18: 
need to delcare `char *hostchar;'

[rsc, 2012-02-06 17:47:54]

I'd like to postpone these Plan 9 CLs until after Go 1.

Thanks.
Russ

[ality, 2012-04-12 13:31:39]

Hello rsc@golang.org, iant@golang.org, iant@google.com,
seed@mail.nanosouffle.net (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-04-25 02:40:34]

looks good but for the include question

http://codereview.appspot.com/5608059/diff/24002/src/cmd/dist/build.c
File src/cmd/dist/build.c (right):

http://codereview.appspot.com/5608059/diff/24002/src/cmd/dist/build.c#newcode864
src/cmd/dist/build.c:864: vadd(&compile, bpathf(&b, "-I%s/include/plan9",
goroot));
What is this about?  We don't have any such directory under goroot.
Same question about next line.

[ality, 2012-04-26 10:13:01]

https://codereview.appspot.com/5608059/diff/24002/src/cmd/dist/build.c
File src/cmd/dist/build.c (right):

https://codereview.appspot.com/5608059/diff/24002/src/cmd/dist/build.c#newcod...
src/cmd/dist/build.c:864: vadd(&compile, bpathf(&b, "-I%s/include/plan9",
goroot));
On 2012/04/25 02:40:35, rsc wrote:
> What is this about?  We don't have any such directory
> under goroot. Same question about next line.

We do. I added it in January:
https://codereview.appspot.com/5552056

You gave it a LGTM at the time.

[rsc, 2012-05-02 01:51:21]

LGTM

[ality, 2012-05-02 05:32:58]

*** Submitted as http://code.google.com/p/go/source/detail?r=92c70c9b9d27 ***

build: dist-based build for Plan 9

R=rsc, iant, iant, seed
CC=golang-dev
http://codereview.appspot.com/5608059