Skip to content

Support hierarchical library mapping in ModelSim#775

Open
ThomasWismer wants to merge 2 commits into
VUnit:masterfrom
ThomasWismer:master
Open

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

Conversation

@ThomasWismer

Copy link
Copy Markdown

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.

Comment thread vunit/sim_if/modelsim.py
write_modelsimini(cfg, self._sim_cfg_file_name)

def _get_mapped_libraries(self):
def _get_mapped_libraries(self, cfg_file_name=None):

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread vunit/sim_if/modelsim.py
if "others" in libraries:
other_libraries = self._get_mapped_libraries(libraries["others"])
# current mappings take precedence over others
libraries = {**other_libraries, **libraries}

Copy link
Copy Markdown
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
Copy Markdown
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.

@umarcor umarcor Oct 30, 2021

Copy link
Copy Markdown
Member

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

dalex78 commented May 10, 2022

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Collaborator

I think we should probably add a test for this.

@dalex78

dalex78 commented May 14, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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

std-max commented Nov 4, 2022

Copy link
Copy Markdown
Contributor

@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.

7 participants