bug#44491: Support GUIX_DISABLE_NETWORK_TESTS environment variable

bug#44491: Support GUIX_DISABLE_NETWORK_TESTS environment variable

[Vagrant Cascadian]

[-- Attachment #1.1: Type: text/plain, Size: 470 bytes --]

The following patch adds a GUIX_DISABLE_NETWORK_TESTS environment
variable and disables tests that require network access when it is set.

This is needed for packaging GNU Guix in Debian, where packaging policies
prohibit network access during builds, but may not technically block network
access during builds.

If this could be considered for the upcoming 1.2 release, that would be
appreciated, though I can also carry the patches in Debian...


live well,
  vagrant

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-tests-Support-disabling-network-tests.patch --]
[-- Type: text/x-diff, Size: 4484 bytes --]

From 36516e767f68dbc2bd3dc186f956c0b0fd7de9f1 Mon Sep 17 00:00:00 2001
From: Vagrant Cascadian <vagrant@debian.org>
Date: Thu, 5 Nov 2020 17:35:52 -0800
Subject: [PATCH] tests: Support disabling network tests.

This is needed for packaging GNU Guix in Debian, where packaging policies
prohibit network access during builds, but may not technically block network
access during builds.

* guix/tests.scm (network-reachable): Return #f when
  GUIX_DISABLE_NETWORK_TESTS is set.
* tests/common.sh: New file.
* tests/guix-build-branch.sh, tests/guix-environment.sh,
  tests/guix-pack.sh, tests/guix-package-net.sh: Use
  network_reachable function from common.sh.
---
 guix/tests.scm             | 7 +++++--
 tests/common.sh            | 8 ++++++++
 tests/guix-build-branch.sh | 8 ++------
 tests/guix-environment.sh  | 5 ++---
 tests/guix-pack.sh         | 5 ++---
 tests/guix-package-net.sh  | 9 ++-------
 6 files changed, 21 insertions(+), 21 deletions(-)
 create mode 100644 tests/common.sh

diff --git a/guix/tests.scm b/guix/tests.scm
index fc3d521163..9f98cef33f 100644
--- a/guix/tests.scm
+++ b/guix/tests.scm
@@ -204,8 +204,11 @@ too expensive to build entirely in the test store."
              (zero? (logand #o222 (stat:mode st)))))))
 
 (define (network-reachable?)
-  "Return true if we can reach the Internet."
-  (false-if-exception (getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)))
+  "Return true if we can reach the Internet and GUIX_DISABLE_NETWORK_TESTS
+is not set."
+  (if (getenv "GUIX_DISABLE_NETWORK_TESTS")
+      #f
+      (false-if-exception (getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV))))
 
 (define-syntax-rule (mock (module proc replacement) body ...)
   "Within BODY, replace the definition of PROC from MODULE with the definition
diff --git a/tests/common.sh b/tests/common.sh
new file mode 100644
index 0000000000..b91c0bdcd4
--- /dev/null
+++ b/tests/common.sh
@@ -0,0 +1,8 @@
+network_reachable() {
+    if [ -n "$GUIX_DISABLE_NETWORK_TESTS" ]; then
+		exit 77
+    fi
+    if ! guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null; then
+        exit 77
+    fi
+}
diff --git a/tests/guix-build-branch.sh b/tests/guix-build-branch.sh
index 79aa06a58f..55f8f388ab 100644
--- a/tests/guix-build-branch.sh
+++ b/tests/guix-build-branch.sh
@@ -24,12 +24,8 @@ guix build --version
 
 # 'guix build --with-branch' requires access to the network to clone the
 # Git repository below.
-
-if ! guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null
-then
-    # Skipping.
-    exit 77
-fi
+. $(dirname $0)/common.sh
+network_reachable
 
 orig_drv="`guix build guile-gcrypt -d`"
 latest_drv="`guix build guile-gcrypt --with-branch=guile-gcrypt=master -d`"
diff --git a/tests/guix-environment.sh b/tests/guix-environment.sh
index f8be48f0c0..d140566aef 100644
--- a/tests/guix-environment.sh
+++ b/tests/guix-environment.sh
@@ -174,9 +174,9 @@ case "$transformed_drv" in
     *)           false;;
 esac
 
+. $(dirname $0)/common.sh
+network_reachable
 
-if guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null
-then
     # Compute the build environment for the initial GNU Make.
     guix environment --bootstrap --no-substitutes --search-paths --pure \
          -e '(@ (guix tests) gnu-make-for-tests)' > "$tmpdir/a"
@@ -244,4 +244,3 @@ then
     do
 	guix gc --references "$profile" | grep "$dep"
     done
-fi
diff --git a/tests/guix-pack.sh b/tests/guix-pack.sh
index 0339221ac2..bc902c7e90 100644
--- a/tests/guix-pack.sh
+++ b/tests/guix-pack.sh
@@ -23,9 +23,8 @@
 
 # A network connection is required to build %bootstrap-coreutils&co,
 # which is required to run these tests with the --bootstrap option.
-if ! guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null; then
-    exit 77
-fi
+. $(dirname $0)/common.sh
+network_reachable
 
 guix pack --version
 
diff --git a/tests/guix-package-net.sh b/tests/guix-package-net.sh
index 6d21c6cff6..ec7952f63d 100644
--- a/tests/guix-package-net.sh
+++ b/tests/guix-package-net.sh
@@ -38,13 +38,8 @@ shebang_too_long ()
 	 -ge 128
 }
 
-if ! guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null \
-	|| shebang_too_long
-then
-    # Skipping.
-    exit 77
-fi
-
+. $(dirname $0)/common.sh
+network_reachable
 
 profile="t-profile-$$"
 profile_alt="t-profile-alt-$$"
-- 
2.20.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

bug#44491: Support GUIX_DISABLE_NETWORK_TESTS environment variable

[Ludovic Courtès]

Hi Vagrant,

Vagrant Cascadian <vagrant@debian.org> skribis:

> If this could be considered for the upcoming 1.2 release, that would be
> appreciated, though I can also carry the patches in Debian...

Yay!  It should be doable, let’s see.

> From 36516e767f68dbc2bd3dc186f956c0b0fd7de9f1 Mon Sep 17 00:00:00 2001
> From: Vagrant Cascadian <vagrant@debian.org>
> Date: Thu, 5 Nov 2020 17:35:52 -0800
> Subject: [PATCH] tests: Support disabling network tests.
>
> This is needed for packaging GNU Guix in Debian, where packaging policies
> prohibit network access during builds, but may not technically block network
> access during builds.
>
> * guix/tests.scm (network-reachable): Return #f when
>   GUIX_DISABLE_NETWORK_TESTS is set.
> * tests/common.sh: New file.
> * tests/guix-build-branch.sh, tests/guix-environment.sh,
>   tests/guix-pack.sh, tests/guix-package-net.sh: Use
>   network_reachable function from common.sh.

[...]

> --- /dev/null
> +++ b/tests/common.sh
> @@ -0,0 +1,8 @@
> +network_reachable() {
> +    if [ -n "$GUIX_DISABLE_NETWORK_TESTS" ]; then
> +		exit 77
> +    fi
> +    if ! guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null; then
> +        exit 77
> +    fi
> +}

Could you add the usual copyright/license header?  Also please add this
file to ‘EXTRA_DIST’ in Makefile.am.

Looking at the tests you modified, we need two things:

  • a ‘network_reachable’ function that returns true or false, without
    exiting;

  • a ‘skip_if_network_is_unreachable’ function that does “exit 77” when
    network is “unreachable”.

> --- a/tests/guix-environment.sh
> +++ b/tests/guix-environment.sh
> @@ -174,9 +174,9 @@ case "$transformed_drv" in
>      *)           false;;
>  esac
>  
> +. $(dirname $0)/common.sh
> +network_reachable
>  
> -if guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null
> -then
>      # Compute the build environment for the initial GNU Make.
>      guix environment --bootstrap --no-substitutes --search-paths --pure \

I think this is the only place where you’d write “if network_reachable”
instead of “skip_if_network_is_unreachable”.

WDYT?

Thanks!

Ludo’.

bug#44491: Support GUIX_DISABLE_NETWORK_TESTS environment variable

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 885 bytes --]

On 2020-11-08, Ludovic Courtès wrote:
> Vagrant Cascadian <vagrant@debian.org> skribis:
>
>> If this could be considered for the upcoming 1.2 release, that would be
>> appreciated, though I can also carry the patches in Debian...
>
> Yay!  It should be doable, let’s see.

It seems like a simpler workaround is to pass RES_OPTIONS=attempts:0,
which should disable name resolution, and thus the network checks will
fail.

With the RES_OPTIONS workaround, the changes to guix/tests.scm
network-reachable are no longer needed ... i think. :)

Might still be worth refactoring some of *.sh tests to use common
functions, since the code is basically copied and pasted in several
different places.

I suspect there are some additional tests that should check for network,
but that is probably a separate bug once they have been identified.


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

bug#44491: Support GUIX_DISABLE_NETWORK_TESTS environment variable

[Ludovic Courtès]

Vagrant Cascadian <vagrant@debian.org> skribis:

> On 2020-11-08, Ludovic Courtès wrote:
>> Vagrant Cascadian <vagrant@debian.org> skribis:
>>
>>> If this could be considered for the upcoming 1.2 release, that would be
>>> appreciated, though I can also carry the patches in Debian...
>>
>> Yay!  It should be doable, let’s see.
>
> It seems like a simpler workaround is to pass RES_OPTIONS=attempts:0,
> which should disable name resolution, and thus the network checks will
> fail.
>
> With the RES_OPTIONS workaround, the changes to guix/tests.scm
> network-reachable are no longer needed ... i think. :)

Oooh nice, the wonders of glibc!

> Might still be worth refactoring some of *.sh tests to use common
> functions, since the code is basically copied and pasted in several
> different places.

Yes, that’s still a good idea.  Would you like to adjust your patch
accordingly?

> I suspect there are some additional tests that should check for network,
> but that is probably a separate bug once they have been identified.

OK!

Thanks,
Ludo’.

bug#44491: Support GUIX_DISABLE_NETWORK_TESTS environment variable

[Vagrant Cascadian]

[-- Attachment #1.1: Type: text/plain, Size: 1331 bytes --]

On 2020-11-10, Ludovic Courtès wrote:
> Vagrant Cascadian <vagrant@debian.org> skribis:
>> On 2020-11-08, Ludovic Courtès wrote:
>>> Vagrant Cascadian <vagrant@debian.org> skribis:
>>>
>>>> If this could be considered for the upcoming 1.2 release, that would be
>>>> appreciated, though I can also carry the patches in Debian...
>>>
>>> Yay!  It should be doable, let’s see.
>>
>> It seems like a simpler workaround is to pass RES_OPTIONS=attempts:0,
>> which should disable name resolution, and thus the network checks will
>> fail.
>>
>> With the RES_OPTIONS workaround, the changes to guix/tests.scm
>> network-reachable are no longer needed ... i think. :)
>
> Oooh nice, the wonders of glibc!
>
>> Might still be worth refactoring some of *.sh tests to use common
>> functions, since the code is basically copied and pasted in several
>> different places.
>
> Yes, that’s still a good idea.  Would you like to adjust your patch
> accordingly?

Thanks for the review!

Updated patch attached, with changes:

* Copyright header added to common.sh.
* New function skip_if_network_unreachable in common.sh
* Dropped GUIX_DISABLE_NETWORK_TESTS in favor of using
  RES_OPTIONS=attempts:0.
* Updated tests to use skip_if_network_unreachable or network_reachable.


live well,
  vagrant


[-- Attachment #1.2: 0001-tests-Add-common-functions-for-to-check-for-network-.patch --]
[-- Type: text/x-diff, Size: 4731 bytes --]

From 3fd93fec957491450639c647e05a5f72f1e787fd Mon Sep 17 00:00:00 2001
From: Vagrant Cascadian <vagrant@debian.org>
Date: Tue, 10 Nov 2020 19:26:04 -0800
Subject: [PATCH] tests: Add common functions for to check for network
 reachability.

* tests/common.sh: New file.
* tests/guix-build-branch.sh, tests/guix-pack.sh,
  tests/guix-package-net.sh: Use skip_if_network_unreachable function
  from common.sh.
* tests/guix-environment.sh: Use network_reachable function from
  common.sh.
---
 Makefile.am                |  1 +
 tests/common.sh            | 30 ++++++++++++++++++++++++++++++
 tests/guix-build-branch.sh |  8 ++------
 tests/guix-environment.sh  |  5 ++---
 tests/guix-pack.sh         |  5 ++---
 tests/guix-package-net.sh  |  9 ++-------
 6 files changed, 39 insertions(+), 19 deletions(-)
 create mode 100644 tests/common.sh

diff --git a/Makefile.am b/Makefile.am
index e7053ee4f4..7dbe41201c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -599,6 +599,7 @@ EXTRA_DIST +=						\
   tests/test.drv					\
   tests/signing-key.pub					\
   tests/signing-key.sec					\
+  tests/common.sh					\
   tests/cve-sample.json					\
   tests/civodul.key					\
   tests/rsa.key						\
diff --git a/tests/common.sh b/tests/common.sh
new file mode 100644
index 0000000000..f9dd3c2c59
--- /dev/null
+++ b/tests/common.sh
@@ -0,0 +1,30 @@
+# GNU Guix --- Functional package management for GNU
+# Copyright © 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
+# Copyright © 2020 Vagrant Cascadian <vagrant@debian.org>
+#
+# This file is part of GNU Guix.
+#
+# GNU Guix is free software; you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or (at
+# your option) any later version.
+#
+# GNU Guix is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
+
+network_reachable() {
+    if ! guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null; then
+        return 0
+    fi
+}
+
+skip_if_network_unreachable() {
+	if ! network_reachable ; then
+		exit 77
+	fi
+}
diff --git a/tests/guix-build-branch.sh b/tests/guix-build-branch.sh
index 79aa06a58f..89f7ea4a08 100644
--- a/tests/guix-build-branch.sh
+++ b/tests/guix-build-branch.sh
@@ -24,12 +24,8 @@ guix build --version
 
 # 'guix build --with-branch' requires access to the network to clone the
 # Git repository below.
-
-if ! guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null
-then
-    # Skipping.
-    exit 77
-fi
+. $(dirname $0)/common.sh
+skip_if_network_unreachable
 
 orig_drv="`guix build guile-gcrypt -d`"
 latest_drv="`guix build guile-gcrypt --with-branch=guile-gcrypt=master -d`"
diff --git a/tests/guix-environment.sh b/tests/guix-environment.sh
index f8be48f0c0..ce01ac04be 100644
--- a/tests/guix-environment.sh
+++ b/tests/guix-environment.sh
@@ -174,9 +174,8 @@ case "$transformed_drv" in
     *)           false;;
 esac
 
-
-if guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null
-then
+. $(dirname $0)/common.sh
+if network_reachable ; then
     # Compute the build environment for the initial GNU Make.
     guix environment --bootstrap --no-substitutes --search-paths --pure \
          -e '(@ (guix tests) gnu-make-for-tests)' > "$tmpdir/a"
diff --git a/tests/guix-pack.sh b/tests/guix-pack.sh
index 0339221ac2..822a67b157 100644
--- a/tests/guix-pack.sh
+++ b/tests/guix-pack.sh
@@ -23,9 +23,8 @@
 
 # A network connection is required to build %bootstrap-coreutils&co,
 # which is required to run these tests with the --bootstrap option.
-if ! guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null; then
-    exit 77
-fi
+. $(dirname $0)/common.sh
+skip_if_network_unreachable
 
 guix pack --version
 
diff --git a/tests/guix-package-net.sh b/tests/guix-package-net.sh
index 6d21c6cff6..f682d09245 100644
--- a/tests/guix-package-net.sh
+++ b/tests/guix-package-net.sh
@@ -38,13 +38,8 @@ shebang_too_long ()
 	 -ge 128
 }
 
-if ! guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null \
-	|| shebang_too_long
-then
-    # Skipping.
-    exit 77
-fi
-
+. $(dirname $0)/common.sh
+skip_if_network_unreachable
 
 profile="t-profile-$$"
 profile_alt="t-profile-alt-$$"
-- 
2.20.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

bug#44491: Support GUIX_DISABLE_NETWORK_TESTS environment variable

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]

On 2020-11-10, Vagrant Cascadian wrote:
> On 2020-11-10, Ludovic Courtès wrote:
>> Vagrant Cascadian <vagrant@debian.org> skribis:
>>> On 2020-11-08, Ludovic Courtès wrote:
>>>> Vagrant Cascadian <vagrant@debian.org> skribis:
>>>>
>>>>> If this could be considered for the upcoming 1.2 release, that would be
>>>>> appreciated, though I can also carry the patches in Debian...
>>>>
>>>> Yay!  It should be doable, let’s see.
>>>
>>> It seems like a simpler workaround is to pass RES_OPTIONS=attempts:0,
>>> which should disable name resolution, and thus the network checks will
>>> fail.
>>>
>>> With the RES_OPTIONS workaround, the changes to guix/tests.scm
>>> network-reachable are no longer needed ... i think. :)
>>
>> Oooh nice, the wonders of glibc!
>>
>>> Might still be worth refactoring some of *.sh tests to use common
>>> functions, since the code is basically copied and pasted in several
>>> different places.
>>
>> Yes, that’s still a good idea.  Would you like to adjust your patch
>> accordingly?
>
> Thanks for the review!
>
> Updated patch attached, with changes:
>
> * Copyright header added to common.sh.
> * New function skip_if_network_unreachable in common.sh
> * Dropped GUIX_DISABLE_NETWORK_TESTS in favor of using
>   RES_OPTIONS=attempts:0.
> * Updated tests to use skip_if_network_unreachable or network_reachable.

...

> diff --git a/tests/common.sh b/tests/common.sh
> new file mode 100644
> index 0000000000..f9dd3c2c59
> --- /dev/null
> +++ b/tests/common.sh
...
> +network_reachable() {
> +    if ! guile -c '(getaddrinfo "www.gnu.org" "80" AI_NUMERICSERV)' 2> /dev/null; then
> +        return 0
> +    fi
> +}

Ooops. I inverted that check... probably "if guile -c ..." and probably
should return 1 or something if it isn't... or maybe 77?


anyways... testing again.


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

bug#44491: Support GUIX_DISABLE_NETWORK_TESTS environment variable

[Ludovic Courtès]

Hi Vagrant,

Should we close this issue now that you found the RES_OPTIONS=attempts:0
trick, or do you think we should still keep the refactoring bits?

Ludo’.

bug#44491: Support GUIX_DISABLE_NETWORK_TESTS environment variable

[Vagrant Cascadian]

[-- Attachment #1: Type: text/plain, Size: 789 bytes --]

On 2020-12-03, Ludovic Courtès wrote:
> Should we close this issue now that you found the RES_OPTIONS=attempts:0
> trick, or do you think we should still keep the refactoring bits?

Well, it's three cases of copy-paste code, and one nearly identical but
inverted. Someone once suggested to me to refactor on the third instance
of copy-pasted code...

Having common tests makes it a little easier to add to new tests in the
future with the same code, and if there's ever a change in the
underlying code, you fix it in once place. It also opens the door to
adding other common functions, if it comes up.

So I'd say it's worth applying, though also would be ok with leaving as
is and just taking advantage of the RES_OPTIONS=attempts:0 workaround.


live well,
  vagrant

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

bug#44491: Support GUIX_DISABLE_NETWORK_TESTS environment variable

[Ludovic Courtès]

Hi!

Vagrant Cascadian <vagrant@debian.org> skribis:

> On 2020-12-03, Ludovic Courtès wrote:
>> Should we close this issue now that you found the RES_OPTIONS=attempts:0
>> trick, or do you think we should still keep the refactoring bits?
>
> Well, it's three cases of copy-paste code, and one nearly identical but
> inverted. Someone once suggested to me to refactor on the third instance
> of copy-pasted code...
>
> Having common tests makes it a little easier to add to new tests in the
> future with the same code, and if there's ever a change in the
> underlying code, you fix it in once place. It also opens the door to
> adding other common functions, if it comes up.
>
> So I'd say it's worth applying, though also would be ok with leaving as
> is and just taking advantage of the RES_OPTIONS=attempts:0 workaround.

Makes sense to me.  I’ll let you push it or let me know if you prefer me
to do it.

Thanks!

Ludo’.