Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bazel: fix git SHA embedding #841

Closed
wants to merge 2 commits into from
Closed

bazel: fix git SHA embedding #841

wants to merge 2 commits into from

Conversation

mattklein123
Copy link
Member

Previously, the SHA generation genrule used a glob over .git/**
to figure out if anything changed. This does not work when envoy is
embedded in another git repo as a submodule, or is potentially built
from a tarball. This commit reworks the generation to expect an env
var called GIT_BUILD_SHA which will be used as the final SHA in the
build. CI has been updated to use the GIT SHA being built from.

If the env var is not defined, the SHA is set to a default SHA.

This commit also removes a dependency on version_lib from test main,
which was causing all tests to be relinked for no reason.

Previously, the SHA generation genrule used a glob over .git/**
to figure out if anything changed. This does not work when envoy is
embedded in another git repo as a submodule, or is potentially built
from a tarball. This commit reworks the generation to expect an env
var called GIT_BUILD_SHA which will be used as the final SHA in the
build. CI has been updated to use the GIT SHA being built from.

If the env var is not defined, the SHA is set to a default SHA.

This commit also removes a dependency on version_lib from test main,
which was causing all tests to be relinked for no reason.
@mattklein123
Copy link
Member Author

@htuch I will add some docs tomorrow, but I wanted to get a pass on this. I'm not thrilled with this, but AFAICT it's the only way of doing this, since I don't think a genrule can be forced to run when an env var changes like a repository_rule can.

Also, it spits out this warning when I run the build:

WARNING: /home/mklein/.cache/bazel/_bazel_mklein/e891e82b12efccc4683c22d6a02939c1/external/envoy/source/version_generated/BUILD:9:1: in srcs attribute of cc_library rule @envoy//source/version_generated:version_generated: please do not import '@git_version//:version_generated.cc' directly. You should either move the file to this package or depend on an appropriate rule there. Since this rule was created by the macro 'envoy_cc_library', the error might have been caused by the macro implementation in /home/mklein/.cache/bazel/_bazel_mklein/e891e82b12efccc4683c22d6a02939c1/external/envoy/bazel/envoy_build_system.bzl:74:16.

If we care about this, I have no idea how to fix it.

Anyway, I think this solution should work for your internal build also. LMK if there is a way to make this better.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this seems like the way to go until Bazel has better support for git repository version info as first class.

EOF

cat <<EOF > BUILD
exports_files(["version_generated.cc"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you create a cc_library here instead of using exports_files, you will be able to avoid the warning, referring to it as @git_version//:version_generated instead.

@@ -5,3 +5,6 @@ load("//bazel:cc_configure.bzl", "cc_configure")

envoy_dependencies()
cc_configure()

load("//bazel:git_version.bzl", "git_version")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIt: nominal Bazel style is to have all load at top of file (https://bazel.build/versions/master/docs/skylark/build-style.html), but it's not a big issue in a short WORKSPACE file to me, just a FYI.


git_version_rule = repository_rule(
implementation = _impl,
environ = ["GIT_BUILD_SHA"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: add , at EOL to match BUILD style

git_version_rule = repository_rule(
implementation = _impl,
environ = ["GIT_BUILD_SHA"]
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: ) at beginning of line.

@@ -8,6 +8,6 @@ envoy_package()

envoy_cc_library(
name = "version_generated",
srcs = ["//:version_generated.cc"],
srcs = ["@git_version//:version_generated.cc"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should basically nuke this file once we have the cc_library change below, since it won't do anything. Then, anyone who wants //source/version_generated as a dep should get an external_dep to version_generated. We should bind version_generated to @git_version//:version_generated in the def git_version(): to make that work. Then.. we can easily handle this during our import :)

@htuch
Copy link
Member

htuch commented Apr 26, 2017

Adding a reference to bazelbuild/bazel#216 and #415 for posterity.

@mattklein123
Copy link
Member Author

Closing in favor of a different solution.

@mattklein123 mattklein123 deleted the fix_git_sha branch April 26, 2017 15:18
htuch added a commit to htuch/envoy that referenced this pull request Apr 26, 2017
mattklein123 pushed a commit that referenced this pull request Apr 27, 2017
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Description: actions infra is failing to start android 28. However, android 29 works fine. This PR updates CI to start the android emulator with android 29.
Risk Level: low
Testing: local and CI
Docs Changes: updated the docs to SDK version 29 for requirements

Fixes #841

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Description: actions infra is failing to start android 28. However, android 29 works fine. This PR updates CI to start the android emulator with android 29.
Risk Level: low
Testing: local and CI
Docs Changes: updated the docs to SDK version 29 for requirements

Fixes #841

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants