-
Notifications
You must be signed in to change notification settings - Fork 19
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
More fixes for Wasm builds, and move configure one layer up #106
Conversation
@@ -58,6 +58,8 @@ endif | |||
# Write the platform we are building for | |||
platform: configure/platform.txt | |||
|
|||
configure: venv platform extension_version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configure should be a target defined in the extension repository I think: This way you can override the target easily without requiring to inline the whole makefile
|
||
wasm_mvp: | ||
DUCKDB_PLATFORM=wasm_mvp make configure release move_wasm_extension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the wasm_mvp
, wasm_eh
wasm_threads
targets can stay in the main makefile? I think that's a decently clean split? that way we do move the internals into extension-ci-tools but the extension repo can still control what is done for those targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, you mean moving:
+wasm_mvp:
+ DUCKDB_PLATFORM=wasm_mvp make configure release move_wasm_extension
+
+wasm_eh:
+ DUCKDB_PLATFORM=wasm_eh make configure release move_wasm_extension
+
+wasm_threads:
+ DUCKDB_PLATFORM=wasm_coi make configure release move_wasm_extension
to the extension own makefile, and keeping the infra here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we agreed: wasm_* targets stay in the inner makefiles, users are supposed to invoke it like:
DUCKDB_PLATFORM=wasm_eh make configure release
0c59a3b
to
ce31455
Compare
lgtm! |
This PR allows to remove
configure
target from extension-template-rs, and have the wasm logic moved completely to the template.