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

Use yowasp_yosys instead of system-installed yosys #50

Closed
wants to merge 3 commits into from

Conversation

daniellimws
Copy link
Contributor

Following the discussion in #40, change to use the wasm-bundled version of yosys as in yowasp_yosys instead of the system-installed yosys (symbiflow projects use conda to install it, but this might not be the case for other users).

One benefit of this is that it allows us to control the version of yosys used by the user. This is important because there might be breaking changes in yosys, such as #13, and it wouldn't be ideal to cater for multiple versions of yosys.

One small downside is this takes about twice the duration compared to native yosys but I don't think this really matters.

Signed-off-by: Daniel Lim Wee Soong weesoong.lim@gmail.com

Signed-off-by: Daniel Lim Wee Soong <weesoong.lim@gmail.com>
@mithro
Copy link
Member

mithro commented Aug 5, 2020

Can we make this a configuration option?

@daniellimws
Copy link
Contributor Author

Yup sure, so a setting in conf.py?

@mithro
Copy link
Member

mithro commented Aug 5, 2020

Yeap! Sounds good.

@daniellimws
Copy link
Contributor Author

Now we support both types of yosys installations. @mithro Do you think we need to implement tests for both use cases? I can't think of any straightforward way of doing so without setting up a whole sphinx project for each of the two cases.

Signed-off-by: Daniel Lim Wee Soong <weesoong.lim@gmail.com>
Signed-off-by: Daniel Lim Wee Soong <weesoong.lim@gmail.com>
@mithro
Copy link
Member

mithro commented Aug 9, 2020

@daniellimws - Can we just set up travis to run in the two configurations?

@daniellimws
Copy link
Contributor Author

@mithro I'm not good with Travis so if you got any ideas you could try it first

@rw1nkler
Copy link
Contributor

I believe that the mechanism for tests provided in the #52 can be useful here.

@mithro mithro requested a review from rw1nkler September 1, 2020 15:50
@mithro
Copy link
Member

mithro commented Sep 1, 2020

@rw1nkler -- Can you take a look at this PR? If @daniellimws doesn't currently have the time, can you take it over and get it merged?

@daniellimws
Copy link
Contributor Author

Hi, yes I currently am quite busy with school work. I might be free next week though. Feel free to take over if you want :).

Copy link
Contributor

@rw1nkler rw1nkler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left my comments below, they are addressed in the #55


def setup(app):
...
VerilogDiagram.use_yowasp = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, the global settings for the extension should be based on variables added to sphinxconf.py file.
See: https://www.sphinx-doc.org/en/master/extdev/appapi.html#sphinx.application.Sphinx.add_config_value

@@ -268,7 +281,7 @@ def diagram_netlistsvg(ipath, opath, module='top', flatten=False):
run_yosys(
src=ipath,
cmd = """\
prep -top {top} {flatten}; cd {top}; write_json {ojson}
prep -top {top} {flatten}; cd {top}; write_json -compat-int {ojson}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compatibility mode seems to work incorectly when added to the standard conda Yosys version.

@rw1nkler rw1nkler closed this in #55 Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants