Rework bundled compilation to support included extensions #127

ankrgyl · 2023-03-07T07:13:50Z

This change reworks the bundled compilation process so that we can support built-in extensions, like httpfs, to the compilation process. There are a number of changes that support it:

Reuse package_build.build_package from the DuckDB repo to gather source files, including extensions. We no longer use the amalgamation, and end up pulling more than 3,000 files. To keep the number of files in the repo small, we tar.gz the duckdb directory (and untar it on demand). This is all managed from a new script called update_sources.py.
DuckDB is now a submodule of the repo, so that when we want to release a new version, we simply update the repo's submodule and run the script.
Copy the openssl-finding code from rust-openssl, and invoke it when including the httpfs feature
Add a set of new features (1 per duckdb extension) that control what gets bundled in. We can extend this to include all of the builtin plugins to duckdb.

ankrgyl · 2023-03-07T07:14:28Z

CC @wangfenjin

wangfenjin · 2023-03-07T07:23:13Z

You can run sh add_rust_fmt_hook.sh in the folder to make sure code will be formatted before commit

https://github.com/wangfenjin/duckdb-rs/blob/main/add_rustfmt_hook.sh

wangfenjin · 2023-03-07T07:28:51Z

Do you think it's better to depends on the openssl crate directly? Instead of copy all the code to our repo

ankrgyl · 2023-03-07T07:31:49Z

You can run sh add_rust_fmt_hook.sh in the folder to make sure code will be formatted before commit

Awesome, I've done that, but for some reason I think it's still producing slightly different results (like maybe the line lengths are different). Let's see if this most recent CI passes...

Do you think it's better to depends on the openssl crate directly? Instead of copy all the code to our repo

I don't think that's possible unfortunately — I had to make some modifications including returning some values out of the main function. We could potentially upstream some changes to that crate that make this logic importable, but I'd prefer to separate that from this effort.

codecov · 2023-03-07T07:47:09Z

Codecov Report

Merging #127 (0024aff) into main (74cd7f0) will decrease coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
- Coverage   59.54%   59.40%   -0.14%     
==========================================
  Files          26       26              
  Lines        1483     1483              
==========================================
- Hits          883      881       -2     
- Misses        600      602       +2

Impacted Files	Coverage Δ
src/lib.rs	`72.82% <0.00%> (-2.18%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

wangfenjin · 2023-03-07T07:53:18Z

Let's add a feature maybe called extension-full to include all the extensions:
https://github.com/wangfenjin/duckdb-rs/blob/main/Cargo.toml#L27

And then add the flag in ci to make it build passed
https://github.com/wangfenjin/duckdb-rs/blob/main/.github/workflows/rust.yaml#L108-L131

Thanks @ankrgyl

wangfenjin · 2023-03-07T07:59:04Z

It will be better if you can also help add some tests to verify these extensions actually works, the tests will also be used as examples for other users.

The tests can be enabled by #[cfg(feature = "foo")]

wangfenjin · 2023-03-07T07:59:39Z

If you don't have the time to add tests it's fine, I'll do that later after this one merged

wangfenjin · 2023-03-07T11:10:23Z

Is the submodule and tar.gz v0.7.1 ?

wangfenjin · 2023-03-07T11:13:40Z

https://github.com/wangfenjin/duckdb-rs/blob/main/libduckdb-sys/upgrade.sh#L14-L16 we may also update this file to make update version easier in the future. But that fine I can do that after this one get merged

snth · 2023-03-07T20:12:52Z

Thank you @ankrgyl . This looks great and I'll be your first happy user of this as I've been hoping for this functionality for some time.

I especially like what you did with tar.gz on the duckdb directory, nice!

ankrgyl · 2023-03-08T02:18:51Z

If you don't have the time to add tests it's fine, I'll do that later after this one merged

Would prefer if you could do that in a separate change. Thank you for offering!

Is the submodule and tar.gz v0.7.1 ?

Yes although I noticed that yours are generated on an x86 linux machine and mine on an arm64 mac. Maybe we should generate them on one of your machines?

https://github.com/wangfenjin/duckdb-rs/blob/main/libduckdb-sys/upgrade.sh#L14-L16 we may also update this file to make update version easier in the future. But that fine I can do that after this one get merged

I think the new update_sources.py script should encompass all of the functionality from this script. Could you verify that?

ankrgyl · 2023-03-08T02:23:55Z

Additionally, I discovered that the build scripts we pulled from OpenSSL were exit(0)-ing in certain cases (that I encountered on Linux in the CI for QueryScript).

I fixed those in this commit: a3c3ced

wangfenjin · 2023-03-08T02:35:17Z

Yes although I noticed that yours are generated on an x86 linux machine and mine on an arm64 mac. Maybe we should generate them on one of your machines?

I generated on a intel mac. Never notice the difference of this, maybe we should delete the file and always generate the file on the fly in the build process. I'll follow up this issue

ankrgyl · 2023-03-08T03:00:49Z

I generated on a intel mac. Never notice the difference of this, maybe we should delete the file and always generate the file on the fly in the build process. I'll follow up this issue

Ah got it. I think technically it may be ok (?) at least according to CI, but I agree that generating it dynamically is safer.

ankrgyl · 2023-03-08T03:44:12Z

@wangfenjin could you help me with windows? I'm not familiar nor have an environment of my own to test with.

I followed https://github.com/sfackler/rust-openssl/blob/master/.github/workflows/ci.yml#L93 to add VCPKG and hoped that would work...

.github/workflows/rust.yaml

ankrgyl · 2023-03-08T06:15:51Z

If it still not work, we may try sfackler/rust-openssl#1062 (comment)

Just pushed a change to try this (although not sure I got the syntax correct).

This change takes advantage of duckdb/duckdb-rs#127 to directly bundle the relevant extensions into DuckDB. With this change, we can be sure that QueryScript will work across all platforms it compiles on, and startup times are much (~0.5-1s) faster.

libduckdb-sys/openssl/find_normal.rs

libduckdb-sys/build.rs

.github/workflows/rust.yaml

Change-Id: I67dd345659e9957cac8590aad1d7dd8ac190cb53

This reverts commit 96bdc43.

Change-Id: Ifa042c7f4be521a210636c7870e1e225946910e8

wangfenjin · 2023-03-09T11:22:48Z

Some follow up:

Add tests
remove the generated bindings and always generate on the fly
Fix the upgrade.sh to use the Python script

wangfenjin · 2023-03-09T11:24:55Z

Thanks @ankrgyl for your help! I’ll merge this PR now

nickpresta · 2023-03-14T16:59:10Z

@wangfenjin Thanks for merging this change in. Any change a new release can be cut with these features available?

wangfenjin · 2023-03-14T23:54:34Z

We try to keep the version same with duckdb for easy maintenance, for now maybe you can use master branch, I always keep master branch works

snth · 2023-03-15T19:41:58Z

Thank you @ankrgyl . Going to try this for my own project at the earliest opportunity.

I've been waiting for this for a while and looking at the changes in the PR, I certainly couldn't have done it without you so very grateful! 🙏🙌

ankrgyl added 11 commits March 6, 2023 18:08

Get to openssl failure

f548486

It compiles!?

99455d9

Add back in the other flags

98e0b4f

Add openssl code

6e27ca8

Add duckdb as a submodule

03cd71b

Clean up the rest of the code

a90f241

Cleanups

8a8f8b6

Exclude duckdb-sources

c6d03c0

Feature flags

57af4ba

Rework to use a tar.gz

7d9b5df

Add to gitignore

6ba341f

ankrgyl mentioned this pull request Mar 7, 2023

Add httpfs config to support packaging it as an extension duckdb/duckdb#6608

Merged

Keep the bindings around

5f7fcd2

ankrgyl mentioned this pull request Mar 7, 2023

Bundle extensions into DuckDB qslang/queryscript#101

Merged

ankrgyl added 2 commits March 7, 2023 17:23

Fix Linux (exit 0's in the openssl code)

a3c3ced

Remove unused import

1a1930a

Comments

208d299

Fix clippy errors

65e78c9

Only include all extensions when bundling and install OpenSSL on windows

805b0d0

wangfenjin reviewed Mar 8, 2023

View reviewed changes

.github/workflows/rust.yaml Show resolved Hide resolved

ankrgyl added 3 commits March 7, 2023 20:09

Add line

3b8536b

Rework slightly

f707f1d

Try the other solution

e2e8cc4

wangfenjin reviewed Mar 8, 2023

View reviewed changes

libduckdb-sys/openssl/find_normal.rs Show resolved Hide resolved

libduckdb-sys/build.rs Outdated Show resolved Hide resolved

.github/workflows/rust.yaml Outdated Show resolved Hide resolved

.github/workflows/rust.yaml Outdated Show resolved Hide resolved

wangfenjin added 3 commits March 9, 2023 18:19

fix openssl issue

96bdc43

Change-Id: I67dd345659e9957cac8590aad1d7dd8ac190cb53

Revert "fix openssl issue"

1622824

This reverts commit 96bdc43.

fix openssl error

0024aff

Change-Id: Ifa042c7f4be521a210636c7870e1e225946910e8

wangfenjin mentioned this pull request Mar 9, 2023

Follow up for the new bundled code #129

Closed

3 tasks

wangfenjin merged commit 0da3aef into duckdb:main Mar 9, 2023

nickpresta mentioned this pull request Mar 17, 2023

Update submodule URL to use the "Smart HTTP" protocol instead of the "SSH" protocol #137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework bundled compilation to support included extensions #127

Rework bundled compilation to support included extensions #127

ankrgyl commented Mar 7, 2023

ankrgyl commented Mar 7, 2023

wangfenjin commented Mar 7, 2023

wangfenjin commented Mar 7, 2023

ankrgyl commented Mar 7, 2023

codecov bot commented Mar 7, 2023 •

edited

wangfenjin commented Mar 7, 2023

wangfenjin commented Mar 7, 2023

wangfenjin commented Mar 7, 2023

wangfenjin commented Mar 7, 2023

wangfenjin commented Mar 7, 2023

snth commented Mar 7, 2023

ankrgyl commented Mar 8, 2023

ankrgyl commented Mar 8, 2023

wangfenjin commented Mar 8, 2023

ankrgyl commented Mar 8, 2023

ankrgyl commented Mar 8, 2023

ankrgyl commented Mar 8, 2023

wangfenjin commented Mar 9, 2023

wangfenjin commented Mar 9, 2023

nickpresta commented Mar 14, 2023

wangfenjin commented Mar 14, 2023

snth commented Mar 15, 2023

Rework bundled compilation to support included extensions #127

Rework bundled compilation to support included extensions #127

Conversation

ankrgyl commented Mar 7, 2023

ankrgyl commented Mar 7, 2023

wangfenjin commented Mar 7, 2023

wangfenjin commented Mar 7, 2023

ankrgyl commented Mar 7, 2023

codecov bot commented Mar 7, 2023 • edited

Codecov Report

wangfenjin commented Mar 7, 2023

wangfenjin commented Mar 7, 2023

wangfenjin commented Mar 7, 2023

wangfenjin commented Mar 7, 2023

wangfenjin commented Mar 7, 2023

snth commented Mar 7, 2023

ankrgyl commented Mar 8, 2023

ankrgyl commented Mar 8, 2023

wangfenjin commented Mar 8, 2023

ankrgyl commented Mar 8, 2023

ankrgyl commented Mar 8, 2023

ankrgyl commented Mar 8, 2023

wangfenjin commented Mar 9, 2023

wangfenjin commented Mar 9, 2023

nickpresta commented Mar 14, 2023

wangfenjin commented Mar 14, 2023

snth commented Mar 15, 2023

codecov bot commented Mar 7, 2023 •

edited