Skip to content

build: workaround for issues with en_US.UTF-8 locale. #15980

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.

Sign up for GitHub

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

Closed
PiotrSikora wants to merge 7 commits into from
Closed

build: workaround for issues with en_US.UTF-8 locale. #15980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5df5ec4
build: update envoy-build-tools to latest (2021-04-14).
PiotrSikora Apr 14, 2021
b306d52
review: undo LC_ALL=en_US.UTF-8, since it breaks sed.
PiotrSikora Apr 15, 2021
29d0cb7
review: undo LC_ALL=en_US.UTF-8 in attempt to fix docker load/save.
PiotrSikora Apr 15, 2021
6029376
review: install gcc needed to build one of the Python deps.
PiotrSikora Apr 15, 2021
314df90
review: install more deps.
PiotrSikora Apr 15, 2021
952e18c
review: revert changes needed by cython.
PiotrSikora Apr 15, 2021
5692bd9
Merge remote-tracking branch 'origin/main' into PiotrSikora/build-too…
PiotrSikora Apr 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ci/do_ci.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ function run_process_test_result() {

function run_ci_verify () {
echo "verify examples..."
docker load < "$ENVOY_DOCKER_BUILD_DIR/docker/envoy-docker-images.tar.xz"
LC_ALL=C docker load < "$ENVOY_DOCKER_BUILD_DIR/docker/envoy-docker-images.tar.xz"
Copy link
Copy Markdown
Member

@phlax phlax Apr 15, 2021

Choose a reason for hiding this comment

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

im struggling to understand how this breaks - weve been using the updated build image for some time

Copy link
Copy Markdown
Member

@phlax phlax Apr 15, 2021

Choose a reason for hiding this comment

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

also this is not run in the build image - so this overriding the locales in the CI (ie azure) image (afaict)

Copy link
Copy Markdown
Contributor Author

@PiotrSikora PiotrSikora Apr 15, 2021

Choose a reason for hiding this comment

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

Yes, it changes locale in the CI, not build image, so that byte sequences passed to docker load/save are interpreted correctly.

Previously, it resulted in EOF during docker loadwhen reading from pipe, perhaps it was a flake, but there was another fallout because of the UTF-8 change and I was running against the clock. Unfortunatelly, I cannot find that run in Azure Pipelines now, though.

Copy link
Copy Markdown
Member

@phlax phlax Apr 15, 2021

Choose a reason for hiding this comment

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

but docker load/save wasnt/isnt broken

_images=$(docker image list --format "{{.Repository}}")
while read -r line; do images+=("$line"); done \
<<< "$_images"
Expand Down
2 changes: 1 addition & 1 deletion ci/docker_ci.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ done
mkdir -p "${ENVOY_DOCKER_IMAGE_DIRECTORY}"
ENVOY_DOCKER_TAR="${ENVOY_DOCKER_IMAGE_DIRECTORY}/envoy-docker-images.tar.xz"
echo "Saving built images to ${ENVOY_DOCKER_TAR}."
docker save "${IMAGES_TO_SAVE[@]}" | xz -T0 -2 >"${ENVOY_DOCKER_TAR}"
LC_ALL=C docker save "${IMAGES_TO_SAVE[@]}" | LC_ALL=C xz -T0 -2 >"${ENVOY_DOCKER_TAR}"

# Only push images for main builds, release branch builds, and tag builds.
if [[ "${AZP_BRANCH}" != "${MAIN_BRANCH}" ]] &&
Expand Down
3 changes: 3 additions & 0 deletions test/exe/version_out_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

set -e -o pipefail

# Undo LC_ALL=en_US.UTF-8, since it breaks sed.
export LC_ALL=C
Copy link
Copy Markdown
Member

@phlax phlax Apr 15, 2021

Choose a reason for hiding this comment

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

how does it break sed ?

Copy link
Copy Markdown
Member

@phlax phlax Apr 15, 2021

Choose a reason for hiding this comment

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

i can see https://www.gnu.org/software/sed/manual/html_node/Locale-Considerations.html

but what is being seded here (afaict) is the version string which shouldnt contain any multibyte chars - which would imply the issue is more with the sed string - i might be wrong

Copy link
Copy Markdown
Contributor Author

@PiotrSikora PiotrSikora Apr 15, 2021

Choose a reason for hiding this comment

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

==================== Test output for //test/exe:version_out_test:
sed: -e expression #1, char 127: Invalid range end

See failed run: https://dev.azure.com/cncf/envoy/_build/results?buildId=71987&view=logs&j=5b1da4ae-91c5-53b4-2dce-4cdfc8f90e84&t=7ce850fe-192d-5106-1bd3-aa6c6dba22dc

Copy link
Copy Markdown
Member

@phlax phlax Apr 15, 2021

Choose a reason for hiding this comment

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

not sure - but afaict both the linux image and the build-tools can be updated without this change

it may be necessary for some other reason that im not understanding - but my reading of the source is that its using sed to change the verssion string - and that should not have any kind of utf-8 issues

Copy link
Copy Markdown
Member

@phlax phlax Apr 15, 2021

Choose a reason for hiding this comment

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

re this @PiotrSikora - i think - and i may be wrong - that the sed expression has an incorrect \ which is being interpreted as a utf-8 multibyte char but i really dont see how the change in the docker image recipe would affect this

Copy link
Copy Markdown
Member

@phlax phlax Apr 15, 2021

Choose a reason for hiding this comment

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

ie its unrelated to the change in envoy-build-tools - it was always there - and its only when one of the tests fails that it has this issue

Copy link
Copy Markdown
Contributor Author

@PiotrSikora PiotrSikora Apr 15, 2021

Choose a reason for hiding this comment

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

I mean, what can I say? See the CI failures from a few hours ago.

Also, the fact that updating Linux image alone works, and updating build-tools alone works, doesn't prove that both changes work fine together, does it? It's not like I was making all those changes for fun, they are direct results of failures on the CI.


ENVOY_BIN="${TEST_SRCDIR}/envoy/source/exe/envoy-static"

COMMIT=$(${ENVOY_BIN} --version | \
Expand Down