-
Notifications
You must be signed in to change notification settings - Fork 561
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
fix radiant bug 'Mal-formed command line - please check for extra quotes in macro' #1718
Conversation
litex/build/lattice/radiant.py
Outdated
@@ -153,7 +153,9 @@ def tcl_path(path): return path.replace("\\", "/") | |||
|
|||
# Add include paths | |||
vincpath = ";".join(map(lambda x: tcl_path(x), self.platform.verilog_include_paths)) | |||
tcl.append("prj_set_impl_opt {include path} {\"" + vincpath + "\"}") | |||
if vincpath != "": # Do not add quotes if vincpath is empty, else Radiant complains |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty string is seen as False
so:
if vincpath and vincpath.strip():
[...]
```
is more pythoniic and catch empty string and string with only blank.
litex/build/lattice/radiant.py
Outdated
tcl.append("prj_set_impl_opt {include path} {\"" + vincpath + "\"}") | ||
if vincpath != "": # Do not add quotes if vincpath is empty, else Radiant complains | ||
vincpath = "\"" + vincpath + "\"" | ||
tcl.append("prj_set_impl_opt {include path} {" + vincpath + "}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is include path
is a mandatory option? ie when vincpath
is empty this line may be skipped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to build fine without include path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trabucayre see latest commit. Happy to make further changes / take feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but l.159 must be removed.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops. Thanks for spotting that.
Applied. Thanks @slagernate ! |
See #1596.