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

Make sqlsmith extension compile #5963

Merged
merged 7 commits into from
Jan 25, 2023
Merged

Conversation

PedroTadim
Copy link
Contributor

Hello! This morning I couldn't compile the sqlsmith extension because of the latest pull request. This should fix it.

Just have one question. Here https://github.com/duckdb/duckdb/blob/master/src/include/duckdb/common/enums/tableref_type.hpp#L23 the numbers have now a gap to keep compatibility with previous releases right?

@Mytherin
Copy link
Collaborator

Thanks! Could you perhaps add BUILD_SQLSMITH to one of the CI runs so this can be caught in the future?

the numbers have now a gap to keep compatibility with previous releases right?

Yes

@@ -281,7 +283,7 @@ jobs:
shell: bash
run: |
mkdir -p build/release
(cd build/release && cmake -DSTATIC_LIBCPP=1 -DBUILD_ICU_EXTENSION=1 -DBUILD_PARQUET_EXTENSION=1 -DBUILD_FTS_EXTENSION=1 -DBUILD_JSON_EXTENSION=1 -DBUILD_EXCEL_EXTENSION=1 -DFORCE_32_BIT=1 -DCMAKE_BUILD_TYPE=Release ../.. && cmake --build .)
(cd build/release && cmake -DSTATIC_LIBCPP=1 -DBUILD_ICU_EXTENSION=1 -DBUILD_PARQUET_EXTENSION=1 -DBUILD_FTS_EXTENSION=1 -DBUILD_JSON_EXTENSION=1 -DBUILD_EXCEL_EXTENSION=1 -DBUILD_SQLSMITH=ON -DFORCE_32_BIT=1 -DCMAKE_BUILD_TYPE=Release ../.. && cmake --build .)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure, but I think this will make for shipping SQLsmith with releases, and probably that can be avoided (both here and on the linux-release-aarch64 job). Probably having it enable only for Main.yml should be enough.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed - adding it to the extension build might also cause it to be shipped by accident. @PedroTadim could you perhaps remove all instances of BUILD_SQLSMITH_EXTENSION except in the Linux (64 Bit) run? That should be sufficient to catch compilation failures early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not aware that the script was used to build production binaries. Time to fix

@Mytherin
Copy link
Collaborator

Thanks!

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.

None yet

4 participants