Test for working en_US.UTF-8 locale #108

mbargull · 2019-07-03T16:18:22Z

Do we really need the changes from gh-83 and gh-97?
It seems like CentOS/sig-cloud-instance-images#71 is a resolved issue and the locale is working, isn't it?
I'd rather like us to test whether the locale is working and only apply fixes if it really is not.
I couldn't find a reference to a recent issue in gh-83. @mariusvniekerk, @hmaarrfk, can you point me to failing cases such that we can incorporate them in the tests?

Also, @mariusvniekerk, gh-83 has been for aarch64 only. The only significant difference to ppc64le I could find is that aarch64 does not have CentOS/sig-cloud-instance-build@2892c17#diff-574fa022ad7d0be565da139e942a15cbR92 , i.e., it has override_install_langs=en_US.UTF-8 in /etc/yum.conf instead of en_US.utf8. If that is the culprit, we should change that (and upstream it, of course).

mbargull · 2019-07-03T16:21:11Z

.circleci/test_docker_container.sh

@@ -2,6 +2,12 @@

 set -xeuo pipefail

+# test for working en_US.UTF-8 locale
+locale -a | grep -i 'en_US.UTF.\?8'
+[ "$( LC_ALL=en_US.UTF-8 sh -euc : 2>&1 )" = "" ]


This would spit out

/usr/bin/sh: warning: setlocale: LC_ALL: cannot change locale (en_US.UTF-8): No such file or directory

on stderr if the locale wouldn't work. If it works, we just get no output and can test for that.

mbargull · 2019-07-03T18:35:52Z

The only significant difference to ppc64le I could find is that aarch64 does not have CentOS/sig-cloud-instance-build@2892c17#diff-574fa022ad7d0be565da139e942a15cbR92 , i.e., it has override_install_langs=en_US.UTF-8 in /etc/yum.conf instead of en_US.utf8. If that is the culprit, we should change that (and upstream it, of course).

Yep, that was is. All fixed now. I'll open an upstream PR.

mbargull · 2019-07-03T18:57:44Z

I'll open an upstream PR.

xref: CentOS/sig-cloud-instance-build#154

mbargull · 2019-07-03T19:51:08Z

linux-anvil-cuda/Dockerfile

@@ -38,7 +30,14 @@ RUN yum update -y && \
        sudo \
        tar \
        which && \
-    yum clean all
+    : '(centos:6 only) If glibc-common is updated, we have to manually purge non-en_US locales.' && \


Docker doesn't like real comments as part of a multi-line RUN so we have to work around that if we want them.
(Ab-)using the null utility (special built-in) : for this is a little hacky, TBH, but it really doesn't do any harm, so it is perfectly fine in my book.

Could we move this out into a bash script that we add and run to all Dockerfiles? This will ensure we only need to update and maintain this in one spot (as opposed to every Docker image), which will keep things maintainable here. Also we can format this more nicely without having to deal with Docker's syntax oddities. 🙂

Sure, I had the same thing in mind. I was just in the "meh, I'll likely refactor stuff later anyway when proposing multi-staged builds, so whatever" state 😉 .

Now it uses a common script in all images so that the rm -rf /var/cache/yum/* addition is only in one place, too.

mbargull · 2019-07-04T15:35:14Z

This is good to go, please take a look.
What this does:

add tests for working locale
properly fix the aarch64 locale issue (write override_install_langs=en_US.utf8 to /etc/yum.conf)
properly rebuilt locale archive on centos:6 (removes ~ 90 MiB)
clean caches more thoroughly (i.e., conda clean -i and rm -rf /var/cache/yum/*)
add missing GPG keys for RPM on aarch64 and ppc64le
show docker history when testing (can be used to inspect layer sizes if --squash is (temporarily) disabled)

hmaarrfk · 2019-07-04T15:38:23Z

.circleci/test_docker_container.sh

+locale -a | grep -i 'en_US.UTF.\?8'
+[ "$( LC_ALL=en_US.UTF-8 sh -euc : 2>&1 )" = "" ]
+# make sure the above fails for non-existent locale
+[ ! "$( LC_ALL=badlocale sh -euc : 2>&1 )" = "" ]


hmaarrfk · 2019-07-04T15:48:58Z

Looks pretty good, but I'm no expert :/

mbargull · 2019-07-04T15:51:00Z

Oh, and apart from testing/fixing the locale, this also shrinks the (squashed, uncompressed) -comp7 image by ~ 30 % (total 1.28 GB vs 1.82 GB total or 1.08 GB vs 1.63 GB for the layers we add).

mbargull · 2019-07-04T15:51:32Z

Thanks for the review @hmaarrfk!

mariusvniekerk · 2019-07-07T15:00:34Z

.circleci/test_docker_container.sh

@@ -2,6 +2,12 @@

 set -xeuo pipefail

+# test for working en_US.UTF-8 locale


Can we add some echo messages here so that is is clear in the logs what is actually going on here? Mostly just a line saying validate utf8 locale configured correctly.

(Also remove unused '-eu' in 'sh -c' call)

mbargull · 2019-07-07T15:17:39Z

.circleci/test_docker_container.sh

@@ -2,6 +2,12 @@

 set -xeuo pipefail

+echo 'Validating correctly configured en_US.UTF-8 locale...'


@mariusvniekerk, like so?

mariusvniekerk

LGTM

mbargull · 2019-07-08T16:35:41Z

scripts/yum_clean_all

+# (centos:6 only) If glibc-common is updated, we have to manually purge non-en_US locales.
+if grep -F 'CentOS release 6' /etc/centos-release ; then
+    if locale -a | grep nds_DE ; then
+        cp /usr/lib/locale/locale-archive /usr/lib/locale/locale-archive.tmpl


(cp instead of mv to avoid writing to the mmapped file later.)

Should we make that a comment in the script?

Nah, not really important. The only change I expect to happen to this code is to be removed whenever this isn't needed anymore, e.g., when we use Centos 7 everywhere.
I just added the comment for my future self if I ever wondered why I changed that.

mbargull · 2019-07-08T16:44:45Z

This would be ready to merge. However, Travis didn't run somehow... Any ideas?

mbargull · 2019-07-08T16:47:04Z

Travis is running. But it's not listed as one of the "checks" needed. Strange.

jakirkham · 2019-07-08T16:53:38Z

linux-anvil-ppc64le/Dockerfile

 RUN yum update -y && \
    yum install -y \
        bzip2 \
        sudo \
        tar \
        which && \
-    yum clean all && \
-    rm -rf /var/cache/yum/*
+    /opt/docker/bin/yum_clean_all


Maybe we should just move all of this into that script.

Yes, this and other parts can/should be consolidated. But let's do further cleanup.maintenance work later, not in this PR.

mbargull · 2019-07-08T17:10:16Z

Travis build finished.... and is listed as "Pending — The Travis CI build is in progress"... right... Well, ready to merge from my side.

mariusvniekerk · 2019-07-09T12:30:48Z

If you really want the full green orb treatment you may want to restart the Travis build. I don't care too much about that :)

mbargull · 2019-07-09T13:50:05Z

the full green orb treatment

Oh, I like many kinds of green hues -- olive green is a nice one; all good ;).
I was just waiting for John's nod. @jakirkham, you okay with the current state and postponing further cleanup and such?

mbargull added 2 commits July 3, 2019 18:02

Test for working en_US.UTF-8 locale

4c39234

Remove locale fixup

395f4f4

mbargull commented Jul 3, 2019

View reviewed changes

mbargull added 4 commits July 3, 2019 20:24

Properly fix locale for aarch64

b61d114

Reduce size by cleaning after broken locale-rebuilt on centos:6

d97ea20

Reduce size by nuking /var/cache/yum (also done in base images)

96c9255

Add missing GPG keys for aarch64 and ppc64le

4177041

mbargull force-pushed the test-locale branch from d511249 to 4177041 Compare July 3, 2019 18:32

mbargull marked this pull request as ready for review July 3, 2019 19:01

mbargull added 3 commits July 3, 2019 21:34

Reduce size by nuking /var/cache/yum (also done in base images)

1fbb83e

Clean conda index, not conda-build source-cache (deprecated)

f5a2686

Show docker image history when testing it

db9d4d2

mbargull commented Jul 3, 2019

View reviewed changes

mbargull requested review from jakirkham and mariusvniekerk July 4, 2019 15:35

hmaarrfk reviewed Jul 4, 2019

View reviewed changes

mariusvniekerk reviewed Jul 7, 2019

View reviewed changes

Add log message for locale test

4b2eb41

(Also remove unused '-eu' in 'sh -c' call)

mbargull commented Jul 7, 2019

View reviewed changes

mariusvniekerk approved these changes Jul 8, 2019

View reviewed changes

Move yum clean all to common script

c395f12

mbargull force-pushed the test-locale branch from 432872b to c395f12 Compare July 8, 2019 16:34

mbargull commented Jul 8, 2019

View reviewed changes

jakirkham reviewed Jul 8, 2019

View reviewed changes

Merge branch 'master' into test-locale

7b505cb

mbargull merged commit de11f43 into conda-forge:master Jul 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test for working en_US.UTF-8 locale #108

Test for working en_US.UTF-8 locale #108

mbargull commented Jul 3, 2019

mbargull Jul 3, 2019

mbargull commented Jul 3, 2019

mbargull commented Jul 3, 2019

mbargull Jul 3, 2019

jakirkham Jul 8, 2019

mbargull Jul 8, 2019

mbargull Jul 8, 2019

mbargull commented Jul 4, 2019

hmaarrfk Jul 4, 2019

hmaarrfk commented Jul 4, 2019

mbargull commented Jul 4, 2019

mbargull commented Jul 4, 2019

mariusvniekerk Jul 7, 2019

mbargull Jul 7, 2019

mariusvniekerk left a comment

mbargull Jul 8, 2019

jakirkham Jul 8, 2019

mbargull Jul 8, 2019

mbargull commented Jul 8, 2019

mbargull commented Jul 8, 2019

jakirkham Jul 8, 2019

mbargull Jul 8, 2019

mbargull commented Jul 8, 2019

mariusvniekerk commented Jul 9, 2019

mbargull commented Jul 9, 2019

		@@ -2,6 +2,12 @@

		set -xeuo pipefail

		# test for working en_US.UTF-8 locale

		@@ -2,6 +2,12 @@

		set -xeuo pipefail

		echo 'Validating correctly configured en_US.UTF-8 locale...'

Test for working en_US.UTF-8 locale #108

Test for working en_US.UTF-8 locale #108

Conversation

mbargull commented Jul 3, 2019

Choose a reason for hiding this comment

mbargull commented Jul 3, 2019

mbargull commented Jul 3, 2019

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

mbargull commented Jul 4, 2019

Choose a reason for hiding this comment

hmaarrfk commented Jul 4, 2019

mbargull commented Jul 4, 2019

mbargull commented Jul 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusvniekerk 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

mbargull commented Jul 8, 2019

mbargull commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbargull commented Jul 8, 2019

mariusvniekerk commented Jul 9, 2019

mbargull commented Jul 9, 2019