Skip to content

add_submodule method to import vunit projects#558

Closed
FranzForstmayr wants to merge 8 commits intoVUnit:masterfrom
FranzForstmayr:dev/submodule
Closed

add_submodule method to import vunit projects#558
FranzForstmayr wants to merge 8 commits intoVUnit:masterfrom
FranzForstmayr:dev/submodule

Conversation

@FranzForstmayr
Copy link
Contributor

This PR enable vunit to import separate managed vunit projects at once.

The vunit class creating has been modified therefore and acts now as a singleton. So every further vunit call returns the same instance, so the project's source files get merged together.

Typical use cases are a top level design like a CPU which imports several peripheries as a submodule. Each periphery is managed separately as vunit project. When importing the module there's no need to track the source files (changes) of the top project anymore.
Another use case would be libraries. So instead of importing several files from a vhdl library only a run.py from the library gets imported.

I opened an issue a while ago, #353 but it is already closed. I needed the feature anyway so i decided to give it a try.

Testers are welcome to try the new feature.

@FranzForstmayr FranzForstmayr changed the title Dev/submodule add_submodule method to import vunit projects Oct 6, 2019
@kraigher
Copy link
Collaborator

kraigher commented Oct 6, 2019

For this type of change it is good to ask before starting the implementation. I think adding a submodule concept to VUnit is a good idea but I do not think this is the right approach. I do not like that there is shared global state in the VUnit object. It will actually break existing code that I know of and will also break the VUnit acceptance tests.

Another concern is a submodule concept should offer modularity and isolation. By having everything being inside one global VUnit object submodules can change settings of other submodules. I would want a solution where submodule A cannot touch anything in submodule B.

Also a run.py file is not a good carrier for defining a submodule since it will contain parsing of command line arguments and other side effects related to the environment. The run.py file is meant to serve as an executable and a means to run the tests. I think submodules should be declared outside of run.py files in files that just serve to declare the submodule.

I think the right aproach is creating a VUnitModule class or similiar which is defined in a file separate from the run.py file. If this approach was taken the user might not even need a run.py file in many cases and VUnit could come with a built-in "run.py" that just accepts a module file.

@eine
Copy link
Collaborator

eine commented Oct 6, 2019

I think that this 'submodule' concept is related to #511.

@FranzForstmayr
Copy link
Contributor Author

FranzForstmayr commented Oct 10, 2019

Thanks for the feedback, I already thought there will be some negative points, but I had finished the code, so I wanted to try.
Can you please the explain the VUnitModule a little bit more?

It will actually break existing code that I know of and will also break the VUnit acceptance tests.

The regression test fails atm because vunit isn't creating a new instance every call anymore. So a library with a specific name is already added and an error is thrown.
Ok, i don't know your way to organize the projects. I tested my own design (with only one vunit call everytime) and the official examples.

@kraigher
Copy link
Collaborator

I think the right way forward is to create an issue for making a submodule system for VUnit. In this issue we need to gather all currently known requirements. Then we can discuss a solution.

@GlenNicholls
Copy link
Contributor

Is this issue going to continue being tracked here or will it move elsewhere once requirements are solidified?

@kraigher
Copy link
Collaborator

kraigher commented Nov 8, 2019

I would need to be tracked elsewhere. This goes in the direction of making VUnit into a package manager which I believe should be a separate project. A package manager also needs to work for synthesis.

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.

4 participants

Comments