[bug#60429,v2,4/5] gnu: yosys: Propagate external dependencies.

* gnu/packages/fpga.scm (yosys)[inputs]: Move graphviz, psmisc, xdot from
here...
[propagated-inputs]: ...to here, to ensure the availability at runtime of
executables invoked by yosys' "show" command.
[arguments]: Remove now-obsolete "fix-paths" phase.
---
 gnu/packages/fpga.scm | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

Simon South <simon@simonsouth.net> writes:

> * gnu/packages/fpga.scm (yosys)[inputs]: Move graphviz, psmisc, xdot from
> here...
> [propagated-inputs]: ...to here, to ensure the availability at runtime of
> executables invoked by yosys' "show" command.
> [arguments]: Remove now-obsolete "fix-paths" phase.
> ---
>  gnu/packages/fpga.scm | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/gnu/packages/fpga.scm b/gnu/packages/fpga.scm
> index 8effebd921..785d385621 100644
> --- a/gnu/packages/fpga.scm
> +++ b/gnu/packages/fpga.scm
> @@ -157,16 +157,6 @@ (define-public yosys
>                             (string-append "PREFIX=" #$output))
>        #:phases
>        #~(modify-phases %standard-phases
> -          (add-before 'configure 'fix-paths
> -            (lambda* (#:key inputs #:allow-other-keys)
> -              (substitute* "./passes/cmds/show.cc"
> -                (("exec xdot")
> -                 (string-append "exec " (search-input-file inputs
> -                                                           "/bin/xdot")))
> -                (("dot -")
> -                 (string-append (search-input-file inputs "/bin/dot") " -"))
> -                (("fuser")
> -                 (search-input-file inputs "/bin/fuser")))))
>            (replace 'configure
>              (lambda* (#:key make-flags #:allow-other-keys)
>                (apply invoke "make" "config-gcc" make-flags)))
> @@ -211,14 +201,14 @@ (define-public yosys
>             python
>             tcl)) ; tclsh for the tests
>      (inputs
> -     (list graphviz
> -           libffi
> -           psmisc
> +     (list libffi
>             readline
> -           tcl
> -           xdot))
> +           tcl))
>      (propagated-inputs
>       (list abc
> +           graphviz ; for dot
> +           psmisc ; for fuser
> +           xdot
>             z3)) ; should be in path for yosys-smtbmc
>      (home-page "https://yosyshq.net/yosys/")
>      (synopsis "FPGA Verilog RTL synthesizer")

Thanks Simon, I've pushed the first 3 patches from this series to the
master branch now.

For the changes regarding propagated-inputs though, I'm not sure this it
the right direction. Firstly, I think it's possible (and maybe
desirable) to keep the 'fix-paths phase, even if the inputs are changed
to be propagated.

I know you say this is related to yosys show in the commit message, can
you elaborate on why these packages are necessary to have in the users
environment?

Thanks,

Chris

Christopher Baines <mail@cbaines.net> writes:
> Thanks Simon, I've pushed the first 3 patches from this series to the
> master branch now.

Nice, thank you.

> I know you say this is related to yosys show in the commit message,
> can you elaborate on why these packages are necessary to have in the
> users environment?

Yosys' "show" command produces its output by building and executing a
shell command that invokes "dot" or "xdot" on the user's data.  The
implementation is in passes/cmds/show.cc[0]; the code for invoking dot
for instance looks like

    #define DOT_CMD "dot -T%s '%s' > '%s.new' && mv '%s.new' '%s'"
    (...)
    std::string cmd = stringf(DOT_CMD, format.c_str(), dot_file.c_str(), out_file.c_str(), out_file.c_str(), out_file.c_str());
    log("Exec: %s\n", cmd.c_str());
    if (run_command(cmd) != 0)
        log_cmd_error("Shell command failed!\n");

Obviously this works only if "dot" is in the user's PATH (as Yosys
blindly assumes), so the graphviz package must be installed as well or
the "show" command will be broken.  Similarly for the "fuser" and "xdot"
executables, which by default are invoked to provide graphical output
(though their use can be overridden by a setting at runtime).

I find this business of generating shell commands a bit ugly but at
least it creates only a loose coupling between the executables: The
shell is free to select a suitable "dot" to run, so the user can
substitute their own as easily as changing their PATH.

In constrast, the existing 'fix-paths phase tightens this coupling
considerably, as it embeds in the DOT_CMD string above the full path to
a specific "dot" executable in the store.  This ties the two executables
together completely, making it very difficult to change which "dot" is
used by Yosys without rebuilding the package.

I see no advantage to coupling the executables together tightly this
way, and in fact it seems counter to what is implied by the code above.
(Note also the "ABCEXTERNAL" build variable adjusted in a previous patch
is provided specifically to allow users to swap in their own version of
abc, which a different Yosys command invokes.)  I'd rather we propagate
the packages needed to ensure Yosys works as expected out-of-the-box,
then leave the user free to override its behaviour as they see fit.

Am Mittwoch, dem 08.02.2023 um 19:35 -0500 schrieb Simon South:
> Yosys' "show" command produces its output by building and executing a
> shell command that invokes "dot" or "xdot" on the user's data.  The
> implementation is in passes/cmds/show.cc[0]; the code for invoking
> dot for instance looks like
> 
>     #define DOT_CMD "dot -T%s '%s' > '%s.new' && mv '%s.new' '%s'"
>     (...)
>     std::string cmd = stringf(DOT_CMD, format.c_str(),
> dot_file.c_str(), out_file.c_str(), out_file.c_str(),
> out_file.c_str());
>     log("Exec: %s\n", cmd.c_str());
>     if (run_command(cmd) != 0)
>         log_cmd_error("Shell command failed!\n");
> 
> Obviously this works only if "dot" is in the user's PATH (as Yosys
> blindly assumes), so the graphviz package must be installed as well
> or the "show" command will be broken.  Similarly for the "fuser" and
> "xdot" executables, which by default are invoked to provide graphical
> output (though their use can be overridden by a setting at runtime).
> 
> I find this business of generating shell commands a bit ugly but at
> least it creates only a loose coupling between the executables: The
> shell is free to select a suitable "dot" to run, so the user can
> substitute their own as easily as changing their PATH.
When propagating inputs, it creates the same tight coupling as
hardcoding would, but with worse UX (think propagating conflicts).


You should instead try:

  std::optional<std::string> dot_bin = which("dot")
  if (!dot_bin)
    dot_bin = "@dot@";
  std::string cmd = string(DOT_CMD, *dot_bin, ...)

Cheers

Liliana Marie Prikler <liliana.prikler@gmail.com> writes:
> You should instead try:

That would be a nice approach but I don't really want to start modifying
the source code.

I'll send out a v3 of the remaining patches that keeps the 'fix-paths
phase (and updates Yosys to 0.26, since that came out yesterday).

Embedding store references directly in source code still feels ugly to
me but I can see how propagating packages just creates a different
problem.  Pity there isn't an elegant way to specify a package should
"use this input, but not exclusively" (though at least there's "guix
build --with-input").

--
Simon South
simon@simonsouth.net