[bug#70001] [PATCH] gnu: reprotest: Add missing propogated inputs

[bug#70001] [PATCH] gnu: reprotest: Add missing propogated inputs

[Skyler Ferris via Guix-patches via]

Hello,

This patch adds some propagated inputs to the reprotest package so that 
most tests can run in a pure shell. I observed some issues running this 
package in a pure shell when reviewing issue 69991 (which updates 
diffoscope). Some of the new propagated inputs have already been listed 
as native inputs, but I did not remove them under the assumption that 
these programs are actually needed for the build machine. There are 3 
tests which still do not work in a pure shell.

The fileordering and user_group tests both require setuid programs. Both 
require fusermount and user_group requires sudo. I do not think it is 
possible to make these tests work within a pure shell because a package 
cannot provide setuid programs (only an operating system can).

The domain_host test does not work in a pure shell due to a dependency 
on a program named domainname. I was unable to locate a package which 
provides this command. The closest I could find is the inetutils package 
which provides dnsdomainname but I do not think this is sufficient for 
the test to run correctly. The domainname command is used in 
reprotest/build.py line 243:

```python
_ = _.prepend_to_build_command(*"unshare -r --uts".split(),
     "sh", "-ec", r"""
     hostname {1}
     domainname "{2}"
     """.format(build.aux_tree, hostname, domainname) + '"$@"', "-")
```

It looks to me like this is trying to set both the hostname and the 
domainname, which makes sense if it wants to test for these settings as 
a cause of non-determinism. The hostname command supports setting, but 
the dnsdomainname command only supports retrieving the domain name.

Additionally, this patch adds a new build phase to call fusermount3 
instead of fusermount, which is the name of the command provided by 
Guix's fuse package. This does not make it work in a pure shell but it 
does make it work in an operating system that provides the suid version 
of the binary.


Summary:

Tests working in pure shell:

- aslr
- build_path
- environment
- exec_path
- home
- kernel
- locales
- num_cpus
- time
- timezone
- umask

Tests work with appropriate suid programs provided:

- fileordering
- user_group

Tests that do not work on any Guix system:

- domain_host

To try reprotest in a pure shell, create the following makefile in an 
empty directory:

```make
destination:
     echo foo > destination

clean:
     rm -f destination
```

Then, run the following command to try the working tests. If you want to 
see the test report a problem you will need to change the recipe to be 
non-deterministic. For example, use `date` instead of `echo foo` in 
order to see the time test report a problem. Note that make is an 
additional package in the shell because this is the build system that 
happens to be used, but it would not make sense to add this as a 
mandatory input to the package because some people might use reprotest 
without using make. If you remove one of the propagated inputs then at 
least one of these tests will crash, with the exception of findutils and 
diffoscope which are used by reprotest outside the context of a specific 
test (to get the hashes of multiple files and display diffs to the user, 
respectively).

```shell
guix shell --pure reprotest make -- bash -c 'cd /tmp/test && reprotest 
"make clean && make" destination --variations="+environment, 
+build_path, +kernel, +aslr, +num_cpus, +time, +home, +locales, 
+exec_path, +timezone, +umask"'
```

You can try the suid-dependent tests with the same Makefile. The sudo 
and fuse-2 packages need to be installed; the default %setuid-programs 
variable includes the relevant binaries. Remove the `--pure` flag from 
the shell invocation and change the `--variations` flag in the reprotest 
variations to have the value `"+fileordering, +user_group"`.

Regards,
Skyler

[bug#70001] [PATCH] gnu: reprotest: Add missing propogated inputs

[Skyler Ferris via Guix-patches via]

From: Skyler Ferris <skyvine@protonmail.com>

* gnu/packages/diffoscope.scm (reprotest): Add bash, coreutils,
diffoscope, disorderfs, findutils, grep, inetutils, libfaketime,
and util-linux as propagated inputs.

Change-Id: Ibc113558032697be9048bebe54ba7678156667b9
---
 gnu/packages/diffoscope.scm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/gnu/packages/diffoscope.scm b/gnu/packages/diffoscope.scm
index 52f615f2cf..b87ed74874 100644
--- a/gnu/packages/diffoscope.scm
+++ b/gnu/packages/diffoscope.scm
@@ -29,12 +29,14 @@ (define-module (gnu packages diffoscope)
   #:use-module (gnu packages android)
   #:use-module (gnu packages backup)
   #:use-module (gnu packages base)
+  #:use-module (gnu packages bash)
   #:use-module (gnu packages bootloaders)
   #:use-module (gnu packages cdrom)
   #:use-module (gnu packages check)
   #:use-module (gnu packages compression)
   #:use-module (gnu packages cpio)
   #:use-module (gnu packages dbm)
+  #:use-module (gnu packages file-systems)
   #:use-module (gnu packages gettext)
   #:use-module (gnu packages ghostscript)
   #:use-module (gnu packages gnome)
@@ -258,6 +260,18 @@ (define-public reprotest
         (base32 "1992wlkil07fmj64lw3i7l16dgkkzphz0f932hbkkj9rlcamdwxd"))))
     (inputs
      (list python-debian python-distro python-libarchive-c python-rstr))
+    (propagated-inputs
+     (list
+       bash         ; the python code sets up shell scripts to be executed
+       coreutils    ; tests: locale, exec_path, home ("env")
+       diffoscope   ; used to display diffs when builds are not reproducible
+       disorderfs   ; test: fileordering, but fuse-2 and sudo must be installed as
+                    ; setuid programs as well.
+       findutils    ; __init__ uses "find" to get the hash of multiple files
+       grep         ; tests: kernel
+       inetutils    ; tests: domain_host ("hostname")
+       libfaketime  ; tests: time
+       util-linux)) ; tests: num_cpus ("taskset")
     (native-inputs
      `(("diffoscope" ,diffoscope)
        ("help2man" ,help2man)

base-commit: 76a3414a1bc500626a9feca013673f994eb51a34
-- 
2.41.0

[bug#70001] [PATCH] gnu: reprotest: Add missing propogated inputs

[Vagrant Cascadian]

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

On 2024-03-25, Skyler Ferris wrote:
> This patch adds some propagated inputs to the reprotest package so that 
> most tests can run in a pure shell.

Thanks for taking an interest in this!

My understanding is that propagated inputs are generally to be used very
sparingly, as they make it very difficult to install multiple packages
that might contain conflicting propagated inputs.

Most of the proposed inputs are largely optional dependencies of
reprotest, which in guix terms, generally the end-user is expected to
install the additional packages to enable desired optional features.

It might make sense to list the additional dependencies in the
description describing what features each enables?

Adding some of them to native-inputs might also make sense to enable
more tests, and give a clear place to write comments about each. Then
"guix shell --pure --development reprotest reprotest" would pull them in
and reprotest itself to be able to run the relevent tests as you had
tried.

If reprotest behaves quite badly without some of the in the working
environment, such as spitting out an ugly backtrace rather than
reporting what is missing, it should probably be fixed upstream (and
possibly patched in guix in the meantime).


> Additionally, this patch adds a new build phase to call fusermount3 
> instead of fusermount, which is the name of the command provided by 
> Guix's fuse package. This does not make it work in a pure shell but it 
> does make it work in an operating system that provides the suid version 
> of the binary.

It seems like this is missing from the patch, though I suspect it is
needed to behave correctly with things relying on fuse!


> You can try the suid-dependent tests with the same Makefile. The sudo 
> and fuse-2 packages need to be installed; the default %setuid-programs 
> variable includes the relevant binaries. Remove the `--pure` flag from 
> the shell invocation and change the `--variations` flag in the reprotest 
> variations to have the value `"+fileordering, +user_group"`.

Thanks for the detailed report ... this might be or should be covered by
upstream documentation ... although if there are guix-specific angles on
it, I am not sure where that should be documented.


So, in summary, I think there are some ideas to explore from your patch,
although it needs a bit of rethinking about implementation details.


live well,
  vagrant

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

[bug#70001] [PATCH] gnu: reprotest: Add missing propogated inputs

[Skyler Ferris via Guix-patches via]

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

Hi Vagrant,

Thanks for the feedback!

On 3/25/24 23:02, Vagrant Cascadian wrote:

> My understanding is that propagated inputs are generally to be used very
> sparingly, as they make it very difficult to install multiple packages
> that might contain conflicting propagated inputs.

Fair enough. I haven't personally run into this pain point, but that's probably because people are using them sparingly. =)

> Most of the proposed inputs are largely optional dependencies of
> reprotest, which in guix terms, generally the end-user is expected to
> install the additional packages to enable desired optional features.

I remember running into this before and it has been a pain point for me because it is not clear what the optional dependencies are. I know that package managers like apt have the ability to recommend packages which are optional dependencies, but the problem that I run into is that if I'm looking at a configuration (manifest, operating-system spec, etc) it's not clear to me which items are included as "invisible dependencies" of other packages and which ones are being included for their own sake. So I would like to propose using a different technique which I believe addresses both of our concerns. Basically, the technique is to embed absolute paths into the python code. For example, I tried adding this phase to reprotest (it would need some polishing before actual submission, for reasons mentioned later in this email): ```scheme (add-after 'adjust-locales 'set-taskset-path (lambda _ (define (quote-surround str) (string-append "'" str "'")) (let ((util-linux #$(this-package-input "util-linux"))) (when util-linux (substitute* "reprotest/build.py" (("'taskset'") (quote-surround (string-append util-linux "/bin/taskset"))) (("setarch") (string-append util-linux "/bin/setarch"))))))) ``` This let me run the num_cpus test from a pure shell without adding util-linux to my explicit inputs. Running taskset from the shell resulted in a "command not found" error so the package was not added to the profile. Additionally, I built a `guix pack` with only reprotest as an input, and util-linux was present. I built a pack with the current version of the package and util-linux was not present. So it seems that even though Guix sees the hash and is aware of it as a run-time dependency, it does not automatically add the package to the profile, addressing both of our concerns. If a user does not want an optional dependency then they can delete the input like so (or replace it for a different version, if so desired): ```scheme (use-modules (guix packages) (gnu packages diffoscope)) (package (inherit reprotest) (inputs (modify-inputs (package-inputs reprotest) (delete "util-linux")))) ``` Unfortunately I do not see a way to remove an input using the with-input option in options->transformation procedure or the analogous --with-input command-line flag. If we do go this route then I will look at adding "#f" as a valid value for the with-input transformation to support this case, but that will be handled in a separate issue. We could default to not including optional dependencies, but I don't think that this would be the most useful thing to do. I would expect that most people want all tests to work and that removing the inputs is a niche interest. It seems likely that "all of the tests work out of the box" is closer to what most people are currently experiencing because all of the packages which I added to the propagated inputs, with the exception of libfaketime and diffoscope, is in the %base-packages variable that Guix recommends people use (and the "normal" diff command is provided by %base-packages as well, so the lack of diffoscope would not cause a problem for users with the default configuration). If we do decide to exclude optional dependencies by default then I will look at adding an "add-input" option to the available transformations.
Doing this for every command used by reprotest would require some up-front work, and I'm not sure if it would be possible to uniquely identify every instance of the program names with regular expressions alone (for example, the above build phase replaces instances of setarch in docstrings which is undesirable). So I might end up adding a guix-specific patch that does something like surround each instance of the program name with double percent signs so that they are uniquely identifiable in the build phase. This might add some additional work when updating the package, because new lines that add commands would need to have the double percent signs added and changes to existing lines could cause conflicts with the patch. But I think that the majority of the work would be the up-front work which I am willing to do.

> It might make sense to list the additional dependencies in the
> description describing what features each enables?

I think this is a useful addition even with the above technique, so that users have awareness about the optional dependencies & how they can manipulate them.

> If reprotest behaves quite badly without some of the in the working
> environment, such as spitting out an ugly backtrace rather than
> reporting what is missing, it should probably be fixed upstream (and
> possibly patched in guix in the meantime).

I agree that this should be part of the revision even with the above technique, because if a user chooses to exclude an optional dependency without realizing a desired test depends on it then they will see the backtrace instead of a clean explanation (there is an explanation at the very beginning of the input, but it is buried by the backtrace). Additionally, the setuid packages need to be handled by the operating-system declaration no matter what we do here.

> It seems like this is missing from the patch, though I suspect it is
> needed to behave correctly with things relying on fuse!

Oops, I meant to delete that paragraph. I wrote it before finding out that there are separate fuse-2 and fuse packages. The fuse-2 package provides a binary named "fusermount", and reprotest used it successfully on my machine without this additional substitution.

> Thanks for the detailed report ... this might be or should be covered by
> upstream documentation ... although if there are guix-specific angles on
> it, I am not sure where that should be documented.

Assuming that we go with the above technique, I think that it would make sense to mention the ability to use package transformation options to control optional dependencies in the description of the reprotest package. In either case I think that the setuid requirements should be added to the description.

I appreciate any feedback you have!

Regards,
Skyler

[-- Attachment #2: Type: text/html, Size: 8745 bytes --]

[bug#70001] [PATCH] gnu: reprotest: Add missing propogated inputs

[Skyler Ferris via Guix-patches via]

Hi again, sorry, it seems like some of my paragraphs and code blocks got 
squashed into one giant paragraph somehow. I'm going to try this again, 
hopefully it works this time. This is the text beneath the "Most of the 
proposed inputs..." quote:

I remember running into this before and it has been a pain point for me 
because it is not clear what the optional dependencies are. I know that 
package managers like apt have the ability to recommend packages which 
are optional dependencies, but the problem that I run into is that if 
I'm looking at a configuration (manifest, operating-system spec, etc) 
it's not clear to me which items are included as "invisible 
dependencies" of other packages and which ones are being included for 
their own sake. So I would like to propose using a different technique 
which I believe addresses both of our concerns.

Basically, the technique is to embed absolute paths into the python 
code. For example, I tried adding this phase to reprotest (it would need 
some polishing before actual submission, for reasons mentioned later in 
this email):

```scheme
(add-after 'adjust-locales 'set-taskset-path
   (lambda _
     (define (quote-surround str)
       (string-append "'" str "'"))

     (let ((util-linux #$(this-package-input "util-linux")))
       (when util-linux
         (substitute* "reprotest/build.py"
           (("'taskset'")
            (quote-surround (string-append util-linux "/bin/taskset")))
           (("setarch")
            (string-append util-linux "/bin/setarch")))))))
```

This let me run the num_cpus test from a pure shell without adding 
util-linux to my explicit inputs. Running taskset from the shell 
resulted in a "command not found" error so the package was not added to 
the profile. Additionally, I built a `guix pack` with only reprotest as 
an input, and util-linux was present. I built a pack with the current 
version of the package and util-linux was not present. So it seems that 
even though Guix sees the hash and is aware of it as a run-time 
dependency, it does not automatically add the package to the profile, 
addressing both of our concerns.

If a user does not want an optional dependency then they can delete the 
input like so (or replace it for a different version, if so desired):

```scheme
(use-modules (guix packages) (gnu packages diffoscope))

(package
     (inherit reprotest)
     (inputs (modify-inputs (package-inputs reprotest)
                            (delete "util-linux"))))
```

Unfortunately I do not see a way to remove an input using the with-input 
option in options->transformation procedure or the analogous 
--with-input command-line flag. If we do go this route then I will look 
at adding "#f" as a valid value for the with-input transformation to 
support this case, but that will be handled in a separate issue.

We could default to not including optional dependencies, but I don't 
think that this would be the most useful thing to do. I would expect 
that most people want all tests to work and that removing the inputs is 
a niche interest. It seems likely that "all of the tests work out of the 
box" is closer to what most people are currently experiencing because 
all of the packages which I added to the propagated inputs, with the 
exception of libfaketime and diffoscope, is in the %base-packages 
variable that Guix recommends people use (and the "normal" diff command 
is provided by %base-packages as well, so the lack of diffoscope would 
not cause a problem for users with the default configuration). If we do 
decide to exclude optional dependencies by default then I will look at 
adding an "add-input" option to the available transformations.