add support for injecting build stamps with link options #186

mikedanese · 2016-11-03T23:04:48Z

for a workspace with BUILD_SCM_VERSION=10 defined as a volatile status and a go binary rule:

go_binary(
    ....
    stamp = True,
    linkstamp = "my/pkg/version",
)

The go binary will be linked with the option -X my/pkg/version.BUILD_SCM_VERSION

bazel-io · 2016-11-03T23:04:50Z

Can one of the admins verify this patch?

pmbethe09 · 2016-11-09T21:53:56Z

How is this different from the 'x_defs' field already on the go_binary?

mikedanese · 2016-11-09T21:57:17Z

@pmbethe09 currently workspace status is not available from skylark: bazelbuild/bazel#1054 so it's not possible to use stamp info from the 'x_defs' field.

pmbethe09 · 2016-11-15T21:47:38Z

go/def.bzl

+    for f in stamp_input:
+      cmds += [
+          "while read -r stamp || [[ -n $stamp ]]; do",
+          "if echo $stamp | grep -qe '^.*[[:space:]].*$'; then",


is there a way to do this in skylark and less bash-like which to me is a hard to read?

it seems like it's not currently possible to access from skylark, per bazelbuild/bazel#1054.

would adding a comment explaining what these lines of shell are doing help?

"reads workspace status file, converting "KEY value" lines to "-X KEY=$linkstamp.value" arguments to the go linker."

ixdy · 2016-12-08T23:30:33Z

go/def.bzl

+    for f in stamp_input:
+      cmds += [
+          "while read -r stamp || [[ -n $stamp ]]; do",
+          "if echo $stamp | grep -qe '^.*[[:space:]].*$'; then",


it seems like it's not currently possible to access from skylark, per bazelbuild/bazel#1054.

ixdy · 2016-12-08T23:31:31Z

go/def.bzl

+
+  stamp_input = []
+  if ctx.attr.stamp and ctx.attr.linkstamp:
+    stamp_input = [ctx.info_file, ctx.version_file]


what are ctx.info_file and ctx.version_file? they don't seem to be documented on https://www.bazel.io/versions/master/docs/skylark/lib/ctx.html.

They are not documented because they are alpha still.

https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java#L816-L838

ixdy · 2016-12-08T23:34:29Z

go/def.bzl

+    for f in stamp_input:
+      cmds += [
+          "while read -r stamp || [[ -n $stamp ]]; do",
+          "if echo $stamp | grep -qe '^.*[[:space:]].*$'; then",


would adding a comment explaining what these lines of shell are doing help?

"reads workspace status file, converting "KEY value" lines to "-X KEY=$linkstamp.value" arguments to the go linker."

ixdy · 2016-12-08T23:35:57Z

go/def.bzl

+      cmds += [
+          "while read -r stamp || [[ -n $stamp ]]; do",
+          "if echo $stamp | grep -qe '^.*[[:space:]].*$'; then",
+          "STAMP_XDEFS+=(-X $(echo %s.${stamp} | tr ' ' '='));" % ctx.attr.linkstamp,


if linkstamp is empty, this results in -X .BUILD_TIMESTAMP=1234 - is what what we want?
also, is it legal for values to have spaces? do we need to add quotes around the values?

ixdy · 2016-12-08T23:39:33Z

go/def.bzl

@@ -558,7 +574,8 @@ go_library = rule(
 go_binary = rule(
    go_binary_impl,
    attrs = go_library_attrs + _crosstool_attrs + {
-        "stamp": attr.bool(default = False),
+        "stamp": attr.bool(default = True),


maybe update the root README.md explaining what stamp and linkstamp do?

Can we do this if/when we decide we like the design? What's the policy on compatibility? I'd like someone from go team to weigh in on this before publicizing, but it unblocks us now.

pmbethe09 · 2016-12-13T15:56:43Z

Jenkins, test this please.

pmbethe09 · 2016-12-21T20:17:43Z

Jenkins, test this please.

mikedanese · 2016-12-21T22:00:54Z

I haven't had a chance to get back to this yet.

ixdy · 2017-01-10T00:08:55Z

go/def.bzl

+  ]
+
+  stamp_input = []
+  if ctx.attr.stamp and ctx.attr.linkstamp:


should we maybe fail if stamp is true but linkstamp isn't set? otherwise it might be confusing why stamp isn't working.

(alternately, do we need both? or just use nonempty linkstamp as the setting?)

I dropped stamp.

pmbethe09 · 2017-01-10T16:02:16Z

Jenkins, test this please.

pmbethe09 · 2017-01-10T16:44:43Z

seems to be failing on linux:
"
bazel-out/local-fastbuild/bin/examples/stamped_bin/bazel-out/local-fastbuild/bin/examples/stamped_bin/stamped_bin.a.GoLinkFile.params: 10: bazel-out/local-fastbuild/bin/examples/stamped_bin/bazel-out/local-fastbuild/bin/examples/stamped_bin/stamped_bin.a.GoLinkFile.params: Syntax error: "(" unexpected
____Building complete.
"

ixdy · 2017-01-18T22:37:44Z

go/def.bzl

+          "while read -r stamp || [[ -n $stamp ]]; do",
+          # reads workspace status file, converting "KEY value" lines to "-X KEY=$linkstamp.value" arguments to the go linker.
+          "if echo $stamp | grep -qe '^.*[[:space:]].*$'; then",
+          "STAMP_XDEFS+=(-X \"$(echo \"%s.${stamp}\" | tr ' ' '=')\");" % ctx.attr.linkstamp,


It seems like we can't use shell arrays here.

maybe just build up a string? there might be subtle quoting or escaping issues, but I'm not sure of a better way.

ixdy · 2017-01-18T22:39:58Z

go/def.bzl

+    for f in stamp_input:
+      cmds += [
+          "while read -r stamp || [[ -n $stamp ]]; do",
+          # reads workspace status file, converting "KEY value" lines to "-X KEY=$linkstamp.value" arguments to the go linker.


I think this comment is wrong - it's -X $linkstamp.KEY=value, right?

jayconrod · 2017-02-16T18:59:48Z

go/def.bzl

+    "STAMP_XDEFS=()",
+  ]
+
+  stamp_input = []


Nit: consider renaming this to stamp_inputs to make it clear it's a list.

jayconrod · 2017-02-16T19:14:17Z

go/def.bzl

+          "while read -r stamp || [[ -n $stamp ]]; do",
+          # reads workspace status file, converting "KEY value" lines to "-X KEY=$linkstamp.value" arguments to the go linker.
+          "if echo $stamp | grep -qe '^.*[[:space:]].*$'; then",
+          "STAMP_XDEFS+=(-X \"$(echo \"%s.${stamp}\" | tr ' ' '=')\");" % ctx.attr.linkstamp,


Do we need the grep? It matches any line with whitespace, right? I'd imagine in some cases, the value for some key would be empty, and a line might not have any space. Maybe use sed instead?

echo $stamp | sed -e 's%^$[^ ]\+$ \?$.*$$%-Xa/b.\1=\2%'

jayconrod

Please also add documentation on the linkstamp attribute to README.md.

Fixes bazelbuild#233

mikedanese · 2017-02-22T19:07:00Z

@jayconrod thanks for finishing this off!

googlebot added the cla: yes label Nov 3, 2016

mikedanese force-pushed the stamp branch from 5cc4d30 to e60db1a Compare November 11, 2016 23:35

pmbethe09 reviewed Nov 15, 2016

View reviewed changes

ixdy reviewed Dec 8, 2016

View reviewed changes

mikedanese force-pushed the stamp branch from e60db1a to 3ffdd7c Compare December 13, 2016 22:56

mikedanese force-pushed the stamp branch 2 times, most recently from 1de13f7 to 2d45df0 Compare January 10, 2017 00:07

ixdy reviewed Jan 10, 2017

View reviewed changes

mikedanese force-pushed the stamp branch 3 times, most recently from cf5d35c to 4ca8724 Compare January 10, 2017 00:39

ixdy reviewed Jan 18, 2017

View reviewed changes

jayconrod reviewed Feb 16, 2017

View reviewed changes

jayconrod force-pushed the stamp branch from 4ca8724 to ffc2bef Compare February 21, 2017 22:51

add support for injecting build stamps with link options

a2debbd

Fixes bazelbuild#233

jayconrod force-pushed the stamp branch from ffc2bef to a2debbd Compare February 21, 2017 23:35

jayconrod merged commit 5fa3994 into bazelbuild:master Feb 22, 2017

mikedanese deleted the stamp branch February 22, 2017 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for injecting build stamps with link options #186

add support for injecting build stamps with link options #186

mikedanese commented Nov 3, 2016

bazel-io commented Nov 3, 2016

pmbethe09 commented Nov 9, 2016

mikedanese commented Nov 9, 2016

pmbethe09 Nov 15, 2016

ixdy Dec 8, 2016

ixdy Dec 8, 2016

ixdy Dec 8, 2016

ixdy Dec 8, 2016

mikedanese Jan 9, 2017

mikedanese Jan 9, 2017

ixdy Dec 8, 2016

ixdy Dec 8, 2016

ixdy Dec 8, 2016

mikedanese Jan 9, 2017

pmbethe09 commented Dec 13, 2016

pmbethe09 commented Dec 21, 2016

mikedanese commented Dec 21, 2016

ixdy Jan 10, 2017

mikedanese Jan 10, 2017

pmbethe09 commented Jan 10, 2017

pmbethe09 commented Jan 10, 2017

ixdy Jan 18, 2017

ixdy Jan 18, 2017

jayconrod Feb 16, 2017

jayconrod Feb 16, 2017

jayconrod left a comment

mikedanese commented Feb 22, 2017

add support for injecting build stamps with link options #186

add support for injecting build stamps with link options #186

Conversation

mikedanese commented Nov 3, 2016

bazel-io commented Nov 3, 2016

pmbethe09 commented Nov 9, 2016

mikedanese commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmbethe09 commented Dec 13, 2016

pmbethe09 commented Dec 21, 2016

mikedanese commented Dec 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmbethe09 commented Jan 10, 2017

pmbethe09 commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayconrod left a comment

Choose a reason for hiding this comment

mikedanese commented Feb 22, 2017