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

add_manual_dependency has no effect #617

Open
nfrancque opened this issue Jan 27, 2020 · 9 comments
Open

add_manual_dependency has no effect #617

nfrancque opened this issue Jan 27, 2020 · 9 comments
Labels

Comments

@nfrancque
Copy link

nfrancque commented Jan 27, 2020

Root issue is with a configuration of the following form:

configuration foo_cfg_empty_bah of foo_tb.foo is
  for rtl -- foo(rtl)
    for u_bar : bar_dsn.bar
      for rtl -- bar(rtl)
        for u_bah : foo_tb.bah       
          use entity foo_tb.bah(empty);
        end for; -- u_bah
      end for; -- rtl
    end for; -- u_bar
  end for; -- rtl
end configuration foo_cfg_empty_bah;

I know it looks weird. Use case is verification team creating an empty version of a sub-block to insert a BFM in its place. This method is the least number of duplicate copies of files, so is ideal but could obviously create separate copies of design files if necessary.

Vunit does not recognize the need to have foo(rtl) compiled before the configuration and a compile error occurs. Running with -k twice is a consistent workaround. If we can resolve this that would be ideal for my use case, but you would expect the manual dependency to be a guaranteed failsafe.

So, after doing the following (semi-psuedocode with the libraries):

No manual dependency
add_manual_dependency(foo_dsn(foo-rtl.vhd), foo_tb(foo_cfg_empty_bah.vhd))
add_manual_dependency(foo_tb(foo_cfg_empty_bah.vhd), foo_dsn(foo-rtl.vhd))

There is no effect on the file order between any 3 of these options. Running with --debug does display inverted printout between the two calls, so it is getting to

"Adding dependency: %s depends on %s", end.name, start.name
.

I did the order flipping because it appears the internal function call ordering may be flipped between

add_dependency(depends_on, source_file)
and
self._manual_dependencies.append((source_file, depends_on))
. Please let me know if I am misinterpreted that.

Anyone have any thoughts on how to approach?

@nfrancque
Copy link
Author

nfrancque commented Jan 27, 2020

Adding tree to make the configuration part make more sense. Here is the directory structure/hierarchy.

├── bar
│   ├── bah
│   │   └── src
│   │       └── dsn
│   │           ├── bah-rtl.vhd
│   │           └── bah.vhd
│   └── src
│       └── dsn
│           ├── bar-rtl.vhd
│           └── bar.vhd
└── src
    ├── cfg
    │   └── foo_cfg_empty_bah.vhd
    ├── dsn
    │   ├── foo-rtl.vhd
    │   └── foo.vhd
    └── tb
        ├── bah-empty.vhd
        ├── bah.vhd -> ../../bar/bah/src/dsn/bah.vhd
        ├── foo-rtl.vhd -> ../dsn/foo-rtl.vhd
        └── foo.vhd -> ../dsn/foo.vhd

Where we have some other scripts that auto organize everything into folder_dsn and folder_tb, which is why the symlinks are required to get everything into foo_tb.

So we are attempting to set the architecture of foo.u_bar.u_bah to empty and are unable to set foo-rtl.vhd as a dependency of foo_cfg_empty_bah.vhd despite printouts saying that it is being added.

@kraigher
Copy link
Collaborator

The feature is documented here. Seems the text is a bit wierd and should be updated.
http://vunit.github.io/py/vunit.html?highlight=add_dependency_on#vunit.ui.source.SourceFile.add_dependency_on

The configuration file should depend on the architecture file:

cfg = lib.add_source_file('configuration.vhd')
arch = lib.add_source_file('architecture.vhd')
cfg.add_dependency_on(arch)

Does this correspond to you usage?

@nfrancque
Copy link
Author

It looks like add_dependency_on ends up calling add_manual_dependency.

vunit/vunit/ui/source.py

Lines 149 to 150 in b06474f

self._project.add_manual_dependency(
self._source_file, depends_on=private_source_file

I hadn't seen this and am calling ui._project.add_manual_dependency directly.

But otherwise yes in concept that is my use case.

@kraigher
Copy link
Collaborator

Are you not seeing the correct dependencies when running with --log-level=debug?

Btw:
The _project field is a private field which means end users should not call it directly. In python it is a convention to have names starting with _ meaning private. Relying on private members can cause undefined behavior and breakage on future updates since we only make an effort to keep the Public API stable and may change any private field at any time for any reason.

@nfrancque
Copy link
Author

nfrancque commented Jan 28, 2020

That's the odd part, I do see the dependency output with --log-level debug. Just no impact.

It looks like some change has happened by switching to add_dependency_on. Let me test that for a bit and get back to you, thanks for suggestion.

And yes I am acutely aware of that unfortunately. Have had to mangle a few private fields on this particular project since they didn't want me to fork and there's some odd use cases here. Good learning experience though, I feel much more capable of helping you guys out now.

@nfrancque
Copy link
Author

Ok. With switching to add_dependency_on the compile order is changed as I expect and things work.

Do you have any interest in looking into the reason it didn't pick up on that particular configuration, or should we chalk that up as another need to switch parser type thing? I am fine with either option personally.

@kraigher
Copy link
Collaborator

Understanding that configuration is not something that would be nice to do with the current regex based parser. We would need a real parser.

One option we have would be to add support for declaring a dependency with a comment. It would be more convenient and local in this case.

@nfrancque
Copy link
Author

nfrancque commented Jan 29, 2020

That's an interesting thought. In this case I am having users create a yaml to specify everything and one of the fields is

manual_dependencies:
  lib1(file1.vhd): lib2(file2.vhd)

However it does seem like it could be hard to find for someone coming along later to edit the code.

It would be nice to see the standardization of some sort of manifest like what you've done with rust_hdl. That seems a more likely place to specify these things. We are doing this on another project now where each block has manifest.yaml with all file/tool information in it, I go and read them to pass to vunit/Vivado/Spyglass/whatever.

Here you can identify the blocks that your block depends on, if necessary.

That discussion is probably best had on tsfpga, though.

@eine
Copy link
Collaborator

eine commented Jan 29, 2020

@nfrancque, refs #347, #352, #353, #577 and #529. This is a proof of concept I wrote, to have a default run.py to read a JSON configuration file. The comments are all the features from the Python API that might potentially be supported.

Moreover, ghdl-language-server currently uses a hdl-prj.json, that is undocumented. It can be deprecated in favour of any other equivalent but documented format that can be shared with other projects, say rust_hdl's. Ideally, GHDL (the simulation and synthesis tool) would accept the same configuration file.

Apart from that, are you comfortable or, at least don't you mind, using Gitter? There have been recent discussions regarding the modularisation of VUnit and how it could improve the integration with other tools, such as tsfpga or cocotb. I think it would be interesting to create a project, say VHDL/cfg, where maintainers of VUnit, rust_hdl, GHDL, tsfpga, cocotb, edalize, fusesoc, etc. can collaborate to define a format that all of them can use as input. I believe we should define a structure with multiple bindings to support multiple file formats JSON/YAML/TOML and multiple languages. For now, we'd only need Python, Rust, JSON and TOML.

@eine eine added the Parsing label Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants