code review 5011046: build: add build comments to core packages

build: add build comments to core packages

The go/build package already recognizes
system-specific file names like

        mycode_darwin.go
        mycode_darwin_386.go
        mycode_386.s

However, it is also common to write files that
apply to multiple architectures, so a recent CL added
to go/build the ability to process comments
listing a set of conditions for building.  For example:

        // +build darwin freebsd openbsd/386

says that this file should be compiled only on
OS X, FreeBSD, or 32-bit x86 OpenBSD systems.

These conventions are not yet documented
(hence this long CL description).

This CL adds build comments to the multi-system
files in the core library, a step toward making it
possible to use go/build to build them.

With this change go/build can handle crypto/rand,
exec, net, path/filepath, os/user, and time.

os and syscall need additional adjustments.

[rsc, 2011-09-15 16:26:05]

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

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

[r, 2011-09-15 17:04:21]

do they have to be

//build

as opposed to

// build

?

[gri, 2011-09-15 17:08:08]

it does match the //line comments we already have (though I wish it
were // line, personally)
- gri

On Thu, Sep 15, 2011 at 10:04 AM,  <r@golang.org> wrote:
> do they have to be
>
> //build
>
> as opposed to
>
> // build
>
> ?
>
> http://codereview.appspot.com/5011046/
>

[rsc, 2011-09-15 17:09:05]

On Thu, Sep 15, 2011 at 13:04,  <r@golang.org> wrote:
> do they have to be
>
> //build
>
> as opposed to
>
> // build

as implemented, yes, same as //line.

in both cases, it makes it unlikely that you'll just
pick off a random comment that happens
to start with a common word and start applying
meaning to it.

[r2, 2011-09-15 17:15:58]

On Sep 15, 2011, at 10:07 AM, Robert Griesemer wrote:

> it does match the //line comments we already have (though I wish it
> were // line, personally)

that begs the question.

-rob

[rsc, 2011-09-15 17:16:41]

there's always // pragma line

[r2, 2011-09-15 17:16:59]

On Sep 15, 2011, at 10:09 AM, Russ Cox wrote:

> On Thu, Sep 15, 2011 at 13:04,  <r@golang.org> wrote:
>> do they have to be
>> 
>> //build
>> 
>> as opposed to
>> 
>> // build
> 
> as implemented, yes, same as //line.
> 
> in both cases, it makes it unlikely that you'll just
> pick off a random comment that happens
> to start with a common word and start applying
> meaning to it.

fair enough (although i first typed that as gsit rnouh)

-rob

[r2, 2011-09-15 17:19:42]

there's also the way the tests do it, where the first line is the magic one. 
makes the check for comments easier and allows more free-form commentary.

-rob

[rsc, 2011-09-15 17:21:56]

On Thu, Sep 15, 2011 at 13:19, Rob 'Commander' Pike <r@google.com> wrote:
> there's also the way the tests do it, where the first line is the magic one.

true, although for //line that's not an option,
because they are by definition interspersed
with the rest of the source file.

the current CL requires that the //build comment
be before the package statement, just so you
don't have to look through the entire file.
it could require it to be the first line, although
i do like not having the copyright in the
middle of the file.

i'm inclined to leave it as //build for now,
since //line has already set a precedent.

russ

[r2, 2011-09-15 17:31:42]

On Sep 15, 2011, at 10:21 AM, Russ Cox wrote:

> On Thu, Sep 15, 2011 at 13:19, Rob 'Commander' Pike <r@google.com> wrote:
>> there's also the way the tests do it, where the first line is the magic one.
> 
> true, although for //line that's not an option,
> because they are by definition interspersed
> with the rest of the source file.

i'm not talking about //line

> the current CL requires that the //build comment
> be before the package statement, just so you
> don't have to look through the entire file.
> it could require it to be the first line, although
> i do like not having the copyright in the
> middle of the file.

for it to be in the middle of the file, the file would have to consist of one
executable line after the copyright.

moreover, i don't see what this concept has to do with the package clause and
therefore see little reason other than pragmatism to attach the marker there.

> i'm inclined to leave it as //build for now,
> since //line has already set a precedent.

a precedent for a completely unrelated concept, so not really a precedent at
all. i think the test dir is a much more applicable precedent.

-rob

[gustavo_niemeyer.net, 2011-09-15 17:39:01]

> a precedent for a completely unrelated concept, so not really a precedent at
all. i think the test dir is a much more applicable precedent.

In terms of precedent, there's also cgo's:

// #cgo linux CFLAGS: ...

So the analogy is not far-fetched, FWIW:

// #build linux

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[rsc, 2011-09-15 17:40:29]

On Thu, Sep 15, 2011 at 13:38, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> In terms of precedent, there's also cgo's:
>
> // #cgo linux CFLAGS: ...

that is completely different.
no one uses // for those, and they are only
interpeted in the doc comment attached
to import "C".  they are like

/*
#cgo CFLAGS: -m32
#include <stdio.h>
*/
import "C"

[rsc, 2011-09-15 18:15:18]

giving line 1 special meaning doesn't help when the
second thing like this comes along.

also people expect that the copyright notice is
boilerplate and skip over it.  putting meaningful lines
above it hides them.  having to add the copyright
notices to the tests exposed the problem with first-line magic.

i think the requirements for the syntax are:
(1) allow some unrelated leading comments such as copyright
(2) very low chance of falsely identifying an ordinary
comment as a directive.
(3) can be applied without parsing the file, so that
the rule could even apply to C files.

the current rule is that (rule A) in a file's
leading run of comments and blank lines,
a line beginning with //build (no /*)
is taken as a build directive.
(we've done this before with //line and //gotest,
so i didn't expect this to be contentious.)

we could change the rule to be that (rule B) in a file's
leading run of comments and blank lines
that is followed by a blank line,
a line beginning with "// build" (no /*, no tabs)
is taken as a build directive.

thus this:

---
// Copyright...
// whatever

// whatever
// build foo

package main
---

contains a build comment but this

---
// Copyright...
// whatever

// whatever
// build foo
package main
---

does not (the last few comments are not followed
by a blank line so they are not part of the leading run).

i should not have mentioned the package clause
in my last mail.  it's just a way to identify 'the run of
comments and blank lines at the top of a file' in a
Go program, since the package clause is always
the first non-comment.

russ

[r2, 2011-09-15 18:24:28]

ok, you've convinced me. but please allow for a little more free-formness, which
means not using "build" but something more specific, like "-build" or ":build",
and allow spaces.

-rob

[rsc, 2011-09-15 18:58:05]

Hello golang-dev@googlegroups.com, r@golang.org, gri@golang.org, r@google.com,
gustavo@niemeyer.net (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2011-09-15 19:29:33]

LGTM

[rsc, 2011-09-15 20:49:00]

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

build: add build comments to core packages

The go/build package already recognizes
system-specific file names like

        mycode_darwin.go
        mycode_darwin_386.go
        mycode_386.s

However, it is also common to write files that
apply to multiple architectures, so a recent CL added
to go/build the ability to process comments
listing a set of conditions for building.  For example:

        // +build darwin freebsd openbsd/386

says that this file should be compiled only on
OS X, FreeBSD, or 32-bit x86 OpenBSD systems.

These conventions are not yet documented
(hence this long CL description).

This CL adds build comments to the multi-system
files in the core library, a step toward making it
possible to use go/build to build them.

With this change go/build can handle crypto/rand,
exec, net, path/filepath, os/user, and time.

os and syscall need additional adjustments.

R=golang-dev, r, gri, r, gustavo
CC=golang-dev
http://codereview.appspot.com/5011046