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

builtins: skip addition of builtins if the library is added previously #771

Merged
merged 2 commits into from Nov 11, 2021

Conversation

umarcor
Copy link
Member

@umarcor umarcor commented Oct 26, 2021

As discussed in #767, this PR modifies the functions for adding the built OSVVM and JSON-for-VHDL. Currently, a try...except block is used:

vunit/vunit/builtins.py

Lines 139 to 142 in 7879504

try:
library = self._vunit_obj.library(library_name)
except KeyError:
library = self._vunit_obj.add_library(library_name)

That approach has two main caveats. On the one hand, the try statement is not case sensitive. Hence, if a library named OSVVM exists, it does not get it when asking for osvvm. That falls back to the exception, which crashes because adding a library is case insensitive. If no crash is produced, additional sources are added to the existing library.

In this PR, _add_library_if_not_exist is added, which lowercases library names to check if it exists. Furthermore, if the library exists already, adding builtins is skipped, instead of mixing the content.

I didn't find any function in the API for retrieving the names of the libraries declared already. Hence, I used [library.lower() for library in self._vunit_obj._project._libraries]. However, we might want to add it in the project class.

except KeyError:
library = self._vunit_obj.add_library(library_name)
library = self._add_library_if_not_exist(
"osvvm", "Library 'OSVVM' previously defined. Skipping addition of builtin OSVVM (2021.09)."
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can we make sure that we will remember to update this message when OSVVM is bumped?

Copy link
Member Author

@umarcor umarcor Nov 9, 2021

Choose a reason for hiding this comment

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

I'm not sure... By navigating the codebase through the GitHub web, you don't get any explicit indication that OSVVM is bumped to a tag. It just points to the commit: https://github.com/OSVVM/OSVVM/tree/71faba779aebe79d3a48ce0abc0a9adc272f99ae.

We might use a test for checking out the history of OSVVM, using git for comparing the commit hash with the tags and find an exact match. Then, in the builtins.py, have the version moved to a variable which the check can import for comparison. However, I'm not sure that complexity is worth. Particularly, whether that should be part of any "regular" test.

We might just add a subsection to http://vunit.github.io/contributing.html about "How to bump a submodule", explaining that we want to bump to tagged commits only (in the case of OSVVM), and that whoever does it should remember to update the string.

When dependabot or some other similar solution supports bumping submodules to tags only, we might actually need an automated procedure.

BTW, the same applies to JSON-for-VHDL below.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the instruction is good enough for now. If we find out we keep missing to update the string we can come up with another solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR now adds a subsection to the docs about bumping submodules.

@eine eine merged commit 18e9c58 into VUnit:master Nov 11, 2021
@umarcor umarcor deleted the skip-builtins branch November 11, 2021 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ThirdParty: JSON Related to JSON-for-VHDL. ThirdParty: OSVVM Related to OSVVM and/or OSVVMLibraries.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants