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

Closed
wants to merge 1 commit into from
Closed

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 nopeslide force-pushed the vunit_dependencies branch from d941b36 to e48ab6a Compare May 18, 2023 12:13
@nopeslide nopeslide force-pushed the vunit_dependencies branch from e48ab6a to 7305d9c Compare May 20, 2023 09:31
@nopeslide
Copy link
Author

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

@olofk
Copy link
Owner

olofk commented Dec 19, 2024

This completely slipped my mind, and I also realized that this is actually implemented since some time ago, so I'll thank you for the contributions and closing this now.

@olofk olofk closed this Dec 19, 2024
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.

2 participants