code review 114640044: go/build: perform simple variable substitution in #cgo lines

go/build: perform simple variable substitution in #cgo lines

Expands $SRCDIR and ${SRCDIR}, into the path to the source directory of the package being built.

Fixes Issue 7891.
Fixes Issue 5428.

[cookieo9, 2014-08-01 14:31:26]

Hello golang-codereviews@googlegroups.com,

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

[slimsag, 2014-08-01 17:35:32]

On 2014/08/01 14:31:26, cookieo9 wrote:
> Hello mailto:golang-codereviews@googlegroups.com,
> 
> I'd like you to review this change to
> http://code.google.com/p/go

LGTM but why do we need expansion for ${VAR} ? I think it is useless, if $VAR
works then we don't need the other form unless I am missing something crucial.

[cookieo9, 2014-08-01 21:35:12]

On 2014/08/01 17:35:32, slimsag wrote:
> On 2014/08/01 14:31:26, cookieo9 wrote:
> > Hello mailto:golang-codereviews@googlegroups.com,
> > 
> > I'd like you to review this change to
> > http://code.google.com/p/go
> 
> LGTM but why do we need expansion for ${VAR} ? I think it is useless, if $VAR
> works then we don't need the other form unless I am missing something crucial.

The ${VAR} format is used tp make the variable easier to use when there is no
space between the variable and the next character.

For example, underscore is a valid character in a variable name, so
$GOOS_$GOARCH might not give you what you expect compared to ${GOOS}_${GOARCH}.
In this case it does, but thats because of the prefix issue shown in the
playground snippet.

[slimsag, 2014-08-01 23:44:34]

On 2014/08/01 21:35:12, cookieo9 wrote:
> On 2014/08/01 17:35:32, slimsag wrote:
> > On 2014/08/01 14:31:26, cookieo9 wrote:
> > > Hello mailto:golang-codereviews@googlegroups.com,
> > > 
> > > I'd like you to review this change to
> > > http://code.google.com/p/go
> > 
> > LGTM but why do we need expansion for ${VAR} ? I think it is useless, if
$VAR
> > works then we don't need the other form unless I am missing something
crucial.
> 
> The ${VAR} format is used tp make the variable easier to use when there is no
> space between the variable and the next character.
> 
> For example, underscore is a valid character in a variable name, so
> $GOOS_$GOARCH might not give you what you expect compared to
${GOOS}_${GOARCH}.
> In this case it does, but thats because of the prefix issue shown in the
> playground snippet.

Thanks. I did briefly look over the play link, but I wanted to make sure I
understood correctly.

Do you mean to say that ${VAR} is for when multiple variables would conflict due
to them sharing the same prefix? For example http://play.golang.org/p/hKbpTy8WXF

If so, I understand the problem but I disagree with having two separate syntax.
It's confusing and strange.

We should either stick with just $VAR and no variables may share the same
prefix, and since we only want a few distinctive variables (namely $GOOS,
$GOARCH, and $SRCDIR) this is okay (but it wouldn't allow for much future
expansion).

Or we can just use ${VAR} and not allow $VAR syntax. Obviously people will
expect the syntax to be $VAR given *nix, but if it causes such problems that we
must create two syntaxes, it's more confusing.

That's just my stance though, it'd be good to hear some others chime in.
Stephen

[cookieo9, 2014-08-02 01:43:35]

On 2014/08/01 23:44:34, slimsag wrote:
> On 2014/08/01 21:35:12, cookieo9 wrote:
> > On 2014/08/01 17:35:32, slimsag wrote:
> > > On 2014/08/01 14:31:26, cookieo9 wrote:
> > > > Hello mailto:golang-codereviews@googlegroups.com,
> > > > 
> > > > I'd like you to review this change to
> > > > http://code.google.com/p/go
> > > 
> > > LGTM but why do we need expansion for ${VAR} ? I think it is useless, if
> $VAR
> > > works then we don't need the other form unless I am missing something
> crucial.
> > 
> > The ${VAR} format is used tp make the variable easier to use when there is
no
> > space between the variable and the next character.
> > 
> > For example, underscore is a valid character in a variable name, so
> > $GOOS_$GOARCH might not give you what you expect compared to
> ${GOOS}_${GOARCH}.
> > In this case it does, but thats because of the prefix issue shown in the
> > playground snippet.
> 
> Thanks. I did briefly look over the play link, but I wanted to make sure I
> understood correctly.
> 
> Do you mean to say that ${VAR} is for when multiple variables would conflict
due
> to them sharing the same prefix? For example
http://play.golang.org/p/hKbpTy8WXF
> 
> If so, I understand the problem but I disagree with having two separate
syntax.
> It's confusing and strange.
> 
> We should either stick with just $VAR and no variables may share the same
> prefix, and since we only want a few distinctive variables (namely $GOOS,
> $GOARCH, and $SRCDIR) this is okay (but it wouldn't allow for much future
> expansion).
> 
> Or we can just use ${VAR} and not allow $VAR syntax. Obviously people will
> expect the syntax to be $VAR given *nix, but if it causes such problems that
we
> must create two syntaxes, it's more confusing.
> 
> That's just my stance though, it'd be good to hear some others chime in.
> Stephen

The ${VAR} format is common to most shells that support the $VAR format, and it
ensures that there is no mistake deciding which of the letters in the string are
designating a variable name, and which are designating raw characters that
appear in the output unmodified.

Example: $GOOS_$GOARCH_v2 in a regular shell will expand to use value of the
(likely non existant) "GOOS_" and "GOARCH_v2" variables because in standard
shell syntax underscores and lower-case letters are valid in variable names. The
solution in a shell is to write "${GOOS}_${GOARCH}_v2" instead to tell the
parser where variable names begin and end.

My goals with this patch were as follows:
 1 - Support the $SRCDIR variable to fix the issue
 2 - Support the $GOOS and $GOARCH variables, as mentioned in comments to the
issue, as they would be useful and easy to add
 3 - Retain support for passing variables like $ORIGIN through to the system
linker by passing everything but the supported variables through un-altered
 4 - Support the ${VAR} syntax, since there are be situations where it's needed
to solve ambiguities
 5 - Allow all code, not just the go tool, to use these substitutions correctly.
This is why the change is here, and not in the cmd/go package.
 
Currently the three solutions I tried (as listed in the playground snippet) each
have a problem with them:
 1 - The os.Expand solution breaks #3 as it is not guaranteed to pass what you
originally typed in through (by converting ${VAR} to $VAR) which might not be a
serious problem
 2 - The strings.Replace solution also breaks #3 in a more obscure way via the
prefix problem
 3 - The regexp solution adds another import to the go/build package which is
not allowed

With enough hacking I can probably fix the current code (#2) to handle prefixes,
or create a modified version of os.Expand, but I wanted to limit the size of the
patch until one or the other was deemed necessary.

[cookieo9, 2014-08-02 07:53:40]

A more complex, but accurate expandVars function (if needed):
http://play.golang.org/p/W0TKkOAHsF

[slimsag, 2014-08-02 16:54:40]

I think I understand your points, but I still argue that #cgo lines do not
specifically have to be shell-like. The question I would ask still remains:

Would it be better to drop $VAR syntax and just go with ${VAR} which would work
better in all cases?

Nonetheless, I don't think it's too big a deal, and at this point it's obviously
just preference or personal opinion. So LGTM-as-is.

[cookieo9, 2014-08-03 05:10:25]

On 2014/08/02 16:54:40, slimsag wrote:
> I think I understand your points, but I still argue that #cgo lines do not
> specifically have to be shell-like. The question I would ask still remains:
> 
> Would it be better to drop $VAR syntax and just go with ${VAR} which would
work
> better in all cases?
> 
> Nonetheless, I don't think it's too big a deal, and at this point it's
obviously
> just preference or personal opinion. So LGTM-as-is.

I've removed the note about the different methods from the issue description,
but I realize now that the comments refer to "the playground snippet" with it
being no longer present. So if anyone wants to follow along here it is:
http://play.golang.org/p/ntxFb0-enN

[cookieo9, 2014-08-03 05:53:54]

I have added table driven tests for the expandVars function. The tests check as
many situations I can think of, and have an adjusted expected value for the
broken behaviour w.r.t. prefixes. 

Should a better solution be implemented, that test case needs to be fixed (as
mentioned in the comment).

PTAL

[aram, 2014-08-04 12:15:38]

NOT LGTM

Please open a thread on golang-dev about this.

It is very unclear why this is needed at all. GOOS and GOARCH interpretation is
not needed as the use case is already handled by build tags and conditional
compilation in general. All that remains is SRCDIR/ORIGIN. People want it, but
it is not clear why, or whether it's a good idea at all. Many people have asked
me about this feature, and when I tried to understand better why they wanted it,
they either couldn't explain, or they wanted to do something truly horrible.

Please discuss this on -dev.

[slimsag, 2014-08-04 17:08:59]

On 2014/08/04 12:15:38, aram wrote:
> NOT LGTM
> 
> Please open a thread on golang-dev about this.
> 
> It is very unclear why this is needed at all. GOOS and GOARCH interpretation
is
> not needed as the use case is already handled by build tags and conditional
> compilation in general. All that remains is SRCDIR/ORIGIN. People want it, but
> it is not clear why, or whether it's a good idea at all. Many people have
asked
> me about this feature, and when I tried to understand better why they wanted
it,
> they either couldn't explain, or they wanted to do something truly horrible.
> 
> Please discuss this on -dev.

Discussion:

https://groups.google.com/forum/#!topic/golang-dev/rr8yNScB4Lg

[gobot, 2014-12-19 05:10:54]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/114640044 is best) in the description in your
new CL.

Thanks very much.