code review 5554079: go/build: add BuildTags to Context, allow !tag

go/build: add BuildTags to Context, allow !tag

This lets the client of go/build specify additional tags that
can be recognized in a // +build directive.  For example,
a build for a custom environment like App Engine might
include "appengine" in the BuildTags list, so that packages
can be written with some files saying

        // +build appengine   (build only on app engine)

or

        // +build !appengine  (build only when NOT on app engine)

App Engine here is just a hypothetical context.  I plan to use
this in the cmd/go sources to distinguish the bootstrap version
of cmd/go (which will not use networking) from the full version
using a custom tag.  It might also be useful in App Engine.

Also, delete Build and Script, which we did not end up using for
cmd/go and which never got turned on for real in goinstall.

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

Hello golang-dev@googlegroups.com,

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

[r, 2012-01-21 22:45:05]

code's fine. where is it documented?

http://codereview.appspot.com/5554079/diff/3001/src/cmd/goinstall/main.go
File src/cmd/goinstall/main.go (right):

http://codereview.appspot.com/5554079/diff/3001/src/cmd/goinstall/main.go#new...
src/cmd/goinstall/main.go:47: useMake           = flag.Bool("make", true, "use
make to build and install (obsolete, always true)")
the words don't feel right

http://codereview.appspot.com/5554079/diff/3001/src/pkg/crypto/tls/root_stub.go
File src/pkg/crypto/tls/root_stub.go (right):

http://codereview.appspot.com/5554079/diff/3001/src/pkg/crypto/tls/root_stub....
src/pkg/crypto/tls/root_stub.go:5: // +build plan9 darwin/!cgo
i don't see where this syntax is documented

[rsc, 2012-01-22 04:12:52]

On Sat, Jan 21, 2012 at 17:45,  <r@golang.org> wrote:
> the words don't feel right

agreed but the surrounding directory will be gone in a few days.

[adg, 2012-01-22 23:39:13]

Can you please add docs for +build directives to go/build in this CL?

http://codereview.appspot.com/5554079/diff/3001/src/pkg/go/build/dir.go
File src/pkg/go/build/dir.go (right):

http://codereview.appspot.com/5554079/diff/3001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:572: if strings.HasPrefix(name, "!!") { // bad syntax,
reject always
Seems odd to reject this when the general case will work as expected.

[rsc, 2012-01-23 17:16:03]

On Sun, Jan 22, 2012 at 18:39,  <adg@golang.org> wrote:
> Seems odd to reject this when the general case will work as expected.

I am not building an arbitrary expression parser here.

[rsc, 2012-01-23 17:16:22]

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

Please take another look.

[rsc, 2012-01-23 17:16:34]

Large comment added.

[r, 2012-01-23 18:42:53]

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go
File src/pkg/go/build/dir.go (right):

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:155: // Files ending in _test.go are treated as test
files and included in
should this comment be at the top of the file?

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:156: // info.TestGoFiles info.XTestGoFiles list instead
of info.GoFiles.
can't parse this

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:166: // Build Constraints
good to have here for now, but really it needs to be described outside the code.
the issue of documentation of flags, usage and best practices can wait for
another day, but it's very serious.

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:186: // Each option is itself a /-separated list of
terms, satisfied only when
s;/;slash;

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:187: // all the terms are satisfied.  In the example
above, ``linux/amd64'' is
drop the quotes.

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:188: // satisfied when both linux and amd64 are
satisfied.
does amd64/linux work too? the similarity to file names is unfortunate and, to
be honest, completely misled me until i read this. i suggest comma or ampersand
instead.

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:192: // and vice versa.
not using slash would make what is now linux/!cgo a lot easier to understand. i
was baffled when i first saw it

[rsc, 2012-01-23 19:40:16]

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go
File src/pkg/go/build/dir.go (right):

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:155: // Files ending in _test.go are treated as test
files and included in
I deleted this paragraph in favor of changing the
comment on GoFiles in DirInfo.

More generally, I agree that this big comment belongs elsewhere,
but right now I don't know where that should be.  I want to think
a bit more about what go/build is for, and that will inform whether
this belongs in a package comment.

It definitely belongs in a "how to use the go tool" document too,
but at least it is written down in one place now.

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:156: // info.TestGoFiles info.XTestGoFiles list instead
of info.GoFiles.
On 2012/01/23 18:42:53, r wrote:
> can't parse this

Deleted.

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:166: // Build Constraints
On 2012/01/23 18:42:53, r wrote:
> good to have here for now, but really it needs to be described outside the
code.
> the issue of documentation of flags, usage and best practices can wait for
> another day, but it's very serious.

Agreed.

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:188: // satisfied when both linux and amd64 are
satisfied.
On 2012/01/23 18:42:53, r wrote:
> does amd64/linux work too? the similarity to file names is unfortunate and, to
> be honest, completely misled me until i read this. i suggest comma or
ampersand
> instead.

Changed to comma.

http://codereview.appspot.com/5554079/diff/9001/src/pkg/go/build/dir.go#newco...
src/pkg/go/build/dir.go:192: // and vice versa.
On 2012/01/23 18:42:53, r wrote:
> not using slash would make what is now linux/!cgo a lot easier to understand.
i
> was baffled when i first saw it

Done.

[rsc, 2012-01-23 19:40:34]

PTAL

[r, 2012-01-23 19:50:50]

LGTM for comment in dir.go after suggested rearrangement

http://codereview.appspot.com/5554079/diff/13001/src/pkg/go/build/dir.go
File src/pkg/go/build/dir.go (right):

http://codereview.appspot.com/5554079/diff/13001/src/pkg/go/build/dir.go#newc...
src/pkg/go/build/dir.go:185: // Each option is itself an comma-separated list of
terms, satisfied only when
s/an/a/

http://codereview.appspot.com/5554079/diff/13001/src/pkg/go/build/dir.go#newc...
src/pkg/go/build/dir.go:188: // have the same meaning.
this is much better, but please do the following
1) define the syntax of one build option, so we know the 'and' of multiple
conditions
2) define the syntax for multiple options, so we know the 'or'
the current structure oscillates between 'and' and 'or' and is confusing.

[rsc, 2012-01-23 20:08:37]

On Mon, Jan 23, 2012 at 14:50,  <r@golang.org> wrote:
> 1) define the syntax of one build option, so we know the 'and' of
> multiple conditions
> 2) define the syntax for multiple options, so we know the 'or'
> the current structure oscillates between 'and' and 'or' and is
> confusing.

I don't believe the current structure oscillates: it is top down.
I kept it top down but shortened it, which should help:

// Build Constraints
//
// A build constraint is a line comment beginning with the directive +build
// that lists the conditions under which a file should be included in
the package.
// Constraints may appear in any kind of source file (not just Go), but
// they must be appear near the top of the file, preceded
// only by blank lines and other line comments.
//
// A build constraint is evaluated as the OR of space-separated options;
// each option evaluates as the AND of ots comma-separated terms;
// and each term is an alphanumeric word or, preceded by !, its negation.
// That is, the build constraint:
//
//	// +build linux,386 darwin,!cgo
//
// corresponds to the boolean formula:
//
//	(linux AND 386) OR (darwin AND (NOT cgo))
//
// During a particular build, the following words are satisfied:
//
//	- the target operating system, as spelled by runtime.GOOS
//	- the target architecture, as spelled by runtime.GOARCH
//	- "cgo", if ctxt.CgoEnabled is true
//	- any additional words listed in ctxt.BuildTags
//
// If a file's name, after stripping the extension and a possible _test suffix,
// matches *_GOOS, *_GOARCH, or *_GOOS_GOARCH for any known operating
// system and architecture values, then the file is considered to have
an implicit
// build constraint requiring those terms.

[rsc, 2012-01-23 20:16:41]

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

go/build: add BuildTags to Context, allow !tag

This lets the client of go/build specify additional tags that
can be recognized in a // +build directive.  For example,
a build for a custom environment like App Engine might
include "appengine" in the BuildTags list, so that packages
can be written with some files saying

        // +build appengine   (build only on app engine)

or

        // +build !appengine  (build only when NOT on app engine)

App Engine here is just a hypothetical context.  I plan to use
this in the cmd/go sources to distinguish the bootstrap version
of cmd/go (which will not use networking) from the full version
using a custom tag.  It might also be useful in App Engine.

Also, delete Build and Script, which we did not end up using for
cmd/go and which never got turned on for real in goinstall.

R=r, adg
CC=golang-dev
http://codereview.appspot.com/5554079