code review 5820070: build: catch API changes during build

build: catch API changes during build

Adds new file api/go1.txt, locking down the current API.
Any changes to the API will need to update that file.

run.bash (but not make.bash, or Windows) will check for
accidental API changes.

[bradfitz, 2012-03-15 22:59:03]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-03-15 23:00:08]

Hah, the API is too large:

$ hg mail 5820070
Issue updated. URL: http://codereview.appspot.com/5820070
Not uploading the patch for api/go1.txt because the file is too large.
Uploading patch for src/cmd/api/goapi.go
Uploading patch for src/run.bash

You can look at it yourself with:

$ mkdir $GOROOT/api
$ go tool api > $GOROOT/api/go1.txt


On Thu, Mar 15, 2012 at 3:59 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> build: catch API changes during build
>
> Adds new file api/go1.txt, locking down the current API.
> Any changes to the API will need to update that file.
>
> run.bash (but not make.bash, or Windows) will check for
> accidental API changes.
>
> Please review this at
http://codereview.appspot.com/**5820070/<http://codereview.appspot.com/5820070/>
>
> Affected files:
>  M src/cmd/api/goapi.go
>  M src/run.bash
>
>
>

[dsymonds, 2012-03-15 23:36:51]

http://codereview.appspot.com/5820070/diff/5001/src/run.bash
File src/run.bash (right):

http://codereview.appspot.com/5820070/diff/5001/src/run.bash#newcode94
src/run.bash:94: go tool api -c $GOROOT/api/go1.txt || echo $?
shouldn't that be "exit $?". We want the build to fail if this doesn't pass,
right?

But this script has "set -e" up top, so I think you can just s/||.*$//.

[bradfitz, 2012-03-16 00:41:10]

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

Please take another look.

[bradfitz, 2012-03-16 00:41:36]

On Thu, Mar 15, 2012 at 4:36 PM, <dsymonds@golang.org> wrote:

>
>
http://codereview.appspot.com/**5820070/diff/5001/src/run.bash<http://coderev...
> File src/run.bash (right):
>
>
http://codereview.appspot.com/**5820070/diff/5001/src/run.**bash#newcode94<ht...
> src/run.bash:94: go tool api -c $GOROOT/api/go1.txt || echo $?
> shouldn't that be "exit $?". We want the build to fail if this doesn't
> pass, right?
>

whoops, typo.


> But this script has "set -e" up top, so I think you can just s/||.*$//


yeah, but the rest of this file does || checks anyway, so I'm being
consistent.

PTAL

[dsymonds, 2012-03-16 02:01:08]

LGTM

[rsc, 2012-03-16 02:47:31]

Please drop the || exit $?.

The rest of the file only does them in places
where they are not implied by set -e
(in ( ) subprocess invocations).

[rsc, 2012-03-16 02:48:05]

Except for line 77, which would be great to fix too.

[dsymonds, 2012-03-16 02:56:18]

On Fri, Mar 16, 2012 at 1:47 PM, Russ Cox <rsc@golang.org> wrote:

> The rest of the file only does them in places
> where they are not implied by set -e
> (in ( ) subprocess invocations).

Even there I don't think they are working as one would expect.

The result of a subprocess invocation is the result of its last
command. If you run
  #!/bin/bash
  set -e
  (false)
  echo "ok"
then you won't get the "ok". The "|| exit $?" isn't doing *anything*
in run.bash, because run.bash would have exited anyway.

The tricky thing with "set -e" and subprocess invocation is that the
"set -e" doesn't propagate inside it, so subprocess invocations with
multiple commands won't exit early if one of their commands fails.

So, instead of line 88-90 looking like
 (xcd ../test
 time go run run.go
 ) || exit $?
it could just be
  (xcd ../test
  time go run run.go)
or
  (xcd ../test; time go run run.go)
or
  (xcd ../test && time go run run.go)

[rsc, 2012-03-16 03:10:32]

On Thu, Mar 15, 2012 at 22:56, David Symonds <dsymonds@golang.org> wrote:
> Even there I don't think they are working as one would expect.
>
> The result of a subprocess invocation is the result of its last
> command. If you run
>  #!/bin/bash
>  set -e
>  (false)
>  echo "ok"
> then you won't get the "ok". The "|| exit $?" isn't doing *anything*
> in run.bash, because run.bash would have exited anyway.

This at least is not true.  A command used on the left of an || or &&
is exempt from set -e processing.  Also we have a long history of
build failures suggesting that it is working just fine.

I do not want to change this today.  This is incredibly subtle and
varies from bash implementation to bash implementation.  It is
entirely possible that you get different results from different versions.
I do know that originally I did not write the || exit $? and the shell
script was not exiting when it should have.  Adding the || exit $?
for the subshells fixed that.

All I am asking is that we take it off the single commands
(lines 77, 86, and the new line 94), to match the other
single commands (lines 26, 30, 34).

Please DO NOT TOUCH the subshell invocations.
They are working as is.  After Go 1, if someone wants to sort this
out (and no doubt break the build scripts on some system or other),
that's fine.  But not before.

Russ

[dsymonds, 2012-03-16 03:24:47]

On Fri, Mar 16, 2012 at 2:10 PM, Russ Cox <rsc@golang.org> wrote:

> On Thu, Mar 15, 2012 at 22:56, David Symonds <dsymonds@golang.org> wrote:
>> Even there I don't think they are working as one would expect.
>>
>> The result of a subprocess invocation is the result of its last
>> command. If you run
>>  #!/bin/bash
>>  set -e
>>  (false)
>>  echo "ok"
>> then you won't get the "ok". The "|| exit $?" isn't doing *anything*
>> in run.bash, because run.bash would have exited anyway.
>
> This at least is not true.  A command used on the left of an || or &&
> is exempt from set -e processing.

Yes, and in the presence of set -e these are all equivalent:
  cmd
  (cmd)
  cmd || exit $?
  (cmd) || exit $?

> Also we have a long history of
> build failures suggesting that it is working just fine.

I didn't say it was breaking anything. It's just not adding anything.

> Please DO NOT TOUCH the subshell invocations.
> They are working as is.  After Go 1, if someone wants to sort this
> out (and no doubt break the build scripts on some system or other),
> that's fine.  But not before.

I agree. I was not suggesting they were broken, just misleading. I am
fine with waiting.


Dave.

[rsc, 2012-03-16 03:28:49]

On Thu, Mar 15, 2012 at 23:24, David Symonds <dsymonds@golang.org> wrote:
> Yes, and in the presence of set -e these are all equivalent:
>  cmd
>  (cmd)
>  cmd || exit $?
>  (cmd) || exit $?
>
>> Also we have a long history of
>> build failures suggesting that it is working just fine.
>
> I didn't say it was breaking anything. It's just not adding anything.

It certainly added something when I put it in originally.

Russ

[bradfitz, 2012-03-17 18:21:03]

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

build: catch API changes during build

Adds new file api/go1.txt, locking down the current API.
Any changes to the API will need to update that file.

run.bash (but not make.bash, or Windows) will check for
accidental API changes.

R=golang-dev, dsymonds, rsc
CC=golang-dev
http://codereview.appspot.com/5820070