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

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

Merged
merged 9 commits into from
Dec 5, 2023

Conversation

carlopi
Copy link
Contributor

@carlopi carlopi commented Dec 1, 2023

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.

@carlopi carlopi force-pushed the ci-duckdb-wasm branch 2 times, most recently from b4d267f to 417885b Compare December 2, 2023 05:35
@carlopi carlopi requested a review from Mytherin December 4, 2023 09:25
@Mytherin
Copy link
Contributor

Mytherin commented Dec 4, 2023

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
Copy link
Contributor Author

carlopi commented Dec 4, 2023

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:

  • a driver like .github/workflow/MainDistributionPipeline.yml
  • a copy of .github/workflow/_extension_deploy.yml
  • a copy of scripts/extension-upload.sh

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
Copy link
Contributor Author

carlopi commented Dec 4, 2023

@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
Copy link
Contributor

Mytherin commented Dec 4, 2023

I'm fine merging this for now

@samansmink
Copy link
Contributor

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 Mytherin merged commit 241497f into duckdb:main Dec 5, 2023
20 checks passed
@Mytherin
Copy link
Contributor

Mytherin commented Dec 5, 2023

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.

3 participants