Skip to content

Support hierarchical library mapping in ModelSim#775

Open
ThomasWismer wants to merge 2 commits intoVUnit:masterfrom
ThomasWismer:master
Open

Support hierarchical library mapping in ModelSim#775
ThomasWismer wants to merge 2 commits intoVUnit:masterfrom
ThomasWismer:master

Conversation

@ThomasWismer
Copy link

The library section of the modelsim.ini file allows to refer to a subordinate INI file using an "others" clause (subordinate files may also contain an "others" clause). The current implementation for collecting all mapped libraries ignores the "others" clause. With this change, all library mappings from all subordinate files are included.

write_modelsimini(cfg, self._sim_cfg_file_name)

def _get_mapped_libraries(self):
def _get_mapped_libraries(self, cfg_file_name=None):
Copy link
Member

Choose a reason for hiding this comment

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

Is the addition of this argument related to the description of the PR? It seems to be a different feature.

Copy link
Author

Choose a reason for hiding this comment

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

This allows the same method to be used for subordinate INI files, see recursive call below on line 229.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I overlooked that the call was recursive.

if "others" in libraries:
other_libraries = self._get_mapped_libraries(libraries["others"])
# current mappings take precedence over others
libraries = {**other_libraries, **libraries}
Copy link
Member

Choose a reason for hiding this comment

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

Can this be a single line?

libraries = {**(self._get_mapped_libraries(libraries["others"])), **libraries}

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I'd prefer two statements for readability, but I can change this if you like.

Copy link
Member

@umarcor umarcor Oct 30, 2021

Choose a reason for hiding this comment

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

It's ok. Just checking that other_libraries is not used somewhere else (it's local to the if clause).

@eine eine added this to the v4.7.0 milestone Oct 31, 2021
@dalex78
Copy link
Contributor

dalex78 commented May 10, 2022

@LarsAsplund this as been reviewed by Umarcor, I suggest to merge and close.

@eine eine requested a review from LarsAsplund May 11, 2022 23:43
@LarsAsplund
Copy link
Collaborator

I think we should probably add a test for this.

@dalex78
Copy link
Contributor

dalex78 commented May 14, 2022

You are right. I don't mind working on that but I do not understand how the tests work. I guess I need to modify the tests/unit/test_modelsim_interface.py but where are specified the content of the tested modelsim.ini ?
If anyone can explain me how the tests are working I would appreciate.

@LarsAsplund
Copy link
Collaborator

@dalex78 The tests create their own ini files. These doesn't have to be real and complete files but just enough to verify that VUnit handles the files as expected. For example, the test setup creates a file containing a single line:

write_file(installed_modelsim_ini, "[Library]")

Some tests, i.e.

def test_copies_modelsim_ini_file_from_install(self):
, don't even use valid ini files to verify behavior

In this case you would need to create ini files containing the others clause and verify that VUnit collects the information correctly.

@ThomasWismer
Copy link
Author

As the author of this pull request, I felt obliged to submit a unit test. @dalex78 Thanks anyway for your offer to take on this "burden".

@std-max
Copy link
Contributor

std-max commented Nov 4, 2022

@LarsAsplund This one might be merged aswell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants