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

Vunit dependencies #593

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

nopeslide
Copy link

added fields to return value of get_files

  • "fileset": fileset which included the src file
  • "core": name of the core which included the src file
  • "depend": name list of core dependencies

@nopeslide
Copy link
Author

WIP because I do not know if this is the intended way to add such a feature

@olofk
Copy link
Owner

olofk commented Nov 25, 2022

Ok, this is an interesting feature and I'm definitely open to adding more metadata to the EDAM file. I just want to be careful to not expose too much internal FuseSoC details since Edalize is intended to work without FuseSoC (and it is already used by a bunch of other projects).

Adding core is the least controversial in my opinion.

I'm less sure about fileset. To me that feels like an internal FuseSoC detail. Do you need that? I don't see it being used in olofk/edalize#358

I see two issues with depend. The first is that we probably need to parse the list to evaluate the flags before sending it to the EDAM. The other thing is that the EDAM file already contains the dependency tree, so if you know which core a file belongs to, you can figure out its dependencies. So with that, I don't think we need to specify dependencies on a file-level as well.

So, I'm happy to accept a modified patch that only adds the "core" field. If you still want to include the other stuff, then we need to discuss that.

@nopeslide
Copy link
Author

@olofk
I didn't know the edam already contained the dependency tree

@nopeslide nopeslide marked this pull request as ready for review November 28, 2022 14:49
@nopeslide
Copy link
Author

Anything else I missed?
I did no thorough testing, is there any test setup I can use?

@olofk
Copy link
Owner

olofk commented Jan 8, 2023

I tested this just now and there is actually a test that breaks with this change. test_capi2_get_files in https://github.com/olofk/fusesoc/blob/main/tests/test_capi2.py breaks. Also, I think you should convert the Vlnv object to a string first, instead of adding it directly

@olofk
Copy link
Owner

olofk commented Jan 16, 2023

@nopeslide I'm planning to release FuseSoC 2.0 very soon. If we want this patch to go in before that, then we need to fix the failing test and do the VLNV to string conversion.

@nopeslide
Copy link
Author

@olofk rebased on latest main
see olofk/edalize#384

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants