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

[Skyler Ferris]

    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