From debbugs-submit-bounces@debbugs.gnu.org Wed Feb 08 19:35:44 2023 Received: (at 60429) by debbugs.gnu.org; 9 Feb 2023 00:35:44 +0000 Received: from localhost ([127.0.0.1]:56942 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pPuue-0001Do-7Q for submit@debbugs.gnu.org; Wed, 08 Feb 2023 19:35:44 -0500 Received: from mailout.easymail.ca ([64.68.200.34]:48648) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pPuuZ-0001DX-05 for 60429@debbugs.gnu.org; Wed, 08 Feb 2023 19:35:42 -0500 Received: from localhost (localhost [127.0.0.1]) by mailout.easymail.ca (Postfix) with ESMTP id 86BAFE878F; Thu, 9 Feb 2023 00:35:31 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at emo08-pco.easydns.vpn Received: from mailout.easymail.ca ([127.0.0.1]) by localhost (emo08-pco.easydns.vpn [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 8360Zc9BPyRQ; Thu, 9 Feb 2023 00:35:31 +0000 (UTC) Received: from earth (23-233-96-72.cpe.pppoe.ca [23.233.96.72]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mailout.easymail.ca (Postfix) with ESMTPSA id 0A71AE8747; Thu, 9 Feb 2023 00:35:31 +0000 (UTC) From: Simon South To: Christopher Baines Subject: Re: [bug#60429] [PATCH v2 4/5] gnu: yosys: Propagate external dependencies. References: <62b19db61f34b63e37ba204fd9691b97d5c245bb.1673202235.git.simon@simonsouth.net> <87357g6pfz.fsf@cbaines.net> Date: Wed, 08 Feb 2023 19:35:30 -0500 In-Reply-To: <87357g6pfz.fsf@cbaines.net> (Christopher Baines's message of "Wed, 08 Feb 2023 17:14:59 +0000") Message-ID: <87v8kb7k19.fsf@simonsouth.net> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Score: -2.3 (--) X-Debbugs-Envelope-To: 60429 Cc: 60429@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -3.3 (---) Christopher Baines 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. -- Simon South simon@simonsouth.net [0] https://github.com/YosysHQ/yosys/blob/1979e0b1f2482dbf0562f5116ab444280a377773/passes/cmds/show.cc