add 'compile_builtins' deprecation warning

[umarcor]

This PR lays the ground for deprecating compile_builtins and vunit.verilog in upcoming releases (not the next one). As discussed in #559, add_builtins is to be split into add_vhdl_builtins and add_verilog_builtins. In the future, users will need to use them explicitly (as it is required for add_osvvm or add_verification_components).

In this PR, compile_builtins is NOT deprecated yet. A warning is added, which will be shown to all the users which are currently using it. Since it is enabled by default, it will be shown for most of the current users of VUnit. The recommendation is that they can use compile_builtins=False and VU.add_vhdl_builtins now, or wait until the deprecation.

This PR does include a breaking change in the public API, since add_builtins is renamed to add_vhdl_builtins. That is to avoid the warning recommending the users a function that is to be removed (add_builtins). However, since the vast majority of VUnit users should not even be aware that add_builtins exists, this change should not be disruptive.

BTW, for some reason add_builtins was not shown in the documentation. I don't remember whether it was hidden on purpose (because of being redundant to compile_builtins) or because it crashed Sphinx. Anyway, I fixed the docstring and, since it is split, the documentation now includes both add_verilog_builtins and add_vhdl_builtins.

[LarsAsplund]

I'm not sure about this one, We create an extra step in the run script and break backward compatibility but are we really making our life that much simpler? Most users will use either VHDL built-ins or Verilog built-ins so it is convenient to have dedicated classes for that. The smaller use cases are to use VUnit without built-ins or with both VHDL and Verilog built-ins. The latter has no documented solution.

An alternative would be to let VUnitVHDL be an alias for today's VUnit, VUnitVerilog be an alias for the verilog.VUnit and then there is a new VUnitPure (or some other suitable name) that has no built-ins and can support the smaller use cases.

[umarcor]

We create an extra step in the run script

In practice, most intermediate and advanced users need at least one extra step, either for adding VUnit's VCs or OSVVM or some other solution providing verification components. I consider the addition of a line/step benefitial, as explained below.

break backward compatibility but are we really making our life that much simpler?

With regard to backward compatibility, this PR is adding a warning for most users, but not introducing a significant breaking change yet. It is mostly for announcing a future modification (#764).

With regard of the purpose itself, I agree with @kraigher's comment in #507 (comment), which motivated the creation of #559.

I believe we are making our lives easier because we are fixing an inconsistency in the design of the API. Currently, all the optional HDL libraries can be added through explicit methods, except for the VHDL builtins, which are implicit and enabled by default.

On the one hand, creation of the Verilog class was required because of that inconsistency. Having builtins managed as other optional resources makes both the code easier (more consistent) and communication less misleading. It is currently misleading that an specific class is required for Verilog, but not for VHDL. In fact, the codebases are 99% the same, and Verilog is not any second class language from VUnit's Python perspective. I believe we could communicate better if all users could just import the same class, without having to interpret what language were examples of the Python features written for.

On the other hand, if builtins are enabled by default, the random package and verification components might be enabled as well, as soon as VCs are out of beta. From a project point of view, I don't see why would we want to enable some only, rather than most opt-out. Per reduction, it's more sensible to achieve consistency (i.e. simplicity) by having none than having all.

Most users will use either VHDL built-ins or Verilog built-ins so it is convenient to have dedicated classes for that.

I believe that having dedicated VHDL and Verilog classes would make sense if they provided anything other than replacing a single 10-20 character line. From a codebase point of view, those classes are additional files of 25 lines each, at least. Hence, I don't think it's worth.

IMHO, a significantly more interesting use case of classes inheriting from VUnit, would be VUnit.OSVVMLibraries or VUnit.UVVM or VUnit.cocotb or VUnit.SVUnit, etc. Those classes can be potentially co-developed by third-parties, even if they are kept in this repository. That would be a clear separation of concerns which we want to communicate as a community; also benefitial from a pure technical coordination point of view.

The smaller use cases are to use VUnit without built-ins or with both VHDL and Verilog built-ins. The latter has no documented solution.

Currently, some Verilog user might copy a run script for a VHDL example using the default VUnit class, and call add_verilog_builtins, which might fail (it's undocumented). They need to disable the default builtins (which has no indication about being VHDL specific), or they need to use a different class and revert the add_verilog_builtins line. I think it would better if adding both builtins was not possible unless done explicitly.

Nonetheless, my main interest is making users explicitly aware of WHEN are the builtins added, not deciding whether to add them or not. I assume that most users want to use VUnit's HDLs builtins, because that's in their own benefit and what most VUnit tutorials/examples use. This is related to the "With VUnit's VCs After" cases in https://umarcor.github.io/osvb/egs/axi4stream.html#id1, and also to #771. The proposal in #771 is to skip OSVVM if such library was added to the project AFTER creating the project but BEFORE adding VUnit's VCs. We might extend that concept: if users add any source to the vunit_lib matching the name of an internal VUnit VHDL source, before adding the builtins, it overrides the internal source when add_builtins is called.

Such feature might be useful for handling #776. That is, third-parties such as OSVVM, UVVM, etc. might override some of the alert/log VHDL sources, without us having to support each possible override through the Python API.

As a side comment, instead of adding option VU.add_builtins(use_external_log=...) (https://github.com/VUnit/vunit/pull/776/files#diff-48f5c6a881c24567ab1180f0533d62d4f71da00a815c504a2606d08aee4b1908R31), we should have method VU.with_external_osvvm_log() called BEFORE VU.add_builtins. Then, third-parties might provide their own with_external_*_log() functions without having to modify VUnit's codebase. They can inherit the VUnit class to add their method(s), but they don't need to override the add_builtins. This comes back to the consistency mentioned above: adding methods to the public API, not options to existing methods.

An alternative would be to let VUnitVHDL be an alias for today's VUnit, VUnitVerilog be an alias for the verilog.VUnit and then there is a new VUnitPure (or some other suitable name) that has no built-ins and can support the smaller use cases.

I think such change would be difficult to communicate. Having VUnitVHDL and VUnitVerilog would be sensible from a consistency point of view. However, as commented above, I think it's overkill to have classes with no more functionality than replacing a single line. Nonetheless, my main concern is that both VUnit and VUnitPure would be very misleading. The logical solution would be for VUnit not to have builtins, and therefore, for VUnitPure not to exist. However, if we did so, we'd be back to the same breaking change: requiring users to change the class, instead of adding a method call.

If VUnit is kept untouched, there is no real motivation for users to adopt VUnitVHDL. We would be fragmenting the code style in the user base without obvious benefit. At some point, we will have users using VUnit.from_argv(), VUnitVHDL.from_argv(), VUnit.from_argv(compile_builtins=False), VUnitVHDL.from_argv(compile_builtins=False) or VUnitPure.from_argv(). Instead, we might homogenize those five syntaxes with just:

VU = VUnit.from_argv()

# Optionally add VUnit's builtin HDL utilities for checking, logging, communication...
# See http://vunit.github.io/vhdl_libraries.html.
VU.add_vhdl_builtins()
# or
# VU.add_verilog_builtins()

That'd be valid for either VHDL or Verilog, and even for both of them (although undocumented at the moment). From a communication point of view, it'd be much easier regardless of the language and features:

1. Create a VUnit project.
2. Add HDL resources in any order: VUnit's builtins, VCs, from third-parties, your own. Be careful about name collisions if you are new to the ecosystem; at the same time, be creative with the order if you are an advanced user.
3. Setup your testbenches.
4. Run.

Overall, I think there is no other solution for fixing the inconsistency than splitting the builtins from from_argv and from_args; and the only way to do that is through a breaking change that does not add the builtins by default. I believe v5.0.0 is a good opportunity to introduce this change (in the context of #763), because we won't change it in a later release alone. If you want to keep builtins added by default, I do understand, but I don't think there is anything we might improve; the actual implementation is the best given that constraint. Then., I would close this and #764; maybe keep the branches to revisit this discussion in a couple of years.

[LarsAsplund]

My main concern is not really the extra characters but the backward compatibility issue. There is a large amount of VUnit-related material online today. There is the material we as VUnit maintainers are in directly control over but then we have all the non-maintainer material produced on LinkedIn, Twitter, StackOverflow, Reddit, Youtube, and other forums. We have university course material, commercial training providers, company-internal material etc. Most of this material is at the very basic level to help new users getting started. My fear is that if the first experience people have with VUnit is a compile error from the simulator when running a simple hello example they might walk away.

I'm not so concerned with deprecating compile_builtins as that is more advanced usage.

An alternative to the VUnitPure class would be to add a very verbose warning/error message saying exactly what needs to be done to the run script. VUnit could recognize that the files to be compiled contains references to the built-ins and then provide an instruction on how to fix that. New users may have no experience with Python so ideally we should provide a message showing the line where the VUnit object is created and what line that needs to be added after that. It should be at the "explain to me like I'm 5 years old"-level. Something like:

As of VUnit 5.0.0 the run script is required to bla, bla, bla.

Solution:
After line xyz in path/to/run/script add <missing method calls>:

prj = VUnit.from_argv()
prj.add_...     # <- Add this line!

For more information see link/to/documentation

[umarcor]

I think that is reasonable. Actually, the proposal in #764 is to show the warning unconditionally. I am good with making it significantly more visible, showing a multi-line and very explicit comment as you proposed. We can try to automatically decide when to hide it, but I would also be good with showing it always. Maybe having an specific option to "hide deprecation warnings", which advanced users can enable on purpose.

Moreover, I would add very visible notices at the top of the documentation/site. Apart from updating the examples (done in #764), I would update the tutorial in tdd-intro. I agree we cannot update all the content, university materials and the content of training providers. However, I think we can make it so that anyone visiting any of our repos/sites can stumble upon a warning/note.

Unfortunately, I don't think we can communicate much now (in this PR). I would like to merge this and release v4.7.0, so we can then merge breaking changes into master and update the documentation acccordingly. We can use several months with breaking changes in master, before releasing v5.0.0. We might even release v5.0.0-rc0.

[LarsAsplund]

What I mean is that if someone takes an online example not coming from the official Github pages, for example:

my_first_vunit = VUnit.from_argv()
lib = my_first_vunit.add_library("lib")
lib.add_source_files(Path(__file__).parent / "*.vhd")
my_first_vunit.main()

and then run that with v5.0.0 they would get something like this

WARNING - C:\test_vunit\tb_test.vhd: failed to find a primary design unit 'vunit_context' in library 'vunit_lib'
Compiling into lib: tb_test.vhd failed
=== Command used: ===
C:\ghdl\bin\ghdl -a --workdir=C:\test_vunit\vunit_out\ghdl\libraries\lib --work=lib --std=08 -PC:\test_vunit\vunit_out\ghdl\libraries\vunit_lib -PC:\test_vunit\vunit_out\ghdl\libraries\lib C:\test_vunit\tb_test.vhd

=== Command output: ===
C:\test_vunit\tb_test.vhd:7:9: cannot find resource library "vunit_lib"
C:\test_vunit\tb_test.vhd:8:19: unit "vunit_context" not found in library "vunit_lib"

Compile failed

For someone with no previous experience this might be the reason not to try any further. However, the first warning shows that VUnit already knows what is going on and can produce an instruction for how to fix the problem. Not a generic instruction but one actually showing what to do in the run script being used. For example,

WARNING - C:\test_vunit\tb_test.vhd: failed to find a primary design unit 'vunit_context' in library 'vunit_lib'

As of VUnit 5.0.0 the run script is required to bla, bla, bla.

Solution:
After line 4 in C:\test_vunit\run.py add a call to add_vhdl_builtins():

my_first_vunit = VUnit.from_argv()
my_first_vunit.add_vhdl_builtins()    # <- Add this line!

For more information see link/to/documentation

I'm hoping that Python inspect can help us doing this but that needs to be checked.

[umarcor]

@LarsAsplund, my bad. I thought that the error was produced by the simulator, not by VUnit. Hence, I was not aware that we could show the message conditionally exactly after the warning. I will look into inspecting the source of the user.

[umarcor]

@LarsAsplund, see the last commit I pushed to #764. Example output: https://github.com/dbhi/vunit/runs/4168970304?check_suite_focus=true#step:3:399

----------------------------- Captured stderr call -----------------------------
WARNING - /src/examples/vhdl/array/src/test/tb_sobel_x.vhd: failed to find a primary design unit 'vunit_context' in library 'vunit_lib'
CRITICAL - As of VUnit v5, HDL builtins are not compiled by default.
To preserve the functionality, the run script is now required to explicitly use
methods 'add_vhdl_builtins()' or 'add_verilog_builtins()'.

Solution - Add a call to 'add_vhdl_builtins()' in the following location:

  File: /src/examples/vhdl/array/run.py
  Line: 21

As shown below:

20|  
21|  VU = VUnit.from_argv()
22|+ VU.add_vhdl_builtins()  # Add this line!
23|  VU.add_osvvm()
24|  

For more information see http://vunit.github.io/hdl_libraries.html.
/src/.tox/py310-acceptance-ghdl/lib/python3.10/site-packages/vunit/sim_if/ghdl.py:252: ...

Ref: https://stackoverflow.com/questions/4152963/get-name-of-current-script-in-python

[LarsAsplund]

I'm getting close to approve this. What about projects with mixed VHDL and Verilog testbenches. Is that just a matter of adding both built-ins now?

[umarcor]

I'm getting close to approve this. What about projects with mixed VHDL and Verilog testbenches. Is that just a matter of adding both built-ins now?

Yes. These are the expected changes for all the cases I can think of:

# VHDL only old
from vunit import VUnit
VU = VUnit.from_argv()

# VHDL only new
from vunit import VUnit
VU = VUnit.from_argv()
VU.add_vhdl_builtins() # Add this!

# Verilog only old
from vunit.verilog import VUnit
VU = VUnit.from_argv()

# Verilog only new
from vunit import VUnit # Change this!
VU = VUnit.from_argv()
VU.add_verilog_builtins() # Add this!

# Mixed old (default class)
from vunit import VUnit
VU = VUnit.from_argv()
VU.add_verilog_builtins()

# Mixed old (Verilog class)
from vunit.verilog import VUnit
VU = VUnit.from_argv()
VU.add_vhdl_builtins()

# Mixed new (VHDL first)
from vunit import VUnit
VU = VUnit.from_argv()
VU.add_vhdl_builtins()
VU.add_verilog_builtins()

# Mixed new (Verilog first)
from vunit import VUnit
VU = VUnit.from_argv()
VU.add_verilog_builtins()
VU.add_vhdl_builtins()