{"thread":{"id":"47714","subject":"[PATCHv2 0/5] Travis CI: don't run the test suite as root in the 32 bit Linux build","startedAt":"2018-01-29T17:17:29Z","lastAt":"2018-01-29T18:26:46Z","messageCount":7,"participants":["SZEDER Gábor","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"337670","messageId":"20180129171713.17471-1-szeder.dev@gmail.com","threadId":"47714","inReplyTo":null,"subject":"[PATCHv2 0/5] Travis CI: don't run the test suite as root in the 32 bit Linux build","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-29T17:17:08Z","receivedAt":"2018-01-29T17:17:29Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This version addresses Peff comments:\n\n  - Updated the commit message to discuss the broken && chain and its\n    consequences in patch 2.\n  - Fixed quoting of the 'su' command snippet and updated the commit\n    message in patch 3.\n  - Squelch error from 'test \"$(id -u $CI_USER)\"' and add debug\n    message in patch 5.\n\nAdditionally, it's rebased onto current master (I branched off v1 from\n'sg/travis-build-during-script-phase' to avoid a conflict, but Junio\napplied it on top of current master).\n\nInterdiff against 'sg/travis-linux32-sanity' at the end.\n\nSZEDER Gábor (5):\n  travis-ci: use 'set -x' for the commands under 'su' in the 32 bit\n    Linux build\n  travis-ci: use 'set -e' in the 32 bit Linux build job\n  travis-ci: don't repeat the path of the cache directory\n  travis-ci: don't run the test suite as root in the 32 bit Linux build\n  travis-ci: don't fail if user already exists on 32 bit Linux build job\n\n ci/lib-travisci.sh       |  7 ++++---\n ci/run-linux32-build.sh  | 51 ++++++++++++++++++++++++++++++++++++------------\n ci/run-linux32-docker.sh |  7 +++++--\n ci/run-tests.sh          |  3 ++-\n 4 files changed, 50 insertions(+), 18 deletions(-)\n\n-- \n2.16.1.158.ge6451079d\n\ndiff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh\nindex 13047adde..2c60d2e70 100755\n--- a/ci/run-linux32-build.sh\n+++ b/ci/run-linux32-build.sh\n@@ -33,9 +33,9 @@ then\n \tCI_USER=root\n else\n \tCI_USER=ci\n-\tif test \"$(id -u $CI_USER)\" = $HOST_UID\n+\tif test \"$(id -u $CI_USER 2>/dev/null)\" = $HOST_UID\n \tthen\n-\t\t: # user already exists with the right ID\n+\t\techo \"user '$CI_USER' already exists with the requested ID $HOST_UID\"\n \telse\n \t\tuseradd -u $HOST_UID $CI_USER\n \tfi\n@@ -51,10 +51,10 @@ else\n fi\n \n # Build and test\n-linux32 --32bit i386 su -m -l $CI_USER -c \"\n+linux32 --32bit i386 su -m -l $CI_USER -c '\n \tset -ex\n \tcd /usr/src/git\n-\ttest -n '$cache_dir' && ln -s '$cache_dir/.prove' t/.prove\n+\ttest -n \"$cache_dir\" && ln -s \"$cache_dir/.prove\" t/.prove\n \tmake --jobs=2\n \tmake --quiet test\n-\"\n+'\n"},{"id":"337671","messageId":"20180129171713.17471-2-szeder.dev@gmail.com","threadId":"47714","inReplyTo":"20180129171713.17471-1-szeder.dev@gmail.com","subject":"[PATCHv2 1/5] travis-ci: use 'set -x' for the commands under 'su' in the 32 bit Linux build","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-29T17:17:09Z","receivedAt":"2018-01-29T17:17:32Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n ci/run-linux32-build.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh\nindex c19c50c1c..5a36a8d7c 100755\n--- a/ci/run-linux32-build.sh\n+++ b/ci/run-linux32-build.sh\n@@ -26,6 +26,7 @@ test -z $HOST_UID || (CI_USER=\"ci\" && useradd -u $HOST_UID $CI_USER) &&\n \n # Build and test\n linux32 --32bit i386 su -m -l $CI_USER -c '\n+    set -x &&\n     cd /usr/src/git &&\n     ln -s /tmp/travis-cache/.prove t/.prove &&\n     make --jobs=2 &&\n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"337672","messageId":"20180129171713.17471-4-szeder.dev@gmail.com","threadId":"47714","inReplyTo":"20180129171713.17471-1-szeder.dev@gmail.com","subject":"[PATCHv2 3/5] travis-ci: don't repeat the path of the cache directory","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-29T17:17:11Z","receivedAt":"2018-01-29T17:17:33Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Some of our 'ci/*' scripts repeat the name or full path of the Travis\nCI cache directory, and the following patches will add new places\nusing that path.\n\nUse a variable to refer to the path of the cache directory instead, so\nit's hard-coded only in a single place.\n\nPay extra attention to the 32 bit Linux build: it runs in a Docker\ncontainer, so pass the path of the cache directory from the host to\nthe container in an environment variable.  Note that an environment\nvariable passed this way is exported inside the container, therefore\nits value is directly available in the 'su' snippet even though that\nsnippet is single quoted.  Furthermore, use the variable in the\ncontainer only if it's been assigned a non-empty value, to prevent\nerrors when someone is running or debugging the Docker build locally,\nbecause in that case the variable won't be set as there won't be any\nTravis CI cache.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n ci/lib-travisci.sh       | 7 ++++---\n ci/run-linux32-build.sh  | 2 +-\n ci/run-linux32-docker.sh | 5 ++++-\n ci/run-tests.sh          | 3 ++-\n 4 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh\nindex 07f27c727..1efee55ef 100755\n--- a/ci/lib-travisci.sh\n+++ b/ci/lib-travisci.sh\n@@ -21,8 +21,6 @@ skip_branch_tip_with_tag () {\n \tfi\n }\n \n-good_trees_file=\"$HOME/travis-cache/good-trees\"\n-\n # Save some info about the current commit's tree, so we can skip the build\n # job if we encounter the same tree again and can provide a useful info\n # message.\n@@ -83,7 +81,10 @@ check_unignored_build_artifacts ()\n # and installing dependencies.\n set -ex\n \n-mkdir -p \"$HOME/travis-cache\"\n+cache_dir=\"$HOME/travis-cache\"\n+good_trees_file=\"$cache_dir/good-trees\"\n+\n+mkdir -p \"$cache_dir\"\n \n skip_branch_tip_with_tag\n skip_good_tree\ndiff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh\nindex 248183982..d020b762c 100755\n--- a/ci/run-linux32-build.sh\n+++ b/ci/run-linux32-build.sh\n@@ -28,7 +28,7 @@ test -z $HOST_UID || (CI_USER=\"ci\" && useradd -u $HOST_UID $CI_USER)\n linux32 --32bit i386 su -m -l $CI_USER -c '\n \tset -ex\n \tcd /usr/src/git\n-\tln -s /tmp/travis-cache/.prove t/.prove\n+\ttest -n \"$cache_dir\" && ln -s \"$cache_dir/.prove\" t/.prove\n \tmake --jobs=2\n \tmake --quiet test\n '\ndiff --git a/ci/run-linux32-docker.sh b/ci/run-linux32-docker.sh\nindex 4f191c5bb..15288ea2c 100755\n--- a/ci/run-linux32-docker.sh\n+++ b/ci/run-linux32-docker.sh\n@@ -11,6 +11,8 @@ docker pull daald/ubuntu32:xenial\n # $ docker run -itv \"${PWD}:/usr/src/git\" --entrypoint /bin/bash daald/ubuntu32:xenial\n # root@container:/# /usr/src/git/ci/run-linux32-build.sh\n \n+container_cache_dir=/tmp/travis-cache\n+\n docker run \\\n \t--interactive \\\n \t--env DEVELOPER \\\n@@ -18,8 +20,9 @@ docker run \\\n \t--env GIT_PROVE_OPTS \\\n \t--env GIT_TEST_OPTS \\\n \t--env GIT_TEST_CLONE_2GB \\\n+\t--env cache_dir=\"$container_cache_dir\" \\\n \t--volume \"${PWD}:/usr/src/git\" \\\n-\t--volume \"${HOME}/travis-cache:/tmp/travis-cache\" \\\n+\t--volume \"$cache_dir:$container_cache_dir\" \\\n \tdaald/ubuntu32:xenial \\\n \t/usr/src/git/ci/run-linux32-build.sh $(id -u $USER)\n \ndiff --git a/ci/run-tests.sh b/ci/run-tests.sh\nindex 22355f009..deba73d9b 100755\n--- a/ci/run-tests.sh\n+++ b/ci/run-tests.sh\n@@ -5,7 +5,8 @@\n \n . ${0%/*}/lib-travisci.sh\n \n-ln -s $HOME/travis-cache/.prove t/.prove\n+ln -s \"$cache_dir/.prove\" t/.prove\n+\n make --quiet test\n \n check_unignored_build_artifacts\n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"337673","messageId":"20180129171713.17471-6-szeder.dev@gmail.com","threadId":"47714","inReplyTo":"20180129171713.17471-1-szeder.dev@gmail.com","subject":"[PATCHv2 5/5] travis-ci: don't fail if user already exists on 32 bit Linux build job","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-29T17:17:13Z","receivedAt":"2018-01-29T17:17:36Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The 32 bit Linux build job runs in a Docker container, which lends\nitself to running and debugging locally, too.  Especially during\ndebugging one usually doesn't want to start with a fresh container\nevery time, to save time spent on installing a bunch of dependencies.\nHowever, that doesn't work quite smootly, because the script running\nin the container always creates a new user, which then must be removed\nevery time before subsequent executions, or the build script fails.\n\nMake this process more convenient and don't try to create that user if\nit already exists and has the right user ID in the container, so\ndevelopers don't have to bother with running a 'userdel' each time\nbefore they run the build script.\n\nThe build job on Travis CI always starts with a fresh Docker\ncontainer, so this change doesn't make a difference there.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n ci/run-linux32-build.sh | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh\nindex 8c1b500e6..2c60d2e70 100755\n--- a/ci/run-linux32-build.sh\n+++ b/ci/run-linux32-build.sh\n@@ -33,7 +33,13 @@ then\n \tCI_USER=root\n else\n \tCI_USER=ci\n-\tuseradd -u $HOST_UID $CI_USER\n+\tif test \"$(id -u $CI_USER 2>/dev/null)\" = $HOST_UID\n+\tthen\n+\t\techo \"user '$CI_USER' already exists with the requested ID $HOST_UID\"\n+\telse\n+\t\tuseradd -u $HOST_UID $CI_USER\n+\tfi\n+\n \t# Due to a bug the test suite was run as root in the past, so\n \t# a prove state file created back then is only accessible by\n \t# root.  Now that bug is fixed, the test suite is run as a\n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"337674","messageId":"20180129171713.17471-3-szeder.dev@gmail.com","threadId":"47714","inReplyTo":"20180129171713.17471-1-szeder.dev@gmail.com","subject":"[PATCHv2 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-29T17:17:10Z","receivedAt":"2018-01-29T17:17:41Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The script 'ci/run-linux32-build.sh' running inside the Docker\ncontainer of the 32 bit Linux build job uses an && chain to break the\nbuild if one of the commands fails.  This is problematic for two\nreasons:\n\n  - The && chain is broken, because there is this in the middle:\n\n    test -z $HOST_UID || (CI_USER=\"ci\" && useradd -u $HOST_UID $CI_USER) &&\n\n    Luckily it is broken in a way that it didn't lead to false\n    successes.  If installing dependencies fails, then the rest of the\n    first && chain is skipped and execution resumes  after the ||\n    operator.  At that point $HOST_UID is still unset, causing\n    'useradd' to error out with \"invalid user ID 'ci'\", which in turn\n    causes the second && chain to abort the script and thus break the\n    build.\n\n  - All other 'ci/*' scripts use 'set -e' to break the build if one of\n    the commands fails.  This inconsistency among these scripts is\n    asking for trouble: I forgot about the && chain more than once\n    while working on this patch series.\n\nEnable 'set -e' for the whole script and for the commands executed\nunder 'su' as well.\n\nWhile touching every line in the 'su' command block anyway, change\ntheir indentation to use a tab instead of spaces.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n ci/run-linux32-build.sh | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh\nindex 5a36a8d7c..248183982 100755\n--- a/ci/run-linux32-build.sh\n+++ b/ci/run-linux32-build.sh\n@@ -6,29 +6,29 @@\n #   run-linux32-build.sh [host-user-id]\n #\n \n-set -x\n+set -ex\n \n # Update packages to the latest available versions\n linux32 --32bit i386 sh -c '\n     apt update >/dev/null &&\n     apt install -y build-essential libcurl4-openssl-dev libssl-dev \\\n \tlibexpat-dev gettext python >/dev/null\n-' &&\n+'\n \n # If this script runs inside a docker container, then all commands are\n # usually executed as root. Consequently, the host user might not be\n # able to access the test output files.\n # If a host user id is given, then create a user \"ci\" with the host user\n # id to make everything accessible to the host user.\n-HOST_UID=$1 &&\n-CI_USER=$USER &&\n-test -z $HOST_UID || (CI_USER=\"ci\" && useradd -u $HOST_UID $CI_USER) &&\n+HOST_UID=$1\n+CI_USER=$USER\n+test -z $HOST_UID || (CI_USER=\"ci\" && useradd -u $HOST_UID $CI_USER)\n \n # Build and test\n linux32 --32bit i386 su -m -l $CI_USER -c '\n-    set -x &&\n-    cd /usr/src/git &&\n-    ln -s /tmp/travis-cache/.prove t/.prove &&\n-    make --jobs=2 &&\n-    make --quiet test\n+\tset -ex\n+\tcd /usr/src/git\n+\tln -s /tmp/travis-cache/.prove t/.prove\n+\tmake --jobs=2\n+\tmake --quiet test\n '\n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"337675","messageId":"20180129171713.17471-5-szeder.dev@gmail.com","threadId":"47714","inReplyTo":"20180129171713.17471-1-szeder.dev@gmail.com","subject":"[PATCHv2 4/5] travis-ci: don't run the test suite as root in the 32 bit Linux build","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-01-29T17:17:12Z","receivedAt":"2018-01-29T17:17:44Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Travis CI runs the 32 bit Linux build job in a Docker container, where\nall commands are executed as root by default.  Therefore, ever since\nwe added this build job in 88dedd5e7 (Travis: also test on 32-bit\nLinux, 2017-03-05), we have a bit of code to create a user in the\ncontainer matching the ID of the host user and then to run the test\nsuite as this user.  Matching the host user ID is important, because\notherwise the host user would have no access to any files written by\nprocesses running in the container, notably the logs of failed tests\ncouldn't be included in the build job's trace log.\n\nAlas, this piece of code never worked, because it sets the variable\nholding the user name ($CI_USER) in a subshell, meaning it doesn't\nhave any effect by the time we get to the point to actually use the\nvariable to switch users with 'su'.  So all this time we were running\nthe test suite as root.\n\nReorganize that piece of code in 'ci/run-linux32-build.sh' a bit to\navoid that problematic subshell and to ensure that we switch to the\nright user.  Furthermore, make the script's optional host user ID\noption mandatory, so running the build accidentally as root will\nbecome harder when debugging locally.  If someone really wants to run\nthe test suite as root, whatever the reasons might be, it'll still be\npossible to do so by explicitly passing '0' as host user ID.\n\nFinally, one last catch: since commit 7e72cfcee (travis-ci: save prove\nstate for the 32 bit Linux build, 2017-12-27) the 'prove' test harness\nhas been writing its state to the Travis CI cache directory from\nwithin the Docker container while running as root.  After this patch\n'prove' will run as a regular user, so in future build jobs it won't\nbe able overwrite a previously written, still root-owned state file,\nresulting in build job failures.  To resolve this we should manually\ndelete caches containing such root-owned files, but that would be a\nhassle.  Instead, work this around by changing the owner of the whole\ncontents of the cache directory to the host user ID.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n ci/run-linux32-build.sh  | 30 +++++++++++++++++++++++++-----\n ci/run-linux32-docker.sh |  2 +-\n 2 files changed, 26 insertions(+), 6 deletions(-)\n\ndiff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh\nindex d020b762c..8c1b500e6 100755\n--- a/ci/run-linux32-build.sh\n+++ b/ci/run-linux32-build.sh\n@@ -3,11 +3,17 @@\n # Build and test Git in a 32-bit environment\n #\n # Usage:\n-#   run-linux32-build.sh [host-user-id]\n+#   run-linux32-build.sh <host-user-id>\n #\n \n set -ex\n \n+if test $# -ne 1 || test -z \"$1\"\n+then\n+\techo >&2 \"usage: run-linux32-build.sh <host-user-id>\"\n+\texit 1\n+fi\n+\n # Update packages to the latest available versions\n linux32 --32bit i386 sh -c '\n     apt update >/dev/null &&\n@@ -18,11 +24,25 @@ linux32 --32bit i386 sh -c '\n # If this script runs inside a docker container, then all commands are\n # usually executed as root. Consequently, the host user might not be\n # able to access the test output files.\n-# If a host user id is given, then create a user \"ci\" with the host user\n-# id to make everything accessible to the host user.\n+# If a non 0 host user id is given, then create a user \"ci\" with that\n+# user id to make everything accessible to the host user.\n HOST_UID=$1\n-CI_USER=$USER\n-test -z $HOST_UID || (CI_USER=\"ci\" && useradd -u $HOST_UID $CI_USER)\n+if test $HOST_UID -eq 0\n+then\n+\t# Just in case someone does want to run the test suite as root.\n+\tCI_USER=root\n+else\n+\tCI_USER=ci\n+\tuseradd -u $HOST_UID $CI_USER\n+\t# Due to a bug the test suite was run as root in the past, so\n+\t# a prove state file created back then is only accessible by\n+\t# root.  Now that bug is fixed, the test suite is run as a\n+\t# regular user, but the prove state file coming from Travis\n+\t# CI's cache might still be owned by root.\n+\t# Make sure that this user has rights to any cached files,\n+\t# including an existing prove state file.\n+\ttest -n \"$cache_dir\" && chown -R $HOST_UID:$HOST_UID \"$cache_dir\"\n+fi\n \n # Build and test\n linux32 --32bit i386 su -m -l $CI_USER -c '\ndiff --git a/ci/run-linux32-docker.sh b/ci/run-linux32-docker.sh\nindex 15288ea2c..21637903c 100755\n--- a/ci/run-linux32-docker.sh\n+++ b/ci/run-linux32-docker.sh\n@@ -9,7 +9,7 @@ docker pull daald/ubuntu32:xenial\n \n # Use the following command to debug the docker build locally:\n # $ docker run -itv \"${PWD}:/usr/src/git\" --entrypoint /bin/bash daald/ubuntu32:xenial\n-# root@container:/# /usr/src/git/ci/run-linux32-build.sh\n+# root@container:/# /usr/src/git/ci/run-linux32-build.sh <host-user-id>\n \n container_cache_dir=/tmp/travis-cache\n \n-- \n2.16.1.158.ge6451079d\n\n"},{"id":"337678","messageId":"20180129182316.GD4185@sigill.intra.peff.net","threadId":"47714","inReplyTo":"20180129171713.17471-1-szeder.dev@gmail.com","subject":"Re: [PATCHv2 0/5] Travis CI: don't run the test suite as root in the 32 bit Linux build","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-29T18:23:16Z","receivedAt":"2018-01-29T18:26:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 29, 2018 at 06:17:08PM +0100, SZEDER Gábor wrote:\n\n> This version addresses Peff comments:\n> \n>   - Updated the commit message to discuss the broken && chain and its\n>     consequences in patch 2.\n>   - Fixed quoting of the 'su' command snippet and updated the commit\n>     message in patch 3.\n>   - Squelch error from 'test \"$(id -u $CI_USER)\"' and add debug\n>     message in patch 5.\n> \n> Additionally, it's rebased onto current master (I branched off v1 from\n> 'sg/travis-build-during-script-phase' to avoid a conflict, but Junio\n> applied it on top of current master).\n\nThanks, this all looks good to me.\n\n-Peff\n"}]}