calyx
calyx copied to clipboard
calyxir
[fud] Process paths with spaces correctly
Fud has lots of trouble with paths that include spaces. Fixing this is a significant undertaking but should be done at some point.
Can you show an example of this? Does the usual trick of adding \ doesn't work? fud uses argparse so not sure if its fixable on our side
Unfortunately, fud quite pervasively commits the sin of constructing command lines as strings, not lists of arguments, and then feeding these strings into a shell to execute. So when it constructs the command calyx -b verilog <path>, if <path> has spaces or other characters that the shell interprets in a special way, it won't work as fud wants.
The original sin is in the shell utility here:
https://github.com/cucapra/calyx/blob/1e5f7d8e27e7abf2c8b4a4202b452550b7f36b83/fud/fud/utils.py#L173-L174
…where it takes in a list but just splats it into a space-separated string. But alas, that's not the only place; this happens a lot throughout fud. Here's another example from the Dahlia stage: https://github.com/cucapra/calyx/blob/1e5f7d8e27e7abf2c8b4a4202b452550b7f36b83/fud/fud/stages/dahlia.py#L23-L30
That one highlights another bit of trickery; the config["stages", self.name, "flags"] string might have spaces in it itself, so it doesn't suffice to pass that list off as a direct (non-shell) subprocess invocation. To make this safe, we'd need to first shlex.split those arguments into pieces and then splice them into the list.
So this is unfortunately not just one bug to be fixed; completely resolving it will require auditing a lot of the way fud works. I have so far, over the years, been embracing the original sin and just resigning myself to the fact that fud doesn't work for ill-behaved paths. But perhaps a reckoning is afoot.
Argh, got it! This is painful indeed. Not sure when this would get prioritized but worth keeping in the log
Linking the main fud2 tracker #1878. Not sure if this issue still exists, so we can maybe close?
I don't think so. It was extremely pervasive and I don't think anyone went around to fix it
Ah, so the issue still exists I guess. Let's keep this open and linked for fud2 implementation