ci: release build with envoy standalone. #857
Conversation
This fixes an issue where the Envoy consumer git hash was picked up in the release binary instead of Envoy proper.
ci/do_ci.sh
Outdated
| echo "Building..." | ||
| bazel --batch build ${BAZEL_BUILD_OPTIONS} -c opt @envoy//source/exe:envoy-static.stripped.stamped | ||
| cd "${ENVOY_BUILD_DIR}" |
There was a problem hiding this comment.
Do we need this? Shouldn't we build from within source (current) directory? Otherwise, this results in Modified git version, since none of the source files are in the tree.
There was a problem hiding this comment.
We need it for bazel.coverage builds, due to the weirdness of the circular symlinks that Bazel creates where you build and gcovr not being able to cope with this. For dev/release builds, we don't, so I will fix.
There was a problem hiding this comment.
OK, I don't get it.
It seems that removing cd "${ENVOY_BUILD_DIR}" should fix the issue, and indeed, calling ./bazel/get_workspace_status from here yields Clean status, but the @envoy//source/exe:envoy-static.stripped.stamped binary still ends up having Modified build-id.
Running the same command from outside Docker results in Clean build-id.
Any ideas?
There was a problem hiding this comment.
Yeah, this is weird. It's even clean in the container immediately before/after the build, but bazel-out/volatile-status.txt says dirty. I'll take a look more in a bit, there's got to be a way.
bazelbuild/bazel#2893 is tracking my concerns over the documentation and support-level for Bazel workspace status and C++. I think it's not release quality yet, much like the coverage - there's in theory a way to do it if you hack around enough and spend O(days) figuring it out yourself, but no documented way of doing it or integration/regression tests.
There was a problem hiding this comment.
Should we merge this now or do you want to look into this today / tomorrow? Even with Modified, it's still better than what's currently in tree.
There was a problem hiding this comment.
Will look into it now, if we can't figure it out in O(1 hour) then we should merge.
| @@ -34,8 +33,8 @@ elif [[ "$1" == "bazel.asan" ]]; then | |||
| cd "${ENVOY_CONSUMER_SRCDIR}" | |||
| echo "Building and testing..." | |||
| # TODO(htuch): This should switch to using clang when available. | |||
| bazel --batch test ${BAZEL_TEST_OPTIONS} -c dbg --config=asan --test_output=all \ | |||
| --cache_test_results=no @envoy//test/... //:echo2_integration_test | |||
| bazel --batch test ${BAZEL_TEST_OPTIONS} -c dbg --config=asan @envoy//test/... \ | |||
There was a problem hiding this comment.
btw: we should use @envoy// prefix across the build, otherwise targets are going to be built twice - once for //... and once for @envoy//..., since Bazel thinks those are 2 different repositories.
There was a problem hiding this comment.
Yes, but it also refers to the "current" workspace, since that's the name defined in WORKSPACE.
Although, now that I tested it, it seems that it doesn't work... I could swear that it did (some time ago), so either they regressed or I'm going crazy. See: bazelbuild/bazel#1866
Anyway, please ignore this comment.
ci/do_ci.sh
Outdated
| bazel --batch test ${BAZEL_TEST_OPTIONS} -c opt --test_output=all \ | ||
| --cache_test_results=no @envoy//test/... //:echo2_integration_test | ||
| bazel --batch test ${BAZEL_TEST_OPTIONS} -c opt //test/... | ||
| rm -f bazel-* |
There was a problem hiding this comment.
Why is this needed? It kills incremental builds.
There was a problem hiding this comment.
It was a small nod to the notion that someone might run bazel.dev and then bazel.coverage. The annoying circular symlinks will then cause problems for bazel.coverage. Let me move this into the bazel.coverage target to avoid it hurting incremental builds.
There was a problem hiding this comment.
Actually, incremental builds work fine, since it's just removing symlinks, not purging cache.
You can leave it as is.
There was a problem hiding this comment.
but yeah, it might be better served inside bazel.coverage target, in case someone runs bazel.coverage twice in a row.
| ln -sf "${ENVOY_SRCDIR}"/tools/bazel.rc "${ENVOY_CONSUMER_SRCDIR}"/tools/ | ||
| ln -sf "${ENVOY_SRCDIR}"/tools/bazel.rc "${ENVOY_CI_DIR}"/tools/ | ||
| mkdir -p "${ENVOY_CONSUMER_SRCDIR}"/bazel | ||
| mkdir -p "${ENVOY_CI_DIR}"/bazel |
There was a problem hiding this comment.
Actually, one more thing.
Could you remove ci/{bazel,tools} directories? or if you don't think that's valid option, then add them to .gitignore? Otherwise tree isn't clean.
This is done. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
This is done. Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
This fixes an issue where the Envoy consumer git hash was picked up in the
release binary instead of Envoy proper.