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

Removed configure_vivado_synth_tool from project #343

Merged
merged 9 commits into from
Nov 15, 2023

Conversation

yonnorc42
Copy link
Contributor

@yonnorc42 yonnorc42 commented Nov 13, 2023

Fixes #314

bfasst/ninja_flows/flow_utils.py Outdated Show resolved Hide resolved
@reillymck
Copy link
Contributor

This is looking pretty good! If you would just delete the configure_vivado_tool function from flow_utils then it should be good to go. (pending the unit tests passing).

@KeenanRileyFaulkner
Copy link
Contributor

I think any issues with the dictionary instance, if they exist, will come up because of how our configure rule works. It has to blow away the ninja build file and remake it. Potential issues would be if we change a script file and run ninja again, and the issue would have to do with how the flow args are re-read into the build file as seen at the bottom of the ninja_flow_manager.py file

@yonnorc42
Copy link
Contributor Author

It looks like one of the unit tests (so far) failed because of the .get("synth")

@jgoeders
Copy link
Member

@yonnorc42 OK, looks like we will need a few more fixes to resolve this. In places like this, you will need to do something like:

if flow_args is None:
  flow_args = []

You may be tempted to change the default flow_args argument value to [], but this is a bad idea.

@yonnorc42
Copy link
Contributor Author

@jgoeders I didn't see your comment before I did another request

@yonnorc42
Copy link
Contributor Author

@yonnorc42 OK, looks like we will need a few more fixes to resolve this. In places like this, you will need to do something like:

if flow_args is None:
  flow_args = []

You may be tempted to change the default flow_args argument value to [], but this is a bad idea.

In this case, wouldn't I make it an empty dictionary instead of an empty list?

@jgoeders
Copy link
Member

@yonnorc42

In this case, wouldn't I make it an empty dictionary instead of an empty list?

Yes, you're correct.

@yonnorc42
Copy link
Contributor Author

@yonnorc42 OK, looks like we will need a few more fixes to resolve this. In places like this, you will need to do something like:

if flow_args is None:
  flow_args = []

You may be tempted to change the default flow_args argument value to [], but this is a bad idea.

When I implement that if statement into the required files, it passes the unittests now, but it fails the pylint tests, and I think it's saying I failed because I have duplicate lines of code in all those places. This makes me think it wants me to use a function instead of a duplicate if statement in all those files, but I feel like that's essentially what we had originally with configure_vivado_synth_tool

@yonnorc42
Copy link
Contributor Author

I think this is ready to merge, unless you want me to change it

@jgoeders jgoeders merged commit dad4a19 into main Nov 15, 2023
13 checks passed
@jgoeders jgoeders deleted the move_configure_method branch November 15, 2023 23:51
@KeenanRileyFaulkner KeenanRileyFaulkner changed the title Moved configure_vivado_synth_tools to flow_utils.py Removed configure_vivado_synth_tool from project Nov 16, 2023
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.

Move configure_vivado_tool out of Flow class
4 participants