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

Node.js Windows Build #2103

Merged
merged 17 commits into from Aug 10, 2021
Merged

Node.js Windows Build #2103

merged 17 commits into from Aug 10, 2021

Conversation

plecong
Copy link
Contributor

@plecong plecong commented Aug 6, 2021

Hello!

Fan of DuckDB and have been having issues with the Node.js build on Windows, so thought I'd help out with a few of these issues. This PR adds a GitHub Action job that will pre-build the Windows binary and fixes several issues along the way:

One deviation from the current Node.js build for other platforms is that this PR uses a matrix build for Windows instead of nvm used by the Linux/OSX builds as nvm didn't appear to work on Windows. There is NVM for Windows, but the author clearly states that it is not the same thing as nvm, so not pursued further. Due to the matrix build, a new script node_build_win.sh was added instead of using the existing node_build.sh.

Additionally, 523343f and 34327eb address when windows.h is included in physical_load.cpp, however noticed that amalgamation.py has special handling for file_system.cpp to append it at the end. Not sure if this is needed for physical_load.cpp as well.

Copy link
Collaborator

@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks great. Some comments:

Additionally, 523343f and 34327eb address when windows.h is included in physical_load.cpp, however noticed that amalgamation.py has special handling for file_system.cpp to append it at the end. Not sure if this is needed for physical_load.cpp as well.

This should not be necessary anymore. This is a relic from before we "correctly" handled windows.h in our codebase (i.e. by avoiding #define conflicts everywhere). The special handling of file_system.cpp should actually be removed.

src/execution/operator/helper/physical_load.cpp Outdated Show resolved Hide resolved
@plecong
Copy link
Contributor Author

plecong commented Aug 6, 2021

In 7575422, the 'relic' code in amalgamation.py for special handling of file_system.cpp has been removed.

@Mytherin
Copy link
Collaborator

Mytherin commented Aug 6, 2021

Thanks, looks great! Will wait for the tests to pass and then I think it is ready to merge from my side.

@Mytherin
Copy link
Collaborator

Mytherin commented Aug 6, 2021

Perhaps @hannesmuehleisen wants to have a look as he made the node.js driver.

@plecong
Copy link
Contributor Author

plecong commented Aug 6, 2021

@Mytherin Thanks for your review! It does appear that a previous CI build ran into #2100. Will have to see if the latest build with the amalgamation.py changes will pass.

I'm interested in using the Node.js driver to read Parquet files and have a working build locally for #1977 that will be coming after this PR is done.

@Mytherin Mytherin merged commit 9b926c2 into duckdb:master Aug 10, 2021
@Mytherin
Copy link
Collaborator

This seems good to merge now, thanks again for the effort!

@plecong plecong deleted the nodejs_windows_build branch August 10, 2021 21:45
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

2 participants