search

duckdb / sqlite_scanner

DuckDB extension to read and write to SQLite databases
MIT License
189 stars 19 forks source link

CI changes for packaging duckdb-wasm AND publishing on nightly bucket #76

Closed carlopi closed 6 months ago

carlopi commented 7 months ago

Other workflows .github/workflows/MacOS.yml & co still have testing infrastructure that is specific to sqlite_scanner.

For now it's a bit wasteful the duplication, but I think it's OK (eventually testing step should go also in).

Update: fixed, stuff has been unified in a single workflow.

Mytherin commented 6 months ago

Do we need to copy and paste the extension-upload.sh script here - can't we use the one from the main repo?

The WASM builds also seem to have a lot of code duplication. Maybe that could be isolated through a WASM_BUILD environment variable (e.g. make wasm WASM_BUILD=eh)?

carlopi commented 6 months ago

Do we need to copy and paste the extension-upload.sh script here - can't we use the one from the main repo?

I think current setting is that repositories based on extension-template should have, at a minimum:

We can look with @samansmink at ways to get rid of extension-upload.sh in general, but for now I think the model requires these to be duplicated. This is true for sure for the general case, but our special case we might do with less code duplication. I will come back to this.

The WASM builds also seem to have a lot of code duplication. Maybe that could be isolated through a WASM_BUILD environment variable (e.g. make wasm WASM_BUILD=eh)? Right, makes sense. This is modelled to the way it's in the extension-template and in duckdb/duckdb/Makefile, but should be done better.

Thanks for the feedback, will come back later after addressing it.

carlopi commented 6 months ago

@Mytherin: I reworked the Makefile part on duckdb-wasm, should be a bit cleaner what's reused and what's specific.

Also, mentioned by @samansmink, we could think of ways to delegate to a common CMake, but that I would move to the next iteration.

On the extra workflow and extra script, I think we have a path forward on that, should that be a blocker or this can go further while working on that on top?

Mytherin commented 6 months ago

I'm fine merging this for now

samansmink commented 6 months ago

I can look into deduplicating the deploy workflow and script (because we now use 1 org, we can do this, 3rd parties will still need the deploy workflow copied). This will require changes to duckdb so for now i would say this is ok. then on 0.10 ( or when switching an extension over to duckdb master ) we can use the shared one in duckdb.

Mytherin commented 6 months ago

Thanks!