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

coremanager: Pick preferred cores for virtual VLNVs #668

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

a-will
Copy link
Contributor

@a-will a-will commented Jan 20, 2024

Add "conflicts" constraints for each core implementing virtual VLNVs, so no two cores implementing the same virtual VLNV may be selected simultaneously.

This change should prevent more than a single implementing core from being selected as the implementation for a virtual VLNV, and when there is an explicit dependency in the tree, that one should be selected.

Extend test_virtual() cases to show desired dependency resolution when
one core depends on a virtual VLNV and another core has an explicit
dependency on an implementing core for that virtual VLNV. The solver
should choose that one implementing core and no other.

Signed-off-by: Alexander Williams <awill@opentitan.org>
Add "conflicts" constraints for each core implementing virtual VLNVs, so
no two cores implementing the same virtual VLNV may be selected
simultaneously.

This change should prevent more than a single implementing core from
being selected as the implementation for a virtual VLNV, and when there
is an explicit dependency in the tree, that one should be selected.

Signed-off-by: Alexander Williams <awill@opentitan.org>
@a-will
Copy link
Contributor Author

a-will commented Jan 20, 2024

The prior test case did not actually test the suggested specs outlined in #235

Now to use the bus, you need to explicitly choose which "provider" (implementation) of vendor:bus:config_default you want to include

Before the first commit here, that test case did not have a core that explicitly chose its implementation, so the bugs have been hidden.

This change still doesn't handle the case where no implementation is specified in the dependency tree (which probably should result in an error). In that case, fusesoc's dependency solver would pick effectively arbitrarily.

@olofk
Copy link
Owner

olofk commented Jan 21, 2024

I'm not super familiar with the code around the virtual / provides support, since that was written by other people, but I see the issue you are solving here and I trust it's doing the right thing. Just to be sure, is this one good to go, or do you want to fix the issue you mention in the last paragraph first?

@a-will
Copy link
Contributor Author

a-will commented Jan 22, 2024

Offhand, I'm not sure how best to fix fusesoc's not reporting an error if no implementing core is part of the dependency tree. The modeling of a package repo with the SAT solver may not allow us to express the relationships we want.

So... this PR is at least an improvement that picks the desired core when it is specified. Good to go, I'd say 😄

@olofk olofk merged commit dc2cb4f into olofk:main Jan 22, 2024
14 checks passed
@olofk
Copy link
Owner

olofk commented Jan 22, 2024

FYI, I'm also unable to improve error messages. I have tried, but failed miserably at understanding the internals of the dependency manager I shoe-horned into FuseSoC. I really want to replace that with a custom one for several reasons, but until then we will have to live with it.

Picked and pushed! Thank you for your contributions

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