VersionInfo: allow non-linkstamp rules to link in definitions #6803

junr03 · 2019-05-03T21:02:48Z

Signed-off-by: Jose Nino jnino@lyft.com

Description: currently envoy uses linkstamp to get access to workspace defined revision and status info for the VersionInfo class. However, if version_lib is depended on by a non-binary top level bazel target, the linkstamp attribute is not used and the linker fails to get definitions for:

envoy/source/common/common/version.cc

Lines 8 to 9 in 1bbf271

    
           extern const char build_scm_revision[]; 
        
           extern const char build_scm_status[];

This PR adds a genrule generated header in order to include definitions for the necessary names.

Risk Level: Medium. This PR changes dependencies on VersionLib which is used on the server.
Testing: I have compiled a static binary, and ran it as a smoke test to verify that the previous path still works as intended.
Docs Changes: added under bazel/README.md.
Release Notes: added.

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2019-05-03T21:07:52Z

bazel/BUILD

@@ -227,6 +227,11 @@ config_setting(
    values = {"cpu": "ios_arm64e"},
 )

+config_setting(
+    name = "manual_stamp",


open to better names

junr03 · 2019-05-03T21:09:04Z

source/common/common/BUILD

+    hdrs = select({
+        "//bazel:manual_stamp": [":generate_version_linkstamp"],
+        # By default the header file is empty.
+        # This is done so that the definitions linked via the linkstamp rule don't cause collisions.


Note that nothing prevents someone from running a rule that respects linkstamp and uses the manual_stamp define. I'll document this in the docs if this is the approach we congeal on.

junr03 · 2019-05-03T21:11:13Z

source/common/common/generate_version_linkstamp.sh

@@ -0,0 +1,13 @@
+#!/bin/bash


We don't have any bash in source. Open to moving this elsewhere.

tools is another option, but the current location is also good with me.

junr03 · 2019-05-03T21:17:21Z

source/common/common/BUILD

+        # By default the header file is empty.
+        # This is done so that the definitions linked via the linkstamp rule don't cause collisions.
+        "//conditions:default": [":generate_version_linkstamp_empty"],
+    }),
    copts = envoy_select_boringssl(
        ["-DENVOY_SSL_VERSION=\\\"BoringSSL-FIPS\\\""],
        ["-DENVOY_SSL_VERSION=\\\"BoringSSL\\\""],
    ),
    linkstamp = "version_linkstamp.cc",


@mattklein123 as I have implemented here, this PR does not solve for #2181 for two reasons:

The generated header still takes the value from the workspace status.

The bazel rule does not conditionally add the linkstamp attribute, so using --define manual_stamp=manual_stamp would actually cause duplicated definitions.

I am starting to think that the best way to resolve #2181 is to include a SOURCE_VERSION file and make that documented per the issue, or include it in the tarball somehow.

However, I am open to fixing by more bazel work here.

junr03 · 2019-05-03T21:17:36Z

This is an MVP. I have left a few comments in places where there might be better options available, would love any feedback you all have.

In general my bazel is pretty rudimentary, so this might not be the most elegant way to accomplish what I want. I am open to alternatives. Mostly I felt that in the absence of truly custom flags or full support for custom build flags bazelbuild/bazel#5577 the --define + select() approach was the best I could come up with.

cc @htuch @jmillikin-stripe @mattklein123

htuch

Generally seems reasonable, but a bit queasy about relying on bazel-out files directly.

htuch · 2019-05-06T15:34:35Z

source/common/common/BUILD

@@ -262,11 +279,21 @@ envoy_cc_library(
 envoy_cc_library(
    name = "version_lib",
    srcs = ["version.cc"],
+    hdrs = select({


This should be in srcs unless you are exporting.

The reason I had to put them in hdrs is because as far as I could tell (empirically and from docs) is that strip_include_prefix only words on hrds and not on header files in srcs. And I needed to do that in order to be able to have the same filename for the header file in version.cc

htuch · 2019-05-06T15:36:00Z

source/common/common/generate_version_linkstamp.sh

+# However, linkstamp is not available to non-binary bazel targets.
+# This means that if the topmost target being used to compile version_lib is a envoy_cc_library or related, linkstamp will not be in effect.
+# In turn this means that version_linkstamp.cc is not linked, and the build_scm_revision and build_scm_status are unknown symbols to the linker.
+build_scm_revision=$(grep BUILD_SCM_REVISION bazel-out/volatile-status.txt | sed 's/^BUILD_SCM_REVISION //' | tr -d '\\n')


How does this work when Envoy is included transitively in a project? I'm thinking something like envoy-filter-example, or whatever is being done for Envoy Mobile.

As long as the outer project is a git repo or it has a SOURCE_VERSION file it works the same as the current flow via linkstamp, i.e it uses the outer project's get_workspace_status command. In Envoy mobile this is set to Envoy's script -- envoy/bazel/get_workspace_status.

htuch · 2019-05-06T16:59:23Z

source/common/common/generate_version_linkstamp.sh

@@ -0,0 +1,13 @@
+#!/bin/bash


tools is another option, but the current location is also good with me.

source/common/common/generate_version_linkstamp.sh

junr03 · 2019-05-06T17:29:51Z

Generally seems reasonable, but a bit queasy about relying on bazel-out files directly.

Yep, makes me a bit queasy as well. Although the stamp attribute on the genrules gives us some guarantees that the file will exist. However, when done this way it is so close to the actual linkstamp process that it makes this feel extra silly.

We could take the route of solving #6803 (comment) and have the value be user defined (not sure how to plumb this quite yet, but I'll figure it out). That way we could close #2181 with this as well.

junr03 · 2019-05-06T18:47:08Z

@htuch I left some comments. lmk what you think.

Signed-off-by: Jose Nino <jnino@lyft.com>

htuch

Thanks @junr03. This is fine as an opt-in for the purposes you are after right now. I think over time we will learn more about the limitations of what's being done and hopefully Bazel improves its support for link stamping.

lizan

Generally LGTM, is in the long term we would like to switch to use genrule for both binary and library?

htuch · 2019-05-08T12:27:04Z

@lizan linkstamp is a bit more robust, so I would prefer to stick with that for binary build. I think long term we want to pursue the appropriate method in Bazel upstream.

@junr03 can you file an issue to track this as a feature (rather than just as something weird in the documented behavior)? I can share this internally with Bazel folks as well once that is done.

junr03 · 2019-05-08T17:25:23Z

@htuch absolutely agree on what you said. That is the reason I didn't switch linkstamp out for the binary for this behavior. linkstamp is definitely the right way to do all of this for both the binary build, and if possible in the future for arbitrary rules.

issue: #6859

Signed-off-by: Jose Nino <jnino@lyft.com>

VersionInfo: allow non-linkstamp rules to link in definitions

7180b49

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 commented May 3, 2019

View reviewed changes

mattklein123 assigned htuch and lizan May 3, 2019

htuch reviewed May 6, 2019

View reviewed changes

ref

2c72dec

Signed-off-by: Jose Nino <jnino@lyft.com>

htuch previously approved these changes May 7, 2019

View reviewed changes

lizan previously approved these changes May 7, 2019

View reviewed changes

junr03 mentioned this pull request May 8, 2019

bazel: expand linkstamp beyond binary rules. #6859

Open

docs

96b9399

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 dismissed stale reviews from lizan and htuch via 96b9399 May 8, 2019 18:27

Jose Nino added 2 commits May 8, 2019 11:29

Merge branch 'master' of github.com:envoyproxy/envoy into manual-stamp

af216bf

Signed-off-by: Jose Nino <jnino@lyft.com>

fix conflict

0c4c647

Signed-off-by: Jose Nino <jnino@lyft.com>

htuch approved these changes May 8, 2019

View reviewed changes

junr03 merged commit 683b875 into master May 8, 2019

junr03 deleted the manual-stamp branch May 9, 2019 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VersionInfo: allow non-linkstamp rules to link in definitions #6803

VersionInfo: allow non-linkstamp rules to link in definitions #6803

junr03 commented May 3, 2019 •

edited

Loading

junr03 May 3, 2019

junr03 May 3, 2019

junr03 May 3, 2019

htuch May 6, 2019

junr03 May 3, 2019

junr03 May 3, 2019

junr03 commented May 3, 2019 •

edited

Loading

htuch left a comment

htuch May 6, 2019

junr03 May 6, 2019

htuch May 6, 2019

junr03 May 6, 2019

htuch May 6, 2019

junr03 commented May 6, 2019

junr03 commented May 6, 2019

htuch left a comment

lizan left a comment

htuch commented May 8, 2019

junr03 commented May 8, 2019

	extern const char build_scm_revision[];
	extern const char build_scm_status[];

VersionInfo: allow non-linkstamp rules to link in definitions #6803

VersionInfo: allow non-linkstamp rules to link in definitions #6803

Conversation

junr03 commented May 3, 2019 • edited Loading

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

junr03 commented May 3, 2019 • edited Loading

htuch left a 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

junr03 commented May 6, 2019

junr03 commented May 6, 2019

htuch left a comment

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

htuch commented May 8, 2019

junr03 commented May 8, 2019

junr03 commented May 3, 2019 •

edited

Loading

junr03 commented May 3, 2019 •

edited

Loading