diff mbox series

[2/2] CI: enable t1509 in Travis and GitHub Actions

Message ID fa2e1f7b44262eac1fa26161fc5d3f3b9b6bdb47.1589375923.git.congdanhqx@gmail.com (mailing list archive)
State New, archived
Headers show
Series CI: Enable t1509 on GitHub Actions and Travis | expand

Commit Message

Đoàn Trần Công Danh May 13, 2020, 1:23 p.m. UTC
Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 ci/lib.sh              | 13 +++++++++++++
 ci/run-docker-build.sh |  3 +++
 2 files changed, 16 insertions(+)

Comments

Junio C Hamano May 13, 2020, 4:51 p.m. UTC | #1
Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  ci/lib.sh              | 13 +++++++++++++
>  ci/run-docker-build.sh |  3 +++
>  2 files changed, 16 insertions(+)
>
> diff --git a/ci/lib.sh b/ci/lib.sh
> index dac36886e3..e9c22ae718 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -113,6 +113,7 @@ then
>  	export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
>  	export GIT_TEST_OPTS="--verbose-log -x --immediate"
>  	MAKEFLAGS="$MAKEFLAGS --jobs=2"
> +	t1509_allowed=YES
>  elif test -n "$SYSTEM_COLLECTIONURI" || test -n "$SYSTEM_TASKDEFINITIONSURI"
>  then
>  	CI_TYPE=azure-pipelines
> @@ -162,6 +163,7 @@ then
>  	echo "::add-matcher::ci/git-problem-matcher.json"
>  	test linux-musl = "$jobname" ||
>  	MAKEFLAGS="$MAKEFLAGS TEST_SHELL_PATH=/bin/sh"
> +	t1509_allowed=YES
>  else
>  	echo "Could not identify CI type" >&2
>  	env >&2
> @@ -184,6 +186,17 @@ export DEVELOPER=1
>  export DEFAULT_TEST_TARGET=prove
>  export GIT_TEST_CLONE_2GB=true
>  
> +if test "$t1509_allowed" = YES
> +then
> +	case "$jobname" in
> +	osx-*) ;;
> +	*)
> +		chmod a+w / || sudo chmod a+w / || true
> +		export IKNOWWHATIAMDOING=YES

Eeeww ;-)  This makes readers wonder where we did not enable the
test and why.  Perhaps throw in a matching

	t1509_allowed=NO

in the azure thing for completeness?

Also, do we want to give a more descriptive name than t1509 to the
variable, say, ROOT_WORK_TREE_TEST_ALLOWED?

> diff --git a/ci/run-docker-build.sh b/ci/run-docker-build.sh
> index 8d47a5fda3..026afe275a 100755
> --- a/ci/run-docker-build.sh
> +++ b/ci/run-docker-build.sh
> @@ -58,6 +58,8 @@ else
>  	test -n "$cache_dir" && chown -R $HOST_UID:$HOST_UID "$cache_dir"
>  fi
>  
> +chmod a+w /
> +
>  # Build and test
>  command $switch_cmd su -m -l $CI_USER -c "
>  	set -ex
> @@ -68,6 +70,7 @@ command $switch_cmd su -m -l $CI_USER -c "
>  	export GIT_TEST_CLONE_2GB='$GIT_TEST_CLONE_2GB'
>  	export MAKEFLAGS='$MAKEFLAGS'
>  	export cache_dir='$cache_dir'
> +	export IKNOWWHATIAMDOING=YES
>  	cd /usr/src/git
>  	test -n '$cache_dir' && ln -s '$cache_dir/.prove' t/.prove
>  	make

Big EWWWWWWwwww.  Do we need to do this for _all_ tests, not
selectively only while running t1509?  This makes me worried as a
test by mistake can easily corrupt the VM and invalidating the
tests; I know we get a fresh one every time, so there is no
permanent harm done by corrupting it, but having one fewer thing we
have to worry about is always better than having one more thing.
Đoàn Trần Công Danh May 13, 2020, 11:52 p.m. UTC | #2
On 2020-05-13 09:51:56-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
> > +if test "$t1509_allowed" = YES
> > +then
> > +	case "$jobname" in
> > +	osx-*) ;;
> > +	*)
> > +		chmod a+w / || sudo chmod a+w / || true
> > +		export IKNOWWHATIAMDOING=YES
> 
> Eeeww ;-)  This makes readers wonder where we did not enable the
> test and why.  Perhaps throw in a matching
> 
> 	t1509_allowed=NO
> 
> in the azure thing for completeness?

I was thinking about allowing people set it via environment variable
and check, but it seems too risky, now.

Perhaps, always reset it to NO before the checking for $CI_TYPE,
and enable it selectively for only Travis, and GitHub Actions.

I didn't enable it for Azure because I can't assure it ;).

> Also, do we want to give a more descriptive name than t1509 to the
> variable, say, ROOT_WORK_TREE_TEST_ALLOWED?

Yeah, I think all caps is better for this risky variable.
I think using T1509_ROOT_WORK_TREE_TEST_ALLOWED is better, to point
out which test is risky. But it require future tests with root
work-tree must be written in t1509, since it's rare usecase, It'd be
fine, I think.

> 
> > diff --git a/ci/run-docker-build.sh b/ci/run-docker-build.sh
> > index 8d47a5fda3..026afe275a 100755
> > --- a/ci/run-docker-build.sh
> > +++ b/ci/run-docker-build.sh
> > @@ -58,6 +58,8 @@ else
> >  	test -n "$cache_dir" && chown -R $HOST_UID:$HOST_UID "$cache_dir"
> >  fi
> >  
> > +chmod a+w /
> > +
> >  # Build and test
> >  command $switch_cmd su -m -l $CI_USER -c "
> >  	set -ex
> > @@ -68,6 +70,7 @@ command $switch_cmd su -m -l $CI_USER -c "
> >  	export GIT_TEST_CLONE_2GB='$GIT_TEST_CLONE_2GB'
> >  	export MAKEFLAGS='$MAKEFLAGS'
> >  	export cache_dir='$cache_dir'
> > +	export IKNOWWHATIAMDOING=YES
> >  	cd /usr/src/git
> >  	test -n '$cache_dir' && ln -s '$cache_dir/.prove' t/.prove
> >  	make
> 
> Big EWWWWWWwwww.  Do we need to do this for _all_ tests, not
> selectively only while running t1509?  This makes me worried as a
> test by mistake can easily corrupt the VM and invalidating the
> tests; I know we get a fresh one every time, so there is no
> permanent harm done by corrupting it, but having one fewer thing we
> have to worry about is always better than having one more thing.

Perhaps pass this variable all the way down from ci/lib.sh?
Adding another variable into t1509 (except T1509_*) doesn't make it
less risky.
Or should we add T1509_ prefix to this env var?
Junio C Hamano May 13, 2020, 11:59 p.m. UTC | #3
Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

>> >  
>> > +chmod a+w /
>> > +
>> >  # Build and test
>> >  command $switch_cmd su -m -l $CI_USER -c "
>> >  	set -ex
>> > @@ -68,6 +70,7 @@ command $switch_cmd su -m -l $CI_USER -c "
>> >  	export GIT_TEST_CLONE_2GB='$GIT_TEST_CLONE_2GB'
>> >  	export MAKEFLAGS='$MAKEFLAGS'
>> >  	export cache_dir='$cache_dir'
>> > +	export IKNOWWHATIAMDOING=YES
>> >  	cd /usr/src/git
>> >  	test -n '$cache_dir' && ln -s '$cache_dir/.prove' t/.prove
>> >  	make
>> 
>> Big EWWWWWWwwww.  Do we need to do this for _all_ tests, not
>> selectively only while running t1509?  This makes me worried as a
>> test by mistake can easily corrupt the VM and invalidating the
>> tests; I know we get a fresh one every time, so there is no
>> permanent harm done by corrupting it, but having one fewer thing we
>> have to worry about is always better than having one more thing.
>
> Perhaps pass this variable all the way down from ci/lib.sh?
> Adding another variable into t1509 (except T1509_*) doesn't make it
> less risky.
> Or should we add T1509_ prefix to this env var?

I was not worried about any environment variable, but the "let's
make the root directory writable by anybody during _all_ tests",
when we need such a crazy permission bits on the filesystem only
while running t1509 and not any other time, stood out as extremely
yucky.
diff mbox series

Patch

diff --git a/ci/lib.sh b/ci/lib.sh
index dac36886e3..e9c22ae718 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -113,6 +113,7 @@  then
 	export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
 	export GIT_TEST_OPTS="--verbose-log -x --immediate"
 	MAKEFLAGS="$MAKEFLAGS --jobs=2"
+	t1509_allowed=YES
 elif test -n "$SYSTEM_COLLECTIONURI" || test -n "$SYSTEM_TASKDEFINITIONSURI"
 then
 	CI_TYPE=azure-pipelines
@@ -162,6 +163,7 @@  then
 	echo "::add-matcher::ci/git-problem-matcher.json"
 	test linux-musl = "$jobname" ||
 	MAKEFLAGS="$MAKEFLAGS TEST_SHELL_PATH=/bin/sh"
+	t1509_allowed=YES
 else
 	echo "Could not identify CI type" >&2
 	env >&2
@@ -184,6 +186,17 @@  export DEVELOPER=1
 export DEFAULT_TEST_TARGET=prove
 export GIT_TEST_CLONE_2GB=true
 
+if test "$t1509_allowed" = YES
+then
+	case "$jobname" in
+	osx-*) ;;
+	*)
+		chmod a+w / || sudo chmod a+w / || true
+		export IKNOWWHATIAMDOING=YES
+		;;
+	esac
+fi
+
 case "$jobname" in
 linux-clang|linux-gcc)
 	if [ "$jobname" = linux-gcc ]
diff --git a/ci/run-docker-build.sh b/ci/run-docker-build.sh
index 8d47a5fda3..026afe275a 100755
--- a/ci/run-docker-build.sh
+++ b/ci/run-docker-build.sh
@@ -58,6 +58,8 @@  else
 	test -n "$cache_dir" && chown -R $HOST_UID:$HOST_UID "$cache_dir"
 fi
 
+chmod a+w /
+
 # Build and test
 command $switch_cmd su -m -l $CI_USER -c "
 	set -ex
@@ -68,6 +70,7 @@  command $switch_cmd su -m -l $CI_USER -c "
 	export GIT_TEST_CLONE_2GB='$GIT_TEST_CLONE_2GB'
 	export MAKEFLAGS='$MAKEFLAGS'
 	export cache_dir='$cache_dir'
+	export IKNOWWHATIAMDOING=YES
 	cd /usr/src/git
 	test -n '$cache_dir' && ln -s '$cache_dir/.prove' t/.prove
 	make