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

[CAPI] Add circt-capi target and build it in CI #7017

Merged
merged 1 commit into from
May 9, 2024

Conversation

fabianschuiki
Copy link
Contributor

@fabianschuiki fabianschuiki commented May 9, 2024

Add a circt-capi target that depends on all C API libraries. Introduce a new add_circt_public_c_api_library CMake function that wraps around the MLIR equivalent, but also adds a dependency from circt-capi. Make at least the short integration tests CI job build the circt-capi target to ensure it has a bit of CI coverage.

@fabianschuiki
Copy link
Contributor Author

Thanks @dtzSiFive and @hailongSun2000 for pointing out that this was an issue.

Copy link
Contributor

@dtzSiFive dtzSiFive 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!

Having check-circt depend on circt-capi so that check-circt build-tests CAPI doesn't feel quite right.

Both because we build things unneeded for testing, and because there may be others omitted (if not needed by a test) when what we really want is to ensure developers and CI are building all the things.

The macro and new target seems useful regardless 👍.

Looks like ninja circt works in a unified build, and just ninja works normally. Perhaps having our CI do whichever of those is the way to go? WDYT?

@fabianschuiki
Copy link
Contributor Author

fabianschuiki commented May 9, 2024

ninja circt doesn't work for me in my unified build 😢 But I agree that having check-circt depend on something that isn't needed to run the unit tests feels ugly. Something like a circt-all or circt target would be nice, as you suggest.

I can just make CI run ninja check-circt circt-capi for now. On the other hand, check-circt is probably the thing used by a lot of people in their dev loop, and having them type ninja check-circt circt-capi also feels kind of weird. Having a single target do the CI and dev loop thing would be nice. Would we want something like ninja circt to build everything and then run the checks as well? Does that include integration tests?

Add a `circt-capi` target that depends on all C API libraries. Introduce
a new `add_circt_public_c_api_library` CMake function that wraps around
the MLIR equivalent, but also adds a dependency from `circt-capi`. Make
at least the short integration tests CI job build the `circt-capi`
target to ensure it has a bit of CI coverage.
@fabianschuiki fabianschuiki changed the title [CAPI] Add circt-capi target and build it on check-circt [CAPI] Add circt-capi target and build it in CI May 9, 2024
@dtzSiFive
Copy link
Contributor

ninja circt doesn't work for me in my unified build 😢

Oops, I.. nevermind. Me either! 😅 .

On the other hand, check-circt is probably the thing used by a lot of people in their dev loop, and having them type

Yes, totally, agreed. I too tend to build-test with check-circt, and agreed it'd be good to have this fixed for dev loop neatly.

Regarding integration tests--
Upstream MLIR moved them all under /test with an option (and lit config val) for enabling/disabling integration tests, maybe this is the way to go?

https://reviews.llvm.org/D97241

Assuming integration tests know how to gate themselves on their tool/environmental dependencies, including them makes sense to me!

@fabianschuiki
Copy link
Contributor Author

Oh that's neat! I keep tripping over the integration tests not being included as part of the regular tests. But that's probably just me being a derp 😬

I'll land this with the circt-capi removed from the check-circt dependencies. I have also added the circt-capi explicitly to the short integration tests, just to get a little bit of coverage going. It would be great to do a ninja circt in CI instead, in a separate "Build CIRCT" step. Maybe a follow-up PR?

@fabianschuiki fabianschuiki merged commit 719bbfd into main May 9, 2024
4 checks passed
@fabianschuiki fabianschuiki deleted the fschuiki/capi-tests branch May 9, 2024 18:21
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