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

Removing black-parrot-tools and black-parrot-sdk #2660

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dpetrisko
Copy link

These don't actually do anything for the synlig built, but take a lot of space and time to clone

@hzeller
Copy link
Collaborator

hzeller commented Nov 18, 2024

Drive-by comment: Maybe in the future the tests should be somewhat separate from submodules; have a file with a list of git URLs and commit-hashes that are fetched when tests are run, but not by default as submodule (as submodule sortof imply that it is a necessary build-resource).

@pgielda
Copy link
Member

pgielda commented Nov 18, 2024

I think the reason is the bot that tries to autoupdate those submodules...

@hzeller
Copy link
Collaborator

hzeller commented Nov 18, 2024

but could it also just auto-update a file with <git-url> <hash> tuples ?

@pgielda
Copy link
Member

pgielda commented Nov 18, 2024

I was not the one that set it up, it was also just a drive-by comment ;) Maybe @kgugala knows if dependabot can handle anything else than submodules.

Note that the CI does git submodule sync && git submodule update --init --recursive third_party/{surelog,yosys} so it goes around the problem this way.

@pgielda
Copy link
Member

pgielda commented Nov 18, 2024

(note that this MR cannot be merged as-is because it makes black parrot tests red)

@dpetrisko
Copy link
Author

Oh, I see there’s a useless Makefile dependency in BP to the tools and sdk. I’ll push a fix and then bump BP to fix it

@dpetrisko
Copy link
Author

dpetrisko commented Nov 21, 2024

Okay, I think that bump should make tests green

@kgugala
Copy link
Member

kgugala commented Nov 22, 2024

I believe you would also need to remove the BP tests (as those failed here).

Why do you need to drop the submodule? All the optional submodules (the ones we use for testing, but not required to build the tool) are marked with update = none (see https://github.com/chipsalliance/synlig/blob/main/.gitmodules#L34). This means that those submodules are not fetched if you run git submodule update --init --recursive. To fetch the optional submodule you have to explicitly point it. In CI we do it with this script https://github.com/chipsalliance/synlig/blob/main/tests/scripts/common.sh#L25 called from https://github.com/chipsalliance/synlig/blob/main/tests/scripts/run_large_designs.sh#L117

@pgielda
Copy link
Member

pgielda commented Nov 22, 2024

why would we be removing BP tests?

@dpetrisko
Copy link
Author

I think the tests failed as result of the bump, looks the patch is no longer valid. Will investigate

@dpetrisko
Copy link
Author

Hm not sure why the ASIC synthesis fails if the other two are working. Any insights?

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.

4 participants