code review 5620045: cmd/dist: new command

cmd/dist: new command

dist is short for distribution.  This is the new Go distribution tool.

The plan is to replace the Makefiles with what amounts to
'go tool dist bootstrap', although it cannot be invoked like
that since it is in charge of getting us to the point where we
can build the go command.

It will also add additional commands to replace bash scripts
like test/run (go tool dist testrun), eventually eliminating our
dependence on not just bash but all the Unix tools and all
of cygwin.

This is strong enough to build (cc *.c) and run (a.out bootstrap)
to build not just the C libraries and tools but also the basic
Go packages up to the bootstrap form of the go command
(go_bootstrap).  I've run it successfully on both Linux and Windows.
This means that once we've switched to this tool in the build,
we can delete the buildscripts.

This tool is not nearly as nice as the go tool.  There are many
special cases that turn into simple if statements or tables in
the code.  Please forgive that.  C does not enjoy the benefits
that we designed into Go.

I was planning to wait to do this until after Go 1, but the
Windows builders are both broken due to a bug in either
make or bash or both involving the parsing of quoted command
arguments.  Make thinks it is invoking

        quietgcc -fno-common -I"c:/go/include" -ggdb -O2 -c foo.c

but bash (quietgcc is a bash script) thinks it is being invoked as

        quietgcc -fno-common '-Ic:/go/include -ggdb' -O2 -c foo.c

which obviously does not have the desired effect.  Rather than fight
these clumsy ports, I accelerated the schedule for the new tool.
We should be completely off cygwin (using just the mingw gcc port,
which is much more standalone) before Go 1.

It is big for a single CL, and for that I apologize.  I can cut it into
separate CLs along file boundaries if people would prefer that.

[rsc, 2012-02-01 23:25:07]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[adg, 2012-02-02 00:02:37]

In the CL description: s/^to build //

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c
File src/cmd/dist/buf.c (right):

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode60
src/cmd/dist/buf.c:60: // bwritestr appendst he string p to the buffer.
s/t he/ the/

[gri, 2012-02-02 00:35:56]

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README
File src/cmd/dist/README (right):

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README#newcode1
src/cmd/dist/README:1: This program, dist, is the bootstrapping tool for the Go
distribution.
why not s/dist/bootstrap/ ? it's not really distributing anything, it seems.

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README#newcode4
src/cmd/dist/README:4: to replace odd job previously done with shell scripts.
s/job/jobs/

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

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/a.h#newcode7
src/cmd/dist/a.h:7: // The unit on Time is unspecified; we just need to
s/on/of/ or s/on/for/ ?

[gri, 2012-02-02 00:38:39]

PS: C is the lowest common denominator, I guess. I am all for this
tool, but I wonder if it can be simpler. For one, perhaps the memory
management can be simplified (albeit there's not much) - just
allocate, never free. Always copy, never share. This tool is not
running long.
- gri

On Wed, Feb 1, 2012 at 4:35 PM,  <gri@golang.org> wrote:
>
> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README
> File src/cmd/dist/README (right):
>
> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README#newcode1
> src/cmd/dist/README:1: This program, dist, is the bootstrapping tool for
> the Go distribution.
> why not s/dist/bootstrap/ ? it's not really distributing anything, it
> seems.
>
> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README#newcode4
> src/cmd/dist/README:4: to replace odd job previously done with shell
> scripts.
> s/job/jobs/
>
> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/a.h
> File src/cmd/dist/a.h (right):
>
> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/a.h#newcode7
> src/cmd/dist/a.h:7: // The unit on Time is unspecified; we just need to
> s/on/of/ or s/on/for/ ?
>
> http://codereview.appspot.com/5620045/

[bradfitz, 2012-02-02 00:53:10]

Exciting.

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README
File src/cmd/dist/README (right):

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README#newcode4
src/cmd/dist/README:4: to replace odd job previously done with shell scripts.
s/job/jobs/

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README#newcode16
src/cmd/dist/README:16: data structures, Buf and Vec, which own all the data
they point at.
from this doc alone, it's not clear whether a Vec owns the member Buf or not. 
in the code sample below, you show bfree in addition to vfree, but never adding
to the vector, so it's not clear either way.

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

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode794
src/cmd/dist/build.c:794: // fixslash rewrites / to \ on Windows, so that the
paths look conventional.
why is this here and not in some portability file?

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c
File src/cmd/dist/unix.c (right):

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c#newcode35
src/cmd/dist/unix.c:35: if(n <= 0)
be fatal on non-EOF errors?

[brainman, 2012-02-02 01:06:23]

You always find ways! Thank you, Russ.

I am just reporting problems. Sorry.

1) building on linux/386 fails

# ./dist bootstrap
hostos linux
uname -m
ld --version
lib9
gcc -Wall -Wno-sign-compare -Wno-missing-braces -Wno-parentheses
-Wno-unknown-pragmas -Wno-switch -Wno-comment -Werror -fno-common -ggdb -O2 -c
-m64 -DPLAN9PORT -I /root/hg/go/include -I /root/hg/go/src/lib9 -o
$WORK/_exits.o /root/hg/go/src/lib9/_exits.c
/root/hg/go/src/lib9/_exits.c:1: sorry, unimplemented: 64-bit mode not compiled
in
go tool dist: gcc failed
rm -rf /var/tmp/go-cbuild-c3PAqT

2) windows needs GOHOSTARCH set. Correct?

3) windows/386 builds fail on lib9:

C:\MinGW\go\src\cmd\dist>dist.exe bootstrap
ld --version
lib9
gcc -Wall -Wno-sign-compare -Wno-missing-braces -Wno-parentheses
-Wno-unknown-pragmas -Wno-switch -Wno-comment -Werror -fno-common -ggdb -O2 -c
-DPLAN9PORT -I c:\mingw\go\include -I c:\mingw\go\src\lib9 -o $WORK\_exits.o
c:\mingw\go\src\lib9\_exits.c
gcc -Wall -Wno-sign-compare -Wno-missing-braces -Wno-parentheses
-Wno-unknown-pragmas -Wno-switch -Wno-comment -Werror -fno-common -ggdb -O2 -c
-DPLAN9PORT -I c:\mingw\go\include -I c:\mingw\go\src\lib9 -o $WORK\_p9dir.o
c:\mingw\go\src\lib9\_p9dir.c
gcc -Wall -Wno-sign-compare -Wno-missing-braces -Wno-parentheses
-Wno-unknown-pragmas -Wno-switch -Wno-comment -Werror -fno-common -ggdb -O2 -c
-DPLAN9PORT -I c:\mingw\go\include -I c:\mingw\go\src\lib9 -o $WORK\argv0.o
c:\mingw\go\src\lib9\argv0.c
gcc -Wall -Wno-sign-compare -Wno-missing-braces -Wno-parentheses
-Wno-unknown-pragmas -Wno-switch -Wno-comment -Werror -fno-common -ggdb -O2 -c
-DPLAN9PORT -I c:\mingw\go\include -I c:\mingw\go\src\lib9 -o $WORK\atoi.o
c:\mingw\go\src\lib9\atoi.c
gcc -Wall -Wno-sign-compare -Wno-missing-braces -Wno-parentheses
-Wno-unknown-pragmas -Wno-switch -Wno-comment -Werror -fno-common -ggdb -O2 -c
-DPLAN9PORT -I c:\mingw\go\include -I c:\mingw\go\src\lib9 -o $WORK\await.o
c:\mingw\go\src\lib9\await.c
c:\mingw\go\src\lib9\await.c:33:22: fatal error: sys/wait.h: No such file or
directory
compilation terminated.
cbuild: gcc failed
rm -rf C:\DOCUME~1\brainman\LOCALS~1\Temp\go3D4.tmp

I take it because lib9 have different source file set on windows. See
lib9/Makefile

Alex

[brainman, 2012-02-02 01:17:51]

Why don't we write this program in Go? We could keep compiled binary in the repo
for every platform we support.

Alex

[dsymonds, 2012-02-02 01:34:42]

On Thu, Feb 2, 2012 at 12:17 PM,  <alex.brainman@gmail.com> wrote:

> Why don't we write this program in Go? We could keep compiled binary in
> the repo for every platform we support.

Because then you have a bootstrapping problem for new targets.

[brainman, 2012-02-02 01:38:27]

On 2012/02/02 01:34:42, dsymonds wrote:
> 
> Because then you have a bootstrapping problem for new targets.

We are cross-compiling. Aren't we? If we can't build program like that for "new
target", then, I would say, we have nothing to "bootstrap".

Alex

[iant, 2012-02-02 01:53:26]

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README
File src/cmd/dist/README (right):

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README#newcode1
src/cmd/dist/README:1: This program, dist, is the bootstrapping tool for the Go
distribution.
Does a README file require a copyright notice?

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README#newcode41
src/cmd/dist/README:41: registers for []byte or []string, sitting in charge of
the allocation for that
I don't understand what you mean by "registers" here.  Unless you are trying to
say something subtle, perhaps this whole sentence can go.

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

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/a.h#newcode7
src/cmd/dist/a.h:7: // The unit on Time is unspecified; we just need to
"Time values are unspecified; we just need to be able...."

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c
File src/cmd/dist/buf.c (right):

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode9
src/cmd/dist/buf.c:9: #include <stdio.h>
Why do you need <stdio.h>?

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode97
src/cmd/dist/buf.c:97: // bprintf replaces the buffer with the result of the
printf formatting.
...and returns a pointer to the NUL-terminated buffer contents.

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode106
src/cmd/dist/buf.c:106: vsnprintf(buf, sizeof buf, fmt, arg);
Not xvsnprintf?

(BTW gcc uses vasprintf to avoid worrying about fixed size buffers.)

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode230
src/cmd/dist/buf.c:230: p[i+1] = '\0';
Doing it this way makes it more likely that someday somebody will get bitten by
passing in a constant string.  You can avoid that and be slightly more efficient
to boot by using vaddbuf(Vec *, char *, int).

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode258
src/cmd/dist/buf.c:258: *p = '\0';
Same comment about constant strings.

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

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode7
src/cmd/dist/build.c:7: #include <stdio.h>
Why do you need <stdio.h>?  It seems to run against the idea expressed in the
README, that everything system-specific was in a portability file.

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode540
src/cmd/dist/build.c:540: stale = 1;
stale is never set to anything other than 1.  Is this a TODO?

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode585
src/cmd/dist/build.c:585: }
Aren't we in trouble if we get here without finding the name in gentab?

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode598
src/cmd/dist/build.c:598: vadd(&compile, "-m64");
Should you use -m32 if gohostarch == i386?

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode642
src/cmd/dist/build.c:642: b.p[b.len-1] = 'o';  // was c
was c or s

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c
File src/cmd/dist/unix.c (right):

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c#newcode91
src/cmd/dist/unix.c:91: bwritestr(&cmd, "$WORK");
What is this about?

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c#newcode519
src/cmd/dist/unix.c:519: xprintf("hostos %s\n", gohostos);
Remove debug print.

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/windows.c
File src/cmd/dist/windows.c (right):

http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/windows.c#newcod...
src/cmd/dist/windows.c:169: break;
error reporting?

[brainman, 2012-02-02 01:54:44]

As far as windows concerned. Perhaps builders mingw are out of date and can be
upgraded. When I run ./all.bash or gobuilder here I see no problem I see on
windows builders.

Alex

[ality, 2012-02-02 13:07:43]

I can't express how great this is. I've written the Plan 9
support already and can either send it to you now or just
send a CL after this goes in.

A few things need to be changed to make it work, though.
Comments are below.

Thanks a bunch for doing this.

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

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/a.h#newcode4
src/cmd/dist/a.h:4: 
THis will make it easier to add Plan 9 support:

#ifdef PLAN9
#include <u.h>
#include <libc.h>
#else
#include <stdarg.h>
#include <stdio.h>
#define nil ((void*)0)
#define nelem(x) (sizeof(x)/sizeof((x)[0]))
#define USED(x) ((void)(x))
#endif

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/a.h#newcode11
src/cmd/dist/a.h:11: #define nil ((void*)0)
Remove these (see previous comment).

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/a.h#newcode64
src/cmd/dist/a.h:64: extern char *gobin;
I've moved the install function out of build.c
and into a new file called install.c. I also wrote
install-plan9.c since there's a bunch of changes
that would make the code much harder to read.

For this to work, a few more things need to have
external linkage.

Add gochar and goversion here.

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/a.h#newcode76
src/cmd/dist/a.h:76: 
Add these here:
void    fixslash(Buf*);
bool    shouldbuild(char*, char*);
void    copy(char*, char*);

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/a.h#newcode80
src/cmd/dist/a.h:80: 
Add:

// install.c
void    install(char*);

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/a.h#newcode111
src/cmd/dist/a.h:111: void	xprintf(char*, ...);
We need to add a declaration for xvsnprintf here
and then write the wrappers for each operating system.
This is necessary to get around the fact that Plan 9
uses vsnprint where Unix uses vsnprint*f*.

void    xvsnprintf(char*, int, char*, va_list);

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c
File src/cmd/dist/buf.c (right):

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode8
src/cmd/dist/buf.c:8: #include <stdarg.h>
Delete.

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode9
src/cmd/dist/buf.c:9: #include <stdio.h>
Delete.

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode106
src/cmd/dist/buf.c:106: vsnprintf(buf, sizeof buf, fmt, arg);
On 2012/02/02 01:53:26, iant wrote:
> Not xvsnprintf?

We'll need xvsnprintf for Plan 9.

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

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode7
src/cmd/dist/build.c:7: #include <stdio.h>
Delete.

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode27
src/cmd/dist/build.c:27: static void fixslash(Buf*);
Remove these three declarations.

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode114
src/cmd/dist/build.c:114: if(b.len == 0 && !streq(gohostos, "windows")) {
if(b.len == 0 && streq(gohostos, "plan9")) {
    xgetenv(&b, "objtype");
    if(b.len == 0)
        fatal("neither $GOHOSTARCH nor $objtype is set");
}
else if(b.len == 0 && !streq(gohostos, "windows")) {

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode172
src/cmd/dist/build.c:172: "5a", "5c", "5g", "5l",
If we rearrange this list to move the Go compilers
after the regular Plan 9 toolchain then we can easily
be more careful when deleting the old binaries.

Remove ?g from these lines.

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode175
src/cmd/dist/build.c:175: "6cov",
Add a line with "5g", "6g", "8g" here.

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode202
src/cmd/dist/build.c:202: run(&b, nil, 0, "ld", "--version", nil);
Enclose this block in if(!streq(gohostos, "plan9")).

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode239
src/cmd/dist/build.c:239: for(i=0; i<nelem(oldtool); i++)
if(streq(gohostos, "plan9")) {
    i = 9; // don't remove the host compilers
} else {
    i = 0;
}
for(; i<nelem(oldtool); i++)

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode248
src/cmd/dist/build.c:248: /*
Move the code starting here and ending just before
matchfield into a new file, "install.c".

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode726
src/cmd/dist/build.c:726: static bool
s/static //

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode795
src/cmd/dist/build.c:795: static void
s/static //

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode808
src/cmd/dist/build.c:808: static void
s/static //

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode908
src/cmd/dist/build.c:908: 
USED(argc);
USED(argv);

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buildgc.c
File src/cmd/dist/buildgc.c (right):

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buildgc.c#newcode6
src/cmd/dist/buildgc.c:6: #include <stdio.h>
Delete.

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

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c#newcode516
src/cmd/dist/unix.c:516: #else
This will fail on Plan 9 since the #else
corresponds to the second #ifndef at the
top of this file.

[ality, 2012-02-02 13:16:44]

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

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode378
src/cmd/dist/build.c:378: ".go",
What about ".y"? How could this have worked
without running yacc or bison? Did you have
stale y.tab.[ch] files hanging around?

[rsc, 2012-02-02 15:15:20]

On Thu, Feb 2, 2012 at 08:16,  <ality@pbrane.org> wrote:
> What about ".y"? How could this have worked
> without running yacc or bison? Did you have
> stale y.tab.[ch] files hanging around?

Generated files that are complex to build will be
checked into the repository instead, like we do
for the z files in package syscall.

Russ

[rsc, 2012-02-02 15:17:25]

Plan 9 is beyond the scope of this particular review.
However, I would encourage you to compile this tool
with pcc on Plan 9.

Russ

[rsc, 2012-02-02 15:22:12]

On Wed, Feb 1, 2012 at 19:38, Robert Griesemer <gri@golang.org> wrote:
> PS: C is the lowest common denominator, I guess. I am all for this
> tool, but I wonder if it can be simpler. For one, perhaps the memory
> management can be simplified (albeit there's not much) - just
> allocate, never free. Always copy, never share. This tool is not
> running long.

I was going to do the never free thing, but there is a lot of data
moving around, and I would still need the concepts of Buf and Vec
for things like reading data from a file or the result of splitlines.
I don't think the memory management would get much simpler.
You'd be able to delete the various bfree/vfree lines, but that's
about it.

Russ

[rsc, 2012-02-02 15:24:55]

On Wed, Feb 1, 2012 at 20:06,  <alex.brainman@gmail.com> wrote:
> 1) building on linux/386 fails
>
> 3) windows/386 builds fail on lib9:

Yes, this tool requires other changes to the repository
that have not yet been committed.

> 2) windows needs GOHOSTARCH set. Correct?

Yes, at the moment.  If you can tell me how to figure out
whether the underlying operating system is a 64-bit or a
32-bit Windows, I'd love to add that.

Thanks.
Russ

[rsc, 2012-02-02 15:26:00]

On Wed, Feb 1, 2012 at 20:17,  <alex.brainman@gmail.com> wrote:
> Why don't we write this program in Go? We could keep compiled binary in
> the repo for every platform we support.

It's ultimately not that big a program, and it means that we can build
Go from scratch, without relying on the correctness of opaque binaries.
It's an important property, and one we'd like to preserve.

Russ

[rsc, 2012-02-02 15:32:11]

On Wed, Feb 1, 2012 at 20:54,  <alex.brainman@gmail.com> wrote:
> As far as windows concerned. Perhaps builders mingw are out of date and
> can be upgraded. When I run ./all.bash or gobuilder here I see no
> problem I see on windows builders.

I think you mean cygwin, since mingw is just the gcc port.
One of the motivations for this is to make it possible to
build Go without cygwin, which does not play well with the
surrounding environment and is quite compllicated to set up.
In the long term, cygwin cannot be a dependency for building
Go from source.  It has served well to get us this far, but it
needs to be shown the door.

My goal is for this dist program to build just as well under
the standard Microsoft command-line compiler (cl.exe),
as a step toward building the whole Go-on-Windows without
even needing mingw, although mingw is significantly less
problematic than cygwin.

Russ

[frichter, 2012-02-02 16:47:43]

http://msdn.microsoft.com/en-us/library/windows/desktop/ms684139(v=vs.85).aspx
 

use GetProcAddress to check for the existence of IsWow64Process which is 
only present in Windows 64

[frichter, 2012-02-02 17:18:24]

Duh,  "Note that this technique is not a reliable way to detect whether the 
operating system is a 64-bit version of Windows because the Kernel32.dll in 
current versions of 32-bit Windows also contains this function."

GetNativeSystemInfo() and checking the wProcessorArchitecture member of the 
returned SYSTEM_INFO struct looks more promising.  GetNativeSystemInfo() is 
supported on XP or later

http://msdn.microsoft.com/en-us/library/ms724340(v=vs.85).aspx

[ality, 2012-02-02 17:43:21]

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

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c#newcode151
src/cmd/dist/unix.c:151: char buf[MAXPATHLEN];
Should this be PATH_MAX? I never know which to use.

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c#newcode154
src/cmd/dist/unix.c:154: if(getcwd(buf, MAXPATHLEN) == nil)
s/MAXPATHLEN/sizeof buf/

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c#newcode162
src/cmd/dist/unix.c:162: xrealwd(Buf *b, char *path)
This appears to be unused.

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c#newcode495
src/cmd/dist/unix.c:495: setenv(name, value, 1);
I'm not sure if setenv is available everywhere.
You could do:

Buf b;

binit(&b);
bprintf(&b, "%s=%s", name, value);
putenv(btake(&b));

https://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c#newcode553
src/cmd/dist/unix.c:553: ep = p+strlen(p);
Duplicated initialization of ep.

[rsc, 2012-02-02 21:06:01]

PTAL

On Wed, Feb 1, 2012 at 20:53,  <iant@golang.org> wrote:
> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README#newcode1
> src/cmd/dist/README:1: This program, dist, is the bootstrapping tool for
> the Go distribution.
> Does a README file require a copyright notice?

I don't believe so.

> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/README#newcode41
> src/cmd/dist/README:41: registers for []byte or []string, sitting in
> charge of the allocation for that
> I don't understand what you mean by "registers" here.  Unless you are
> trying to say something subtle, perhaps this whole sentence can go.

I replaced it with something maybe more understandable.

> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/a.h#newcode7
> src/cmd/dist/a.h:7: // The unit on Time is unspecified; we just need to
> "Time values are unspecified; we just need to be able...."

Done.

> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode9
> src/cmd/dist/buf.c:9: #include <stdio.h>
> Why do you need <stdio.h>?

Gone.

> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode97
> src/cmd/dist/buf.c:97: // bprintf replaces the buffer with the result of
> the printf formatting.
> ...and returns a pointer to the NUL-terminated buffer contents.

Done.  I moved this into the unix.c/windows.c, so that it can
use the appropriate <stdio.h> or whatever.

> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode106
> src/cmd/dist/buf.c:106: vsnprintf(buf, sizeof buf, fmt, arg);
> Not xvsnprintf?
>
> (BTW gcc uses vasprintf to avoid worrying about fixed size buffers.)

Moved into unix.c/windows.c so that each system can do what
is appropriate.

> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode230
> src/cmd/dist/buf.c:230: p[i+1] = '\0';
> Doing it this way makes it more likely that someday somebody will get
> bitten by passing in a constant string.  You can avoid that and be
> slightly more efficient to boot by using vaddbuf(Vec *, char *, int).

Done, nice suggestion.

> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/buf.c#newcode258
> src/cmd/dist/buf.c:258: *p = '\0';
> Same comment about constant strings.

Done.

> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode7
> src/cmd/dist/build.c:7: #include <stdio.h>
> Why do you need <stdio.h>?  It seems to run against the idea expressed
> in the README, that everything system-specific was in a portability
> file.

Gone.  It was for bprintf.

>
http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode540
> src/cmd/dist/build.c:540: stale = 1;
> stale is never set to anything other than 1.  Is this a TODO?

Yes, added explicit comment.

>
http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode585
> src/cmd/dist/build.c:585: }
> Aren't we in trouble if we get here without finding the name in gentab?

Yes.  I was letting the build fail, but will fail here with
a better message.

>
http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode598
> src/cmd/dist/build.c:598: vadd(&compile, "-m64");
> Should you use -m32 if gohostarch == i386?

Done.

>
http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/build.c#newcode642
> src/cmd/dist/build.c:642: b.p[b.len-1] = 'o';  // was c
> was c or s

Done.

> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c#newcode91
> src/cmd/dist/unix.c:91: bwritestr(&cmd, "$WORK");
> What is this about?

For debugging, it's nice to see the commands, but on a Mac
$TMPDIR is 50 characters long, so it's nice to see it shorter.
I commented that this is for logging.

> http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/unix.c#newcode519
> src/cmd/dist/unix.c:519: xprintf("hostos %s\n", gohostos);
> Remove debug print.

Done.

>
http://codereview.appspot.com/5620045/diff/4001/src/cmd/dist/windows.c#newcod...
> src/cmd/dist/windows.c:169: break;
> error reporting?

Done.

Thanks for the careful review.
Russ

[rsc, 2012-02-02 23:10:55]

Has anyone looked at this enough to LGTM it?
Thanks.

[brainman, 2012-02-02 23:12:26]

On 2012/02/02 15:24:55, rsc wrote:
> ...  If you can tell me how to figure out
> whether the underlying operating system is a 64-bit or a
> 32-bit Windows, ...
> 

I can see some people wishing to use windows/386 on Windows 64 bit. I don't
think OS is the factor here - compiler is. I think, you should look for mingw
compiler and then decide if 32 or 64 bit compiler used:

#if defined(__MINGW32__)	// mingw compiler
#if defined(_WIN64)		// only if target is 64-bit
	fatal("win64");
#else
	fatal("win32");
#endif
#endif

Alex

[brainman, 2012-02-02 23:14:14]

Forgot to include http://jesusnjim.com/programming/common-compiler-defines.html.
But you could google for more.

Alex

[rsc, 2012-02-02 23:17:16]

I don't want to look at the compiler, because sometimes people
are running a compiler that defaults to 32-bit mode even on a
64-bit platform.  Fred's suggestion to use GetNativeSystemInfo
looks perfect, but I will make the change in a future CL.
(This CL is not enabling the tool yet.)

[brainman, 2012-02-02 23:32:03]

On 2012/02/02 23:17:16, rsc wrote:
> I don't want to look at the compiler, because sometimes people
> are running a compiler that defaults to 32-bit mode even on a
> 64-bit platform.  Fred's suggestion to use GetNativeSystemInfo
> looks perfect, ...

If you have Windows 64 bit, but your compiler is 32 bit. How would you build
windows-amd64 Go?

Regardless what we decide, I assume, you would allow it to be overridden by
environment variable or something. Wouldn't you?

Alex

[ality, 2012-02-02 23:38:33]

Russ Cox <rsc@golang.org> once said:
> Plan 9 is beyond the scope of this particular review.
> However, I would encourage you to compile this tool
> with pcc on Plan 9.

That's a good idea.  The diffs to support using pcc
are *much* smaller.

  Anthony

[rsc, 2012-02-03 00:15:21]

On Thu, Feb 2, 2012 at 18:32,  <alex.brainman@gmail.com> wrote:
> If you have Windows 64 bit, but your compiler is 32 bit. How would you
> build windows-amd64 Go?

You can build a 64-bit Go compiler using a 32-bit C compiler.
That works fine.  The usual case is that people have a 64-bit-capable
gcc installed but it defaults to 32-bit mode to avoid surprising people.

> Regardless what we decide, I assume, you would allow it to be overridden
> by environment variable or something. Wouldn't you?

Yes, definitely.  The environment variable always wins.

Russ

[brainman, 2012-02-03 00:17:23]

https://codereview.appspot.com/5620045/diff/1007/src/cmd/dist/windows.c
File src/cmd/dist/windows.c (right):

https://codereview.appspot.com/5620045/diff/1007/src/cmd/dist/windows.c#newco...
src/cmd/dist/windows.c:316: n = GetCurrentDirectory(0, nil);
s/ry/ryW/

https://codereview.appspot.com/5620045/diff/1007/src/cmd/dist/windows.c#newco...
src/cmd/dist/windows.c:363: return attr >= 0 && (attr &
FILE_ATTRIBUTE_DIRECTORY);
Not sure about attr >=0. I think it always be true, even for errors.
GetFileAttributesW returns DWORD = uint32. If error, INVALID_FILE_ATTRIBUTES is
returned.

https://codereview.appspot.com/5620045/diff/1007/src/cmd/dist/windows.c#newco...
src/cmd/dist/windows.c:375: return attr >= 0 && !(attr &
FILE_ATTRIBUTE_DIRECTORY);
Same.

https://codereview.appspot.com/5620045/diff/1007/src/cmd/dist/windows.c#newco...
src/cmd/dist/windows.c:391: return 0;
Must close handle once you are done.
FindClose(h);

[iant, 2012-02-03 00:30:43]

LGTM

This is going to be slower than running a parallel make, perhaps that won't
bother us too much.

http://codereview.appspot.com/5620045/diff/1007/src/cmd/dist/unix.c
File src/cmd/dist/unix.c (right):

http://codereview.appspot.com/5620045/diff/1007/src/cmd/dist/unix.c#newcode354
src/cmd/dist/unix.c:354: if(mkdtemp(bstr(&b)) == nil)
Just a note: mkdtemp is a relatively recent function--we may need to replace
this one for greater portability.

http://codereview.appspot.com/5620045/diff/1007/src/cmd/dist/unix.c#newcode594
src/cmd/dist/unix.c:594: ep = p+strlen(p);
Why not just call strrchr?  It's in C89 and should be at least as available as
strstr.  If you don't just cal strrchr, note that ep is initialized twice.

[rsc, 2012-02-03 00:38:14]

On Thu, Feb 2, 2012 at 19:30,  <iant@golang.org> wrote:
> This is going to be slower than running a parallel make, perhaps that
> won't bother us too much.

Yes, I measured that earlier today.  It's unfortunate.  We could have
a background version of run and explicit wait if we wanted to, but
I think that can wait.

Russ

[rsc, 2012-02-03 00:38:55]

On Thu, Feb 2, 2012 at 19:30,  <iant@golang.org> wrote:
> Why not just call strrchr?  It's in C89 and should be at least as
> available as strstr.  If you don't just cal strrchr, note that ep is
> initialized twice.

Thanks.  I always confuse strrchr with memrchr; the latter
is *not* widely available.  Will use strrchr here.

Russ

[rsc, 2012-02-03 00:41:44]

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

cmd/dist: new command

dist is short for distribution.  This is the new Go distribution tool.

The plan is to replace the Makefiles with what amounts to
'go tool dist bootstrap', although it cannot be invoked like
that since it is in charge of getting us to the point where we
can build the go command.

It will also add additional commands to replace bash scripts
like test/run (go tool dist testrun), eventually eliminating our
dependence on not just bash but all the Unix tools and all
of cygwin.

This is strong enough to build (cc *.c) and run (a.out bootstrap)
to build not just the C libraries and tools but also the basic
Go packages up to the bootstrap form of the go command
(go_bootstrap).  I've run it successfully on both Linux and Windows.
This means that once we've switched to this tool in the build,
we can delete the buildscripts.

This tool is not nearly as nice as the go tool.  There are many
special cases that turn into simple if statements or tables in
the code.  Please forgive that.  C does not enjoy the benefits
that we designed into Go.

I was planning to wait to do this until after Go 1, but the
Windows builders are both broken due to a bug in either
make or bash or both involving the parsing of quoted command
arguments.  Make thinks it is invoking

        quietgcc -fno-common -I"c:/go/include" -ggdb -O2 -c foo.c

but bash (quietgcc is a bash script) thinks it is being invoked as

        quietgcc -fno-common '-Ic:/go/include -ggdb' -O2 -c foo.c

which obviously does not have the desired effect.  Rather than fight
these clumsy ports, I accelerated the schedule for the new tool.
We should be completely off cygwin (using just the mingw gcc port,
which is much more standalone) before Go 1.

It is big for a single CL, and for that I apologize.  I can cut it into
separate CLs along file boundaries if people would prefer that.

R=golang-dev, adg, gri, bradfitz, alex.brainman, dsymonds, iant, ality,
hcwfrichter
CC=golang-dev
http://codereview.appspot.com/5620045