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 lit config for circt-bmc #7884

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

unlsycn
Copy link
Contributor

@unlsycn unlsycn commented Nov 24, 2024

#7857 introduces a Python script to test circt-bmc, but there are two problems:

  1. We need to add the script to the substitution list in lit.cfg.py so that lit can correctly substitute the path in the test.
  2. The script invokes circt-opt but the executable is not in the PATH variable causing python cannot find it. This PR simply adds circt_tool_dir to PATH to resolve it, but I think a better way is to get rid of the python script and allow lit to substitute the path.

@unlsycn
Copy link
Contributor Author

unlsycn commented Nov 24, 2024

cc @fabianschuiki

@unlsycn
Copy link
Contributor Author

unlsycn commented Nov 25, 2024

One confusing point is that the Github CI can pass this test without path substitution and the additional PATH variable, which seems to be an existing CIRCT binary path in the PATH.
https://github.com/llvm/circt/actions/runs/11998261862/job/33447711728 (this is the CI job for #7886)

@fabianschuiki
Copy link
Contributor

Hmmm, this is interesting. The circt-test-runner-*.py scripts are installed alongside the circt-test executable. But you're right, if circt-test is expanded by lit instead of found through the PATH, then the scripts, which are searched for in the PATH, won't be found either. Can we tweak lit to add the binary build directory to the PATH, such that we don't have to muck with the PATH variable in RUN lines?

@fabianschuiki
Copy link
Contributor

Ah sorry, I just now realized that this is for circt-opt, not the benchmark script. In the lit config, it should be possible to add the CIRCT tool build path to the config.path list, which will be used to populate the PATH variable. That might fix things.

@unlsycn
Copy link
Contributor Author

unlsycn commented Nov 25, 2024

Ah sorry, I just now realized that this is for circt-opt, not the benchmark script. In the lit config, it should be possible to add the CIRCT tool build path to the config.path list, which will be used to populate the PATH variable. That might fix things.

Yeah, it's a possible fix that I wasn't sure made sense before. Maybe we should just consider it a workaround to work with the Python script rather than using it to replace the substitution flow.

@@ -65,6 +65,7 @@

# Tweak the PATH to include the tools dir.
llvm_config.with_environment('PATH', config.llvm_tools_dir, append_path=True)
config.environment['PATH'] = os.pathsep.join([config.circt_tools_dir, config.environment['PATH']])
Copy link
Contributor Author

@unlsycn unlsycn Nov 25, 2024

Choose a reason for hiding this comment

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

TestingConfig should not have this member?

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.

Really nice, thanks!

@fabianschuiki
Copy link
Contributor

Would you like to merge this yourself, or do you want me to do it?

@unlsycn
Copy link
Contributor Author

unlsycn commented Nov 26, 2024

Would you like to merge this yourself, or do you want me to do it?

I'd be really grateful if you could help me merge them for now. Thanks so much!

@unlsycn unlsycn force-pushed the test-bmc-fix branch 2 times, most recently from 399f68e to c723acf Compare November 26, 2024 05:24
Signed-off-by: unlsycn <unlsycn@unlsycn.com>
@fabianschuiki fabianschuiki merged commit 2d1b963 into llvm:main Nov 26, 2024
4 checks passed
@unlsycn
Copy link
Contributor Author

unlsycn commented Nov 26, 2024

Oh..sorry, my wrong rebase screwed everything up!

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