Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fud] Process paths with spaces correctly #1549

Open
EclecticGriffin opened this issue Jun 6, 2023 · 7 comments
Open

[fud] Process paths with spaces correctly #1549

EclecticGriffin opened this issue Jun 6, 2023 · 7 comments
Labels
C: fud Calyx Driver

Comments

@EclecticGriffin
Copy link
Collaborator

Fud has lots of trouble with paths that include spaces. Fixing this is a significant undertaking but should be done at some point.

@EclecticGriffin EclecticGriffin added the C: fud Calyx Driver label Jun 6, 2023
@rachitnigam
Copy link
Contributor

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

@sampsyo
Copy link
Contributor

sampsyo commented Jun 6, 2023

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.

@rachitnigam
Copy link
Contributor

Argh, got it! This is painful indeed. Not sure when this would get prioritized but worth keeping in the log

@EclecticGriffin
Copy link
Collaborator Author

Linking the main fud2 tracker #1878. Not sure if this issue still exists, so we can maybe close?

@rachitnigam
Copy link
Contributor

Did it get fixed in fud?

@EclecticGriffin
Copy link
Collaborator Author

I don't think so. It was extremely pervasive and I don't think anyone went around to fix it

@rachitnigam
Copy link
Contributor

Ah, so the issue still exists I guess. Let's keep this open and linked for fud2 implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: fud Calyx Driver
Projects
None yet
Development

No branches or pull requests

3 participants