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

chore: skip c tests if env var is present #136

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

rymurr
Copy link
Contributor

@rymurr rymurr commented Jan 10, 2025

While integrating this extension as an out of tree extension using https://github.com/duckdb/duckdb/tree/main/extension#remote-github-repo and building duckdb I found that the C tests don't compile properly: the submodules aren't checked out in this path.

This was the simplest and cleanest way I could find to skip the compilation of the C tests when compiling substrait directly in duckdb (these tests aren't run in that path anyways and the tests still are run as part of normal CI)

@rymurr rymurr requested a review from anshuldata January 10, 2025 10:09
@CLAassistant
Copy link

CLAassistant commented Jan 10, 2025

CLA assistant check
All committers have signed the CLA.

jacques-n
jacques-n previously approved these changes Jan 10, 2025
Copy link
Contributor

@jacques-n jacques-n left a comment

Choose a reason for hiding this comment

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

Seems good for now. Wonder if we should make tests work/run in that path at some point.

@rymurr
Copy link
Contributor Author

rymurr commented Jan 10, 2025

Seems good for now. Wonder if we should make tests work/run in that path at some point.

100% agree. It appears this is the first/only extension that has C tests so is the first one that needs C headers present. I am sure there is a way to tell cmake to change where it looks for include files or do some symlinking but its beyond my cmake skills so wanted to defer until I am more comfortable w cmake

anshuldata
anshuldata previously approved these changes Jan 10, 2025
Copy link
Contributor

@anshuldata anshuldata left a comment

Choose a reason for hiding this comment

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

lgtm

@rymurr rymurr dismissed stale reviews from anshuldata and jacques-n via b5e7414 January 10, 2025 10:54
@rymurr rymurr changed the title Skip c tests if env var is present chore: skip c tests if env var is present Jan 10, 2025
@rymurr rymurr requested a review from anshuldata January 10, 2025 11:06
@anshuldata anshuldata merged commit acbd4ab into substrait-io:main Jan 10, 2025
11 checks passed
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