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

[circt-test] fix SymbiYosys integration test #7886

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

Conversation

unlsycn
Copy link
Contributor

@unlsycn unlsycn commented Nov 24, 2024

#7756 adds the SymbiYosys test runner and requires the sby feature, but did not add the corresponding available feature in lit.cfg.py, so this test never seems to run, and #7763 introduces a syntax error, a backslash in the f-string expression part, in the Python script.
Unlike #7884, there is no need to modify PATH variable for the script since sby should be in the PATH if we can check it in CMake.

@unlsycn unlsycn marked this pull request as ready for review November 24, 2024 17:54
@unlsycn
Copy link
Contributor Author

unlsycn commented Nov 24, 2024

cc @fabianschuiki

@unlsycn
Copy link
Contributor Author

unlsycn commented Nov 25, 2024

@fabianschuiki
Copy link
Contributor

Good thinking! We should definitely add SymbiYosys to the set of tools installed for integration tests.

Copy link
Contributor

@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks a lot for fixing this!

@fabianschuiki
Copy link
Contributor

If you would like to land this yourself once CI passes, feel free to obtain commit access to LLVM. Otherwise let me know and I can merge things for you 😃

@fabianschuiki
Copy link
Contributor

@unlsycn I have added SymbiYosys and a more up-to-date build of Yosys to the integration test CI image (v18.0) in #7895. I'll land that in a few minutes if CI passes. Once that's in main, you could try to rebase this PR onto main and see if the integration test CI picks up the SymbiYosys installation and the test you fixed here now runs 🙂

@fabianschuiki
Copy link
Contributor

It looks like something might be broken in the integration image's sby setup 😞, sorry about that. This is going to be a bit annoying to debug. One approach could be to tweak the circt-test-runner-sby.py file to not just say "see xyz.log", but to make it read and print that xyz.log. That would allow us to see the sby output log in the GitHub action.

@unlsycn
Copy link
Contributor Author

unlsycn commented Nov 27, 2024

It looks like something might be broken in the integration image's sby setup 😞, sorry about that. This is going to be a bit annoying to debug. One approach could be to tweak the circt-test-runner-sby.py file to not just say "see xyz.log", but to make it read and print that xyz.log. That would allow us to see the sby output log in the GitHub action.

circt/images#35

Copy link
Contributor

@darthscsi darthscsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@unlsycn
Copy link
Contributor Author

unlsycn commented Dec 19, 2024

It looks like something might be broken in the integration image's sby setup 😞, sorry about that. This is going to be a bit annoying to debug. One approach could be to tweak the circt-test-runner-sby.py file to not just say "see xyz.log", but to make it read and print that xyz.log. That would allow us to see the sby output log in the GitHub action.

circt/images#35

We can rerun the ci now since it is merged.

@unlsycn
Copy link
Contributor Author

unlsycn commented Dec 20, 2024

It looks like something might be broken in the integration image's sby setup 😞, sorry about that. This is going to be a bit annoying to debug. One approach could be to tweak the circt-test-runner-sby.py file to not just say "see xyz.log", but to make it read and print that xyz.log. That would allow us to see the sby output log in the GitHub action.

circt/images#35

We can rerun the ci now since it is merged.

hmmmm..I forgot that the image has not been released. I can pass all the tests on my machine with the latest docker image.

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.

3 participants